Re: [PATCH 3/6] io_uring: switch local task_work to a mpscq

On 6/11/26 9:20 PM, Caleb Sander Mateos wrote:
>> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
>> index 85e12b4884a5..9df5584ec3b1 100644
>> --- a/include/linux/io_uring_types.h
>> +++ b/include/linux/io_uring_types.h
>> @@ -351,6 +351,14 @@ struct io_ring_ctx {
>>                  */
>>                 atomic_t                cancel_seq;
>>
>> +               /*
>> +                * Consumer cursor for ->work_list, protected by ->uring_lock.
>> +                * Deliberately kept away from the producer side of the queue,
>> +                * as it's written for every popped entry, and the producer
>> +                * cacheline is contended enough as it is.
>> +                */
>> +               struct llist_node       *work_head;
> 
> Looks like this field has padding both before (next to atomic_t) and
> after (next to bool). Probably doesn't matter currently, as the outer
> struct is cache-aligned and has 16 bytes of padding at the end, but
> could save 8 bytes of padding by reordering next to an existing
> 8-byte-aligned field.

Indeed - I'll still move it, then we don't have to hunt holes later.

>> +       /*
>> +        * No one is waiting (IO_CQ_WAKE_INIT), or this cycle's wake up has
>> +        * already been issued (zero or negative, see below).
>> +        */
>>         nr_wait = atomic_read(&ctx->cq_wait_nr);
>> -       /* not enough or no one is waiting */
>> -       if (nr_tw < nr_wait)
>> +       if (nr_wait <= 0)
>>                 return;
>> -       /* the previous add has already woken it up */
>> -       if (nr_tw_prev >= nr_wait)
>> +       if (flags & IOU_F_TWQ_LAZY_WAKE) {
>> +               /*
>> +                * ->cq_wait_nr counts down the number of lazy adds, once it
>> +                * hits zero we're good to wake the waiter.
>> +                */
>> +               if (!atomic_dec_and_test(&ctx->cq_wait_nr))
>> +                       return;
> 
> It's possible that another task work wakes up the task before this one
> reaches the atomic_dec_and_test(), right? If the submitter task begins
> a new wait in between, this could decrement cq_wait_nr even though the
> queued task work has already been processed after the previous wakeup.
> I guess that's okay; in the worse case, the waiter will be woken
> prematurely.

That's correct, if the race is particularly unlucky, it could wake
early. I think that's fine, that's worth living with, and should be rare
enough to not really matter. It's not a lost wake, which would have been
a real problem.

I'll add a comment.

>> diff --git a/io_uring/wait.h b/io_uring/wait.h
>> index a4274b137f81..6d494297e1ce 100644
>> --- a/io_uring/wait.h
>> +++ b/io_uring/wait.h
>> @@ -5,12 +5,14 @@
>>  #include <linux/io_uring_types.h>
>>
>>  /*
>> - * No waiters. It's larger than any valid value of the tw counter
>> - * so that tests against ->cq_wait_nr would fail and skip wake_up().
>> + * ->cq_wait_nr is armed with the number of lazy task_work adds the waiter
>> + * still needs, and counted down by the add side, with the add reaching zero
>> + * issuing the (single) wake up for this wait cycle. Zero and below means no
>> + * wake up is to be issued: IO_CQ_WAKE_INIT when no task is waiting (also
>> + * what a forced wake up resets it to when claiming one), zero once the
>> + * countdown has fired.
>>   */
>>  #define IO_CQ_WAKE_INIT                (-1U)
> 
> Since cq_wait_nr is now used as a signed value, would it make sense to
> drop the U here?

Indeed, I'll fix that too.

-- 
Jens Axboe