lkml.org 
[lkml]   [2008]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] cpu hotplug, sched: Introduce cpu_active_map and redo sched domain managment (take 2)


    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.

    I guess testing inside the double_rq_lock() does not hurt anyway. We already
    have fail recovery path there. But are you sure it's needed given the explicit
    sync (in fact we have double sync now :), one with synchronize_sched() and
    then with the stop_machine)).

    > (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 ?

    > (3) I assume, we have some kind of explicit sched_sync() after
    > cpu_clear(cpu, cpu_active_mask) because:
    >
    > (a) not all places where task-migration may take place do take the
    > rq-lock for dest_cpu : e.g. try_to_wake_up() or even
    > sched_migrate_task() [ yes, there is a special (one might call
    > "subtle") assumption/restriction in this case ]
    >
    > that's why the fact that cpu_down() takes the rq-lock for
    > soon-to-be-offline cpu at some point can not be a "natural" sync.
    > point to guarantee that "stale" cpu_active_map is not used.
    >
    > (b) in fact, stop_machine() acts as a (very strong) sync. point,
    > sched-wise. But perhaps, we don't want to have this new easy-to-follow
    > approach to be built on top of assumptions on how something from
    > another sub-system behaves.
    Yep. As you suggested I've added synchronize_sched() and updated the comment
    that explains the deal with the stop machine.
    http://lkml.org/lkml/2008/7/15/736
    Peter, already ACKed it.

    Max


    \
     
     \ /
      Last update: 2008-07-16 22:31    [W:4.260 / U:1.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site