Re: [PATCH for-next 04/12] io_uring: reschedule retargeting at shutdown of ring

[Jens Axboe <axboe@kernel.dk>]

On 10/31/22 10:44 AM, Dylan Yudaken wrote:
> On Mon, 2022-10-31 at 10:02 -0600, Jens Axboe wrote:
>> On 10/31/22 7:41 AM, Dylan Yudaken wrote:
>>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>>> index 8d0d40713a63..40b37899e943 100644
>>> --- a/io_uring/rsrc.c
>>> +++ b/io_uring/rsrc.c
>>> @@ -248,12 +248,20 @@ static unsigned int
>>> io_rsrc_retarget_table(struct io_ring_ctx *ctx,
>>>         return refs;
>>>  }
>>>  
>>> -static void io_rsrc_retarget_schedule(struct io_ring_ctx *ctx)
>>> +static void io_rsrc_retarget_schedule(struct io_ring_ctx *ctx,
>>> bool delay)
>>>         __must_hold(&ctx->uring_lock)
>>>  {
>>> -       percpu_ref_get(&ctx->refs);
>>> -       mod_delayed_work(system_wq, &ctx->rsrc_retarget_work, 60 *
>>> HZ);
>>> -       ctx->rsrc_retarget_scheduled = true;
>>> +       unsigned long del;
>>> +
>>> +       if (delay)
>>> +               del = 60 * HZ;
>>> +       else
>>> +               del = 0;
>>> +
>>> +       if (likely(!mod_delayed_work(system_wq, &ctx-
>>>> rsrc_retarget_work, del))) {
>>> +               percpu_ref_get(&ctx->refs);
>>> +               ctx->rsrc_retarget_scheduled = true;
>>> +       }
>>>  }
>>
>> What happens for del == 0 and the work running ala:
>>
>> CPU 0                           CPU 1
>> mod_delayed_work(.., 0);
>>                                 delayed_work runs
>>                                         put ctx
>> percpu_ref_get(ctx)
> 
> The work takes the lock before put(ctx), and CPU 0 only releases the
> lock after calling get(ctx) so it should be ok.

But io_ring_ctx_ref_free() would've run at that point? Maybe I'm
missing something...

In any case, would be saner to always grab that ref first. Or at
least have a proper comment as to why it's safe, because it looks
iffy.

>> Also I think that likely() needs to get dropped.
>>
> 
> It's not a big thing, but the only time it will be enqueued is on ring
> shutdown if there is an outstanding enqueue. Other times it will not
> get double enqueued as it is protected by the _scheduled bool (this is
> important or else it will continually push back by 1 period and maybe
> never run)

We've already called into this function, don't think it's worth a
likely. Same for most of the others added in this series, imho they
only really make sense for a very hot path where that branch is
inline.

-- 
Jens Axboe