Re: timer code oops when calling mod_delayed_work

[Tejun Heo <tj@kernel.org>]

(cc'ing Lai)

Hello, Jeff.

On Thu, Oct 29, 2015 at 01:58:36PM -0400, Jeff Layton wrote:
> crash> p cache_cleaner
> cache_cleaner = $12 = {
>   work = {
>     data = {
>       counter = 0xfffffffe1

If I'm not mistaken, PENDING, flush color 14, OFFQ and POOL_NONE.

>     }, 
>     entry = {
>       next = 0xffffffffa03623c8 <cache_cleaner+8>, 
>       prev = 0xffffffffa03623c8 <cache_cleaner+8>

Empty entry.

>     }, 
>     func = 0xffffffffa03333c0 <cache_cleaner_func>
>   }, 
>   timer = {
>     entry = {
>       next = 0x0, 
>       pprev = 0xffff88085fd0eaf8
>     }, 
>     expires = 0x100021e99, 
>     function = 0xffffffff810b66a0 <delayed_work_timer_fn>, 
>     data = 0xffffffffa03623c0, 
>     flags = 0x200014, 
>     slack = 0xffffffff, 
>     start_pid = 0x0, 
>     start_site = 0x0, 
>     start_comm = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
>   }, 
>   wq = 0xffff88085f48fc00, 
>   cpu = 0x14
> }
> 
> So the PENDING bit is set (lowest bit in data.counter), and timer->entry.pprev
> pprev pointer is not NULL (so timer_pending is true). I also see that
> there are several nfsd threads running the shrinker at the same time.
> 
> There is one potential problem that I see, but I'd appreciate someone
> sanity checking me on this. Here is mod_delayed_work_on:
...
> ...and here is the beginning of try_to_grab_pending:
> 
> ------------------[snip]------------------------
>         /* try to steal the timer if it exists */
>         if (is_dwork) {
>                 struct delayed_work *dwork = to_delayed_work(work);
> 
>                 /*
>                  * dwork->timer is irqsafe.  If del_timer() fails, it's
>                  * guaranteed that the timer is not queued anywhere and not
>                  * running on the local CPU.
>                  */
>                 if (likely(del_timer(&dwork->timer)))
>                         return 1;
>         }
> 
>         /* try to claim PENDING the normal way */
>         if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work)))
>                 return 0;
> ------------------[snip]------------------------
> 
> 
> ...so if del_timer returns true, we'll return 1 from
> try_to_grab_pending without actually setting the
> WORK_STRUCT_PENDING_BIT, and then will end up calling
> __queue_delayed_work.
> 
> That seems wrong to me -- shouldn't we be ensuring that that bit is set
> when returning 1 from try_to_grab_pending to guard against concurrent
> queue_delayed_work_on calls?

But if try_to_grab_pending() succeeded at stealing dwork->timer, it's
known that the PENDING bit must already be set.  IOW, the bit is
stolen together with the timer.

Heh, this one is tricky.  Yeah, try_to_grab_pending() missing PENDING
would explain the failure but I can't see how it'd leak at the moment.

Thanks.

-- 
tejun