Re: [git pull] x86 fixes

Ingo Molnar
2009-01-26T19:59:32Z

* 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.

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.

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.

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.

I also agree that the commit should have included this description.

Ingo