Re: [PATCH 2/3] work_on_cpu: Use our own workqueue.

[Dmitry Adamushko]

2009/2/4 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>:
> On Wed, 4 Feb 2009 22:35:19 +0100
> Ingo Molnar <mingo@xxxxxxx> wrote:
>
>>
>> * Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> > mm/pdflush.c:
>> >
>> >   wtf what the heck is all that stuff and who added it?  weird.
>> >
>> >   Leave it alone I guess.  Can admins manually move kernel threads to
>> >   other CPUs?
>>
>> they can - and there's even tools that do that (there's some -rt tools where
>> you can put kernel thread priorities into a config file).
>>
>
> Oh well, DontDoThatThen.
>
> I expect that the same argument applies to most of the set_cpus_allowed()
> callsites - they're run by root-only code.  Sure, root can (with
> careful timing) move root's own thread onto the wrong CPU in the middle
> of microcode loading.  In which case root gets to own both pieces.

Another issue is that those set_cpus_allowed() callsites may
effectivelly cancel the effect of sched_setaffinity() being run by an
administrator in parallel with a target process calling e.g.
cpufreq_get(0)

[ there are a couple of callsites in drivers/{video,pcmcia}, possibly
running in a process context for which sched_setaffinity() with mask
!= 'all_cpus_set' may be legitimate from admin's POV :-) ]

iow, not all use-cases are so obvious (like microcode) to fall into
the category of "didn't you know that this action could do some
unsynchronized cpu-mask-fiddling work behind your back". Not to say
that the existence of sych category is wrong, imho.

(regarding microcode)

> This code is just nuts.  What's the point in pinning itself to > a CPU for the act of loading the microcode into main
> memory?  It's only
> the loading of the microcode which should care about
> which CPU
> executes the code.  ie: apply_microcode().

Well, basically I tried to preserve the existing mechanisms/schemes as
much as possible when reworking this code and yes, I'm the person to
be blamed for this 'nuts' code.

> The code needs some laundering, switch to >schedule_work_on().

I had a patch doing exactly this but then there were other concerns
with the 'schedule_work_on()' approach
(e.g. http://linux.derkeiler.com/Mailing-Lists/Kernel/2008-08/msg02827.html)

then I had another idea (run it from start_secondary() or something
like this), but I never managed to look at this issue again
(and noone else seemed to care about really running "it as early as
possible" ;-)

In any case, it should be fixable one way or another.

> Ensure that the callback functions don't take >microcode_mutex.

yes, these actually look redundant in cpu-hotplug paths (other callers
call get/put_online_cpus() so this part should be ok).

Will fix. Thanks for your comments!


-- 
Best regards,
Dmitry Adamushko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/