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.
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?
