Re: [PATCH 2/6] io_uring: add io_file_can_poll() helper

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

On 2/7/24 02:15, Jens Axboe wrote:
> On 2/6/24 5:57 PM, Pavel Begunkov wrote:
>> On 2/6/24 16:22, Jens Axboe wrote:
>>> This adds a flag to avoid dipping dereferencing file and then f_op
>>> to figure out if the file has a poll handler defined or not. We
>>> generally call this at least twice for networked workloads.
>>
>> Sends are not using poll every time. For recv, we touch it
>> in io_arm_poll_handler(), which is done only once, and so
>> ammortised to 0 for multishots.
> 
> Correct
> 
>> Looking at the patch, the second time we might care about is
>> in io_ring_buffer_select(), but I'd argue that it shouldn't
>> be there in the first place. It's fragile, and I don't see
>> why selected buffers would care specifically about polling
>> but not asking more generally "can it go true async"? For
>> reads you might want to also test FMODE_BUF_RASYNC.
> 
> That is indeed the second case that is hit, and I don't think we can
> easily get around that which is the reason for the hint.
> 
>> Also note that when called from recv we already know that
>> it's pollable, it might be much easier to pass it in as an
>> argument.
> 
> I did think about that, but I don't see a clean way to do it. We could
> potentially do it as an issue flag, but that seems kind of ugly to me.
> Open to suggestions!

I'd argue passing it as an argument is much much cleaner
and more robust design wise, those leaked abstractions are
always fragile and unreliable. And now there is an argument
that it's even faster because for recv you can just pass "true".
IOW, I'd prefer here potentially a slightly uglier but safer
code.

Surely it'd have been be great to move this "eject buffer"
thing out of the selection func and let the caller decide,
but I haven't stared at the code for long enough to say
anything concrete.

-- 
Pavel Begunkov