Re: [Query]: delayed wq not killed completely with cancel_delayed_work_sync()

Re: [Query]: delayed wq not killed completely with cancel_delayed_work_sync()

[Viresh Kumar]

On 09-06-15, 16:43, Viresh Kumar wrote:
> HI Tejun,
>
> We had few races in cpufreq core for some time now and we
> are looking to fix them.
>
> Briefly, we run a delayed_work on each cpu at a fixed interval
> (sampling rate) and when that expires that take a look at system
> load and adjust frequency accordingly. We also requeue the
> delayed-works from these handlers.
>
> The problem we are facing is NULL pointer dereference from
> work handler..
>
> Before we set the pointers to NULL and free resource (for which
> we are seeing the crashes), we cancel the delayed works with
> cancel_delayed_work_sync(&dwork);
>
> We expect the work to not fire at all once this returns, but it
> looks like the work handler does get called..
>
> The mainline version of cpufreq_governor.c is a bit older than
> what we have, but I just wanted the clarification on the routine
> itself.
>
> Thanks for reading this :)

How can I send an HTML mail, should be jailed for that :)

-- 
viresh

Re: [Query]: delayed wq not killed completely with cancel_delayed_work_sync()

[Viresh Kumar]

On 09-06-15, 16:48, Viresh Kumar wrote:
> On 09-06-15, 16:43, Viresh Kumar wrote:
> > HI Tejun,
> >
> > We had few races in cpufreq core for some time now and we
> > are looking to fix them.
> >
> > Briefly, we run a delayed_work on each cpu at a fixed interval
> > (sampling rate) and when that expires that take a look at system
> > load and adjust frequency accordingly. We also requeue the
> > delayed-works from these handlers.
> >
> > The problem we are facing is NULL pointer dereference from
> > work handler..
> >
> > Before we set the pointers to NULL and free resource (for which
> > we are seeing the crashes), we cancel the delayed works with
> > cancel_delayed_work_sync(&dwork);
> >
> > We expect the work to not fire at all once this returns, but it
> > looks like the work handler does get called..
> >
> > The mainline version of cpufreq_governor.c is a bit older than
> > what we have, but I just wanted the clarification on the routine
> > itself.
> >
> > Thanks for reading this :)

And another query:

Do we have support for this kind of scenarios in wq framework ?

- Enqueue a single delayed work for a group of CPUs (and should fire
  on any one of them). We are doing this per-cpu today in cpufreq.
- It has to be a deffered one, so that if none of the CPUs from that
  group are online, we don't fire it.
- As the per-cpu workqueue thing is unnecessary burden on CPUs.

Thanks.

-- 
viresh

Re: [Query]: delayed wq not killed completely with cancel_delayed_work_sync()

[Tejun Heo]

Hey, Viresh.

On Tue, Jun 09, 2015 at 04:56:27PM +0530, Viresh Kumar wrote:
> On 09-06-15, 16:48, Viresh Kumar wrote:
> > On 09-06-15, 16:43, Viresh Kumar wrote:
> > > HI Tejun,
> > >
> > > We had few races in cpufreq core for some time now and we
> > > are looking to fix them.
> > >
> > > Briefly, we run a delayed_work on each cpu at a fixed interval
> > > (sampling rate) and when that expires that take a look at system
> > > load and adjust frequency accordingly. We also requeue the
> > > delayed-works from these handlers.
> > >
> > > The problem we are facing is NULL pointer dereference from
> > > work handler..
> > >
> > > Before we set the pointers to NULL and free resource (for which
> > > we are seeing the crashes), we cancel the delayed works with
> > > cancel_delayed_work_sync(&dwork);
> > >
> > > We expect the work to not fire at all once this returns, but it
> > > looks like the work handler does get called..

cancel_delayed_work_sync() can reliably shoot down a self-requeueing
delayed work item but it doesn't do anything about queueings which
aren't from the work item itself, so, to shutdown a recurring work
item, you'd need to first block external queueing and then do
cancel_delayed_work_sync().  If you're already doing this but still
seeing the work item executing afterwards, it's a workqueue bug but
the code in that area has been stable for years, so I'd be surprised
if there's a bug like that but you never know.

This does get tricky and I've been thinking about adding something
like kill_delayed_work() which cancels and disables the work item till
it gets reinitialized.  Hmmm...

> And another query:
> 
> Do we have support for this kind of scenarios in wq framework ?
> 
> - Enqueue a single delayed work for a group of CPUs (and should fire
>   on any one of them). We are doing this per-cpu today in cpufreq.
> - It has to be a deffered one, so that if none of the CPUs from that
>   group are online, we don't fire it.
> - As the per-cpu workqueue thing is unnecessary burden on CPUs.

I'm not sure I'm following but shouldn't you be able to do the above
from cpu hotplug callbacks?  Or are you asking whether wq already has
something which would help implementing the above?

Thanks.

-- 
tejun

Re: [Query]: delayed wq not killed completely with cancel_delayed_work_sync()

[Viresh Kumar]

Hi Tejun,

Thanks for your response :)

On 10-06-15, 14:03, Tejun Heo wrote:
> cancel_delayed_work_sync() can reliably shoot down a self-requeueing
> delayed work item but it doesn't do anything about queueings which
> aren't from the work item itself, so, to shutdown a recurring work
> item, you'd need to first block external queueing and then do
> cancel_delayed_work_sync().

Fair enough.

> If you're already doing this but still

Fingers crossed :)

> seeing the work item executing afterwards, it's a workqueue bug but
> the code in that area has been stable for years, so I'd be surprised
> if there's a bug like that but you never know.

I really believe/hope I am doing this properly. But will crosscheck
that again.

> This does get tricky and I've been thinking about adding something
> like kill_delayed_work() which cancels and disables the work item till
> it gets reinitialized.  Hmmm...

I think its a good idea to get rid of such races. If you have
something in mind and can code it quickly enough, I would be happy to
test it for you. That will also help in my use case.

> > And another query:
> > 
> > Do we have support for this kind of scenarios in wq framework ?
> > 
> > - Enqueue a single delayed work for a group of CPUs (and should fire
> >   on any one of them). We are doing this per-cpu today in cpufreq.
> > - It has to be a deffered one, so that if none of the CPUs from that
> >   group are online, we don't fire it.

Urg, s/online/not-idle. IOW, the work shouldn't wake up CPUs from idle
state.

> > - As the per-cpu workqueue thing is unnecessary burden on CPUs.
> 
> I'm not sure I'm following

Above correction might make it better :)

> but shouldn't you be able to do the above
> from cpu hotplug callbacks?

Sorry it wasn't about online CPUs. My fault.

> Or are you asking whether wq already has
> something which would help implementing the above?

Okay, lemme explain a bit and then you can tell me what to do.

A group of CPUs which switch their DVFS (Dynamic voltage/frequency
scaling) state together (or which share their clock rails) are
considered specially in cpufreq. As changing frequency for any one of
them will affect all others.

Today's governors (of course badly designed, and people are working on
getting scheduler involved) run background work at regular intervals
to find the per-cpu load for this group of CPUs. Any cpu can run the
algorithm for the entire group. Earlier we were running this
background work on only one CPU, but because its a deffered work it
was missing cycles if that CPU was idle. And so we ended up adding the
work per-cpu to fix that. We do check on the per-cpu handler if any
other CPU had run the algo recently and in that case we return early
from the handler.

What I was thinking was to get some kind of support for these requests
from the wq core. So that we can ask the workqueue core to run a
work-handler on any non-idle CPU from a group of CPUs.

Hope I made it more clear this time around.

-- 
viresh

Re: [Query]: delayed wq not killed completely with cancel_delayed_work_sync()

[Tejun Heo]

Yo,

On Wed, Jun 10, 2015 at 11:50:19AM +0530, Viresh Kumar wrote:
...
> > This does get tricky and I've been thinking about adding something
> > like kill_delayed_work() which cancels and disables the work item till
> > it gets reinitialized.  Hmmm...
> 
> I think its a good idea to get rid of such races. If you have
> something in mind and can code it quickly enough, I would be happy to
> test it for you. That will also help in my use case.

It's not a race per-se.  It's just that cancel[_delayed]_work_sync()
doesn't disable the work item after it got cancelled and the work item
can be reused afterwards by queueing it again.  If you don't shut down
somebody queueing it again (excluding the work itself), the work item
is being simply being reactivated after being cancelled.

This fits some use cases and even for full shut down cases, plugging
the external queueing source is often necessary no matter what, so I'm
a bit torn about introuding another cancel function.  Regardless,
let's first debug this one properly.

> > > And another query:
> > > 
> > > Do we have support for this kind of scenarios in wq framework ?
> > > 
> > > - Enqueue a single delayed work for a group of CPUs (and should fire
> > >   on any one of them). We are doing this per-cpu today in cpufreq.
> > > - It has to be a deffered one, so that if none of the CPUs from that
> > >   group are online, we don't fire it.
> 
> Urg, s/online/not-idle. IOW, the work shouldn't wake up CPUs from idle
> state.

I see.

> > > - As the per-cpu workqueue thing is unnecessary burden on CPUs.
> > 
> > I'm not sure I'm following
> 
> Above correction might make it better :)
> 
> > but shouldn't you be able to do the above
> > from cpu hotplug callbacks?
> 
> Sorry it wasn't about online CPUs. My fault.
> 
> > Or are you asking whether wq already has
> > something which would help implementing the above?
> 
> Okay, lemme explain a bit and then you can tell me what to do.
> 
> A group of CPUs which switch their DVFS (Dynamic voltage/frequency
> scaling) state together (or which share their clock rails) are
> considered specially in cpufreq. As changing frequency for any one of
> them will affect all others.
> 
> Today's governors (of course badly designed, and people are working on
> getting scheduler involved) run background work at regular intervals
> to find the per-cpu load for this group of CPUs. Any cpu can run the
> algorithm for the entire group. Earlier we were running this
> background work on only one CPU, but because its a deffered work it
> was missing cycles if that CPU was idle. And so we ended up adding the
> work per-cpu to fix that. We do check on the per-cpu handler if any
> other CPU had run the algo recently and in that case we return early
> from the handler.
> 
> What I was thinking was to get some kind of support for these requests
> from the wq core. So that we can ask the workqueue core to run a
> work-handler on any non-idle CPU from a group of CPUs.
> 
> Hope I made it more clear this time around.

Hmmm.... that's pretty specific.  The deferring is implemented from
the timer side, so as long as timer doesn't provide a mechanism to do
collective deferring (ie. deferring across multiple cpus), I don't
think it makes sense for wq to try to implement that.  :(

Thanks.

-- 
tejun

Re: [Query]: delayed wq not killed completely with cancel_delayed_work_sync()

[Viresh Kumar]

On 10-06-15, 16:07, Tejun Heo wrote:
> It's not a race per-se.  It's just that cancel[_delayed]_work_sync()
> doesn't disable the work item after it got cancelled and the work item
> can be reused afterwards by queueing it again.  If you don't shut down
> somebody queueing it again (excluding the work itself), the work item
> is being simply being reactivated after being cancelled.

Right, it might be wrong in my case to use any such routines. As the
call sites need to fix this problem, a race or whatever.

> This fits some use cases and even for full shut down cases, plugging
> the external queueing source is often necessary no matter what, so I'm
> a bit torn about introuding another cancel function.  Regardless,
> let's first debug this one properly.

Got it.

> Hmmm.... that's pretty specific.  The deferring is implemented from
> the timer side, so as long as timer doesn't provide a mechanism to do
> collective deferring (ie. deferring across multiple cpus), I don't
> think it makes sense for wq to try to implement that.  :(

Fair enough. And it would be difficult to have something like this in
timers AFAICT. With timers, we choose the target CPU when the timer is
enqueued and so a single timer for a group of CPUs wouldn't work.

What I can do right away, is stop using per-cpu delayed work and use
per-cpu timers instead. And keep a single work which can be queued
from any of these CPUs. That will avoid queuing per-cpu works (might
be less racy). Might be worth giving a try.

Anyway, thanks for patience :)

-- 
viresh