Re: [PATCH v10 2/5] io-uring: add napi busy poll support

[Stefan Roesch <shr@devkernel.io>]

Jens Axboe <axboe@kernel.dk> writes:

> On 4/26/23 7:41?PM, Jens Axboe wrote:
>>> +static void io_napi_multi_busy_loop(struct list_head *napi_list,
>>> +		struct io_wait_queue *iowq)
>>> +{
>>> +	unsigned long start_time = busy_loop_current_time();
>>> +
>>> +	do {
>>> +		if (list_is_singular(napi_list))
>>> +			break;
>>> +		if (!__io_napi_busy_loop(napi_list, iowq->napi_prefer_busy_poll))
>>> +			break;
>>> +	} while (!io_napi_busy_loop_should_end(iowq, start_time));
>>> +}
>>
>> Do we need to check for an empty list here?
>>
>>> +static void io_napi_blocking_busy_loop(struct list_head *napi_list,
>>> +		struct io_wait_queue *iowq)
>>> +{
>>> +	if (!list_is_singular(napi_list))
>>> +		io_napi_multi_busy_loop(napi_list, iowq);
>>> +
>>> +	if (list_is_singular(napi_list)) {
>>> +		struct io_napi_ht_entry *ne;
>>> +
>>> +		ne = list_first_entry(napi_list, struct io_napi_ht_entry, list);
>>> +		napi_busy_loop(ne->napi_id, io_napi_busy_loop_should_end, iowq,
>>> +			iowq->napi_prefer_busy_poll, BUSY_POLL_BUDGET);
>>> +	}
>>> +}
>>
>> Presumably io_napi_multi_busy_loop() can change the state of the list,
>> which is why we have if (cond) and then if (!cond) here? Would probably
>> warrant a comment as it looks a bit confusing.
>
> Doesn't look like that's the case? We just call into
> io_napi_multi_busy_loop() -> napi_busy_loop() which doesn't touch it. So
> the state should be the same?
>
> We also check if the list isn't singular before we call it, and then
> io_napi_multi_busy_loop() breaks out of the loop if it is. And we know
> it's not singular when calling, and I don't see what changes it.
>
> Unless I'm missing something, which is quite possible, this looks overly
> convoluted and has extra pointless checks?

I'll fix it.