Re: [PATCH for-next 5/7] io_uring: post msg_ring CQE in task context

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

On 12/5/22 15:18, Jens Axboe wrote:
> On 12/5/22 8:12?AM, Dylan Yudaken wrote:
>> On Mon, 2022-12-05 at 04:53 -0700, Jens Axboe wrote:
>>> On 12/4/22 7:44?PM, Pavel Begunkov wrote:
>>>> We want to limit post_aux_cqe() to the task context when -
>>>>> task_complete
>>>> is set, and so we can't just deliver a IORING_OP_MSG_RING CQE to
>>>> another
>>>> thread. Instead of trying to invent a new delayed CQE posting
>>>> mechanism
>>>> push them into the overflow list.
>>>
>>> This is really the only one out of the series that I'm not a big fan
>>> of.
>>> If we always rely on overflow for msg_ring, then that basically
>>> removes
>>> it from being usable in a higher performance setting.
>>>
>>> The natural way to do this would be to post the cqe via task_work for
>>> the target, ring, but we also don't any storage available for that.
>>> Might still be better to alloc something ala
>>>
>>> struct tw_cqe_post {
>>> ????????struct task_work work;
>>> ????????s32 res;
>>> ????????u32 flags;
>>> ????????u64 user_data;
>>> }
>>>
>>> and post it with that?

What does it change performance wise? I need to add a patch to
"try to flush before overflowing", but apart from that it's one
additional allocation in both cases but adds additional
raw / not-batch task_work.

>> It might work to post the whole request to the target, post the cqe,
>> and then return the request back to the originating ring via tw for the
>> msg_ring CQE and cleanup.
> 
> I did consider that, but then you need to ref that request as well as
> bounce it twice via task_work. Probably easier to just alloc at that
> point? Though if you do that, then the target cqe would post later than
> the original. And potentially lose -EOVERFLOW if the target ring is
> overflown...

Double tw is interesting for future plans, but yeah, I don't think
it's so much of a difference in context of this series.

-- 
Pavel Begunkov