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

[Jens Axboe <axboe@kernel.dk>]

On 12/6/22 8:59 PM, Pavel Begunkov wrote:
> On 12/6/22 16:06, Jens Axboe wrote:
>> On 12/6/22 3:42?AM, Pavel Begunkov wrote:
>>> 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 adds alloc+free for each one, and overflow flush needed on the
>> recipient side. It also adds a cq lock/unlock, though I don't think that
>> part will be a big deal.
> 
> And that approach below does 2 tw swings, neither is ideal but
> it feels like a bearable price for poking into another ring.
> 
> I sent a series with the double tw approach, should be better for
> CQ ordering, can you pick it up instead? I don't use io_uring tw
> infra of a ring the request doesn't belong to as it seems to me
> like shooting yourself in the leg.

Yeah I think that's the right choice, it was just a quick hack on
my end to see if it was feasible. But it's not a good fit to use
our general tw infra for this.

-- 
Jens Axboe