Re: [GIT PULL] io_uring updates for 6.14-rc1

[Linus Torvalds <torvalds@linux-foundation.org>]

On Sun, 19 Jan 2025 at 07:05, Jens Axboe <axboe@kernel.dk> wrote:
>
> Note that this will throw a merge conflict, as there's a conflict
> between a fix that went into mainline after 6.13-rc4. The
> io_uring/register.c one is trivial, the io_uring/uring_cmd.c requires a
> small addition. Here's my resolution [..]

Ok, so while doing this merge, I absolutely *hate* your resolution in
both files.

The READ_ONCE/WRITE_ONCE changes during resize operations may not
actually matter, but the way you wrote things, it does multiple
"READ_ONCE()" operations. Which is kind of against the whole *point*.

So in io_uring/register.c, after the loop that copies the old ring contents with

        for (i = old_head; i < tail; i++) {

I changed the

        WRITE_ONCE(n.rings->sq.head, READ_ONCE(o.rings->sq.head));
        WRITE_ONCE(n.rings->sq.tail, READ_ONCE(o.rings->sq.tail));

to instead just *use* the original READ_ONCE() values, and thus do

        WRITE_ONCE(n.rings->sq.head, old_head);
        WRITE_ONCE(n.rings->sq.tail, tail);

instead (and same for the 'cq' head/tail logic)

Otherwise, what's the point of reading "once", when you then read again?

Now, presumably (and hopefully) this doesn't actually matter, and
nobody should even have access to the old ring when it gets resized,
but it really bothered me.

And it's also entirely possible that I have now screwed up royally,
and I actually messed up. Maybe I just misunderstood the code. But the
old code really looked nonsensical, and I felt I couldn't leave it
alone.

Now, the other conflict didn't look nonsensical, and I *did* leave it
alone, but I still do hate it even if I did it as you did. Because I
hate that pattern.

There are now three cases of 'use the init_once callback" for
io_uring_alloc_async_data(), and all of them just clear out a couple
of fields.

Is it really worth it?

Could we not get rid of that 'init_once' pattern completely, and
replace it with just always using 'kzalloc()' to clear the *whole*
allocation initially?

From what I can tell, all those things are fairly small structures.
Doing a simple 'memset()' is *cheaper* than calling an indirect
function pointer that then messes up the cache by setting just one or
two fields (and has to do a read-for-ownership in order to do so).

Are there cases where the allocations are so big that doing a
kmalloc() and then clearing one field (using an indirect function
pointer) really is worth it?

Anyway, I left that logic alone, because my hatred for it may run hot
and deep, but the pattern goes beyond just the conflict.

So please tell me why I'm wrong, and please take a look at the
WRITE_ONCE() changes I *did* do, to see if I might be confused there
too.

               Linus