On 1/20/25 9:38 PM, Linus Torvalds wrote:
> On Sun, 19 Jan 2025 at 07:05, Jens Axboe <axboe@xxxxxxxxx> 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.
Hah, noted!
>
> 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.
I do agree with you in that it's nonsensical to use READ_ONCE when it's
ready multiple times, even if it is for documentation purposes. Even
things like the old_head doesn't matter - yes userspace can screw itself
if it updates it after the initial read, but it won't cause any harm. At
the same time, if we've read the old_head, then we should just use that
going forward. So I think it all looks fine, thanks for cleaning that up
while merging.
> 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.
I'll take a look at this and see if we can't clean that up. The fast
path should be cached anyway.
> 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.
Looks good to me.
--
Jens Axboe
