Messages in this thread | | | Date | Wed, 16 Jul 2008 23:55:30 +0200 | From | "Dmitry Adamushko" <> | Subject | Re: [PATCH] cpu hotplug, sched: Introduce cpu_active_map and redo sched domain managment (take 2) |
| |
2008/7/16 Max Krasnyansky <maxk@qualcomm.com>: > > > Dmitry Adamushko wrote: >> 2008/7/15 Max Krasnyansky <maxk@qualcomm.com>: >>> From: Max Krasnyanskiy <maxk@qualcomm.com> >>> >>> Addressed Ingo's comments and merged on top of latest Linus's tree. >> >> a few remarks: >> >> (1) in __migrate_task(), a test for !cpu_active(dest_cpu) should be >> done after double_rq_lock() [ or add the second one ] >> >> migration_thread() calls __migrate_task() with disabled interrupts (no >> rq-locks held), i.e. if we merely rely on rq-locks for >> synchronization, this can race with cpu_down(dest_cpu). >> >> [ assume, the test was done in __migration_task() and it's about to >> take double_lock()... and at this time, down_cpu(dest_cpu) starts and >> completes on another CPU ] >> >> note, we may still take the rq-lock for a "dead" cpu in this case and >> then only do a check (remark: in fact, not with stop_machine() in >> place _but_ I consider that we don't make any assumptions on its >> behavior); > Hmm, as you suggested I added synchronize_sched() after clearing the active > bit (see below). Is that not nought enough ? I mean you mentioned that > stop_machine syncs things up, I assume synchronize_sched does too.
Yes, sorry for the noise here.
* synchronize_sched - block until all CPUs have exited any non-preemptive * kernel code sequences
so "any non-preemptive" sections, not just the ones with run-queue locks being held.
>> (2) it's worth to take a look at the use of any_online_cpu(): >> >> many places are ok, because there will be an additional check against >> cpu_active_mask later on, but e.g. >> >> set_cpus_allowed_ptr() -> >> migrate_task(p, any_online_cpu(mask), ...) -> >> migrate_task(p, dest_cpu) >> >> doesn't seem to have any verifications wrt cpu_active_map. > How about we just introduce any_active_cpu() and replace all the usages of > any_online_cpu() in the scheduler ?
I think, at least for places related to task placement (like migrate_task(..., any_online_cpu()) it would make sense, consistency-wise.
> > Max >
-- Best regards, Dmitry Adamushko
| |