Re: [RFC net-next v3 23/29] io_uring: allow to pass addr into sendzc

[Stefan Metzmacher <metze@samba.org>]

Hi Pavel,

>>> Thanks for giving a thought about the API, are you trying
>>> to use it in samba?
>>
>> Yes, but I'd need SENDMSGZC and then I'd like to test,
>> which variant gives the best performance. It also depends
>> on the configured samba vfs module stack.
> 
> I can send you a branch this week if you would be
> willing to try it out as I'll be sending the "msg" variant
> only for 5.21

I'm not sure I'll have time to do runtime testing,
but it would be great to have a look at the code and give some comments
based on that.

>> I think it should be:
>>
>>                    if (up->arg)
>>                            slot->tag = up->arg;
>>                    if (!slot->notif)
>>                            continue;
>>                    io_notif_slot_flush_submit(slot, issue_flags);
>>
>> or even:
>>
>>                    slot->tag = up->arg;
>>                    if (!slot->notif)
>>                            continue;
>>                    io_notif_slot_flush_submit(slot, issue_flags);
>>
>> otherwise IORING_RSRC_UPDATE_NOTIF would not be able to reset the tag,
>> if notif was never created or already be flushed.
> 
> Ah, you want to update it for later. The idea was to affect only
> those notifiers that are flushed by this update.
> ...

notif->cqe.user_data = slot->tag; happens in io_alloc_notif(),
so the slot->tag = up->arg; here is always for the next IO_SENDZC.

With IORING_RSRC_UPDATE_NOTIF linked to a IORING_OP_SENDZC(with IORING_RECVSEND_NOTIF_FLUSH)
I basically try to reset slot->tag to the same (or related) user_data as the SENDZC itself.

So that each SENDZC generates two CQEs with the same user_data belonging
to the same userspace buffer.


> I had a similar chat with Dylan last week. I'd rather not rob SQE of
> additional u64 as there is only addr3 left and then we're fully packed,
> but there is another option we were thinking about based on OVERRIDE_TAG
> feature I scrapped from the final version of zerocopy patches.
> 
> Long story short, the idea is to copy req->cqe.user_data of a
> send(+flush) request into the notification CQE, so you'll get 2 CQEs
> with identical user_data but they can be distinguished by looking at
> cqe->flags.
> 
> What do you think? Would it work for you?

I guess that would work.

>>>> I'm also wondering what will happen if a notif will be referenced by the net layer
>>>> but the io_uring instance is already closed, wouldn't
>>>> io_uring_tx_zerocopy_callback() or __io_notif_complete_tw() crash
>>>> because notif->ctx is a stale pointer, of notif itself is already gone...
>>>
>>> io_uring will flush all slots and wait for all notifications
>>> to fire, i.e. io_uring_tx_zerocopy_callback(), so it's not a
>>> problem.
>>
>> I can't follow :-(
>>
>> What I see is that io_notif_unregister():
>>
>>                  nd = io_notif_to_data(notif);
>>                  slot->notif = NULL;
>>                  if (!refcount_dec_and_test(&nd->uarg.refcnt))
>>                          continue;
>>
>> So if the net layer still has a reference we just go on.
>>
>> Only a wild guess, is it something of:
>>
>> io_alloc_notif():
>>          ...
>>          notif->task = current;
>>          io_get_task_refs(1);
>>          notif->rsrc_node = NULL;
>>          io_req_set_rsrc_node(notif, ctx, 0);
>>          ...
>>
>> and
>>
>> __io_req_complete_put():
>>                  ...
>>                  io_req_put_rsrc(req);
>>                  /*
>>                   * Selected buffer deallocation in io_clean_op() assumes that
>>                   * we don't hold ->completion_lock. Clean them here to avoid
>>                   * deadlocks.
>>                   */
>>                  io_put_kbuf_comp(req);
>>                  io_dismantle_req(req);
>>                  io_put_task(req->task, 1);
>>                  ...
>>
>> that causes io_ring_exit_work() to wait for it.> It would be great if you or someone else could explain this in detail
>> and maybe adding some comments into the code.
> 
> Almost, the mechanism is absolutely the same as with requests,
> and notifiers are actually requests for internal purposes.
> 
> In __io_alloc_req_refill() we grab ctx->refs, which are waited
> for in io_ring_exit_work(). We usually put requests into a cache,
> so when a request is complete we don't put the ref and therefore
> in io_ring_exit_work() we also have a call to io_req_caches_free(),
> which puts ctx->refs.

Ok, thanks.

Would a close() on the ring fd block? I guess not, but the exit_work may block, correct?
So a process would be a zombie until net released all references?

metze