Re: [git pull] x86 fixes

Andrew Morton
2009-01-26T20:15:36Z

On Mon, 26 Jan 2009 20:59:10 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > On Mon, 26 Jan 2009 20:20:16 +0100
> > Ingo Molnar <mingo@elte.hu> wrote:
> >
> > >
> > > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > > On Mon, 26 Jan 2009 18:17:23 +0100
> > > > Ingo Molnar <mingo@elte.hu> wrote:
> > > >
> > > > > Rusty Russell (2):
> > > > > ...
> > > > > work_on_cpu: Use our own workqueue.
> > > >
> > > > wtf?
> > >
> > > an x86 fix depends on it:
> > >
> > > 7285908: cpufreq: use work_on_cpu in acpi-cpufreq.c for drv_read and drv_write
> > >
> >
> > Right. And these are currently under active (albeit rather slow)
> > discussion.
> >
> > The changelogs suck, nobody can be assed actually telling us what the
> > bug is and the patches just casually toss yet another gaggle of kernel
> > threads into there.
>
> One problem is that for example do_dbs_timer() [used both in the
> cpufreq_ondemand and cpufreq_conservative cpufreq drivers] gets queued
> into the generic kevent workqueue via schedule_work(). But work_on_cpu()
> needs to serialize on the worklet - i.e. it needs to do a flush_work() -
> and does this with the cpufreq lock held.

We've discovered many times that doing a workqueue flush with a lock
held is a bad idea. If any callback takes that lock, or takes some
other lock which someone else takes while holding the original lock,
etc, we deadlock.

> So we have a 'worklet inversion' bug here - and this got reported as a
> hard to debug boot hang on some systems.
> The root cause is that kevents is not that do_dbs_timer() uses
> schedule_work() - the root cause is that kevents workqueue is a too
> generic workqueue that is the union of all casual workqueue users in the
> kernel. That is fine (and it is its purpose) but it should not be used for
> core kernel facilities such as work_on_cpu() - precisely because doing so
> would limit that facility's genericity.

Yes, that's a point.

> This bug was always there but dormant until work_on_cpu() was used from a
> deep enough codepath.
>
> So the solution here is to isolate work_on_cpu()s mechanisms from the
> 'misc' workqueue that schedule_work() deals with - this is what Rusty's
> patch does.

But it's still deadlockable, isn't it? If you do a work_on_cpu() while
holding a lock, and the callback function takes that lock, deadlock?
If so, things didn't get much better?

> Your observation about there being too many workqueue threads is correct
> but this commit is IMO a valid use of the workqueue facility. This
> workqueue it only gets created on CONFIG_SMP so there cost is about ~10K
> RAM per CPU.

mm.. This is why I'd like to expend a little more effort trying to
avoid having to make this change.