Re: IORING_CQE_F_COPIED

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/19/22 17:12, Stefan Metzmacher wrote:
> Hi Pavel,
> 
>>> As I basically use the same logic that's used to generate SO_EE_CODE_ZEROCOPY_COPIED
>>> for the native MSG_ZEROCOPY, I don't see the problem with IORING_CQE_F_COPIED.
>>> Can you be more verbose why you're thinking about something different?
>>
>> Because it feels like something that should be done roughly once and in
>> advance. Performance wise, I agree that a bunch of extra instructions in
>> the (io_uring) IO path won't make difference as the net overhead is
>> already high, but I still prefer to keep it thin. The complexity is a
>> good point though, if only we could piggy back it onto MSG_PROBE.
>> Ok, let's do IORING_CQE_F_COPIED and aim 6.2 + possibly backport.
> 
> Thanks!
> 
> Experimenting with this stuff lets me wish to have a way to
> have a different 'user_data' field for the notif cqe,
> maybe based on a IORING_RECVSEND_ flag, it may make my life
> easier and would avoid some complexity in userspace...
> As I need to handle retry on short writes even with MSG_WAITALL
> as EINTR and other errors could cause them.
> 
> What do you think?
> 
>> First, there is no more ubuf_info::zerocopy, see for-next, but you can
>> grab space in io_kiocb, io_kiocb::iopoll_completed is a good candidate.
> 
> Ok I found your "net: introduce struct ubuf_info_msgzc" and
> "net: shrink struct ubuf_info" commits.
> 
> I think the change would be trivial, the zerocopy field would just move
> to struct io_notif_data..., maybe as 'bool copied'.
> 
>> You would want to take one io_uring patch I'm going to send (will CC
>> you), with that you won't need to change anything in net/.
> 
> The problem is that e.g. tcp_sendmsg_locked() won't ever call
> the callback at all if 'zc' is false.
> 
> That's why there's the:
> 
>                          if (!zc)
>                                  uarg->zerocopy = 0;
> 
> Maybe I can inverse the logic and use two variables 'zero_copied'
> and 'copied'.
> 
> We'd start with both being false and this logic in the callback:> 
> if (success) {
>      if (unlikely(!nd->zero_copied && !nd->copied))
>         nd->zero_copied = true;
> } else {
>      if (unlikely(!nd->copied)) {
>         nd->copied = true;
>         nd->zero_copied = false;
>      }
> }

Yep, sth like that should do, but let's guard against
spurious net_zcopy_put() just in case.

used = false;
copied = false;

callback(skb, success, ubuf) {
	if (skb)
		used = true;
	if (!success)
		copied = true;
}
complete() {
	if (!used || copied)
		set_flag(IORING_CQE_F_COPIED);
}

> And __io_notif_complete_tw still needs:
> 
>          if (!nd->zero_copied)
>                  notif->cqe.flags |= IORING_CQE_F_COPIED;

Which can be shoved in a custom callback


>> And the last bit, let's make the zc probing conditional under IORING_RECVSEND_* flag,
>> I'll make it zero overhead when not set later by replacing the callback.
> 
> And the if statement to select a highspeed callback based on
> a IORING_RECVSEND_ flag is less overhead than
> the if statements in the slow callback version?

I'm more concerned about future changes around it, but there won't
be extra ifs.

#define COMMON_FLAGS (RECVSEND_FIRST_POLL|...)
#define ALL_FLAGS (COMMON_FLAGS|RECVSEND_PROBE)

if (flags & ~COMMON_FLAGS) {
	if (flags & ~ALL_FLAGS)
		return err;
	if (flags & RECVSEND_PROBE)
		set_callback(notif);
}

-- 
Pavel Begunkov