lkml.org 
[lkml]   [2008]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: get_online_cpus() && workqueues
    On Sat, Apr 26, 2008 at 06:43:30PM +0400, Oleg Nesterov wrote:
    > Gautham, Srivatsa, seriously, can't we uglify cpu.c a little bit to solve
    > the problem. Please see the illustration patch below. It looks complicated,
    > but in fact it is quite trivial.
    >
    > In short: work_struct can't use get_online_cpus() due to deadlock with the
    > CPU_DEAD phase.
    >
    > Can't we add another nested lock which is dropped right after __cpu_die()?
    > (in fact I think it could be dropped after __stop_machine_run).
    >
    > The new read-lock is get_online_map() (just a random name for now). The only
    > difference wrt get_online_cpus() is that it doesn't protect against CPU_DEAD,
    > but most users of get_online_cpus() doesn't need this, they only need a
    > stable cpu_online_map and sometimes they need to be sure that some per-cpu
    > object (say, cpu_workqueue_struct->thread) can't be destroyed under this
    > lock.
    >
    > get_online_map() seem to fit for this, and can be used from work->func().
    > (actually, I think most users of use get_online_cpus() could use the new
    > helper instead, but this doen't matter).
    >
    > Heiko, what do you think? Is it suitable for arch_reinit_sched_domains()?
    >
    > Oleg.
    >
    > --- 25/kernel/cpu.c~HP_LOCK 2008-02-16 18:36:37.000000000 +0300
    > +++ 25/kernel/cpu.c 2008-04-26 18:14:25.000000000 +0400
    > @@ -25,7 +25,7 @@ static __cpuinitdata RAW_NOTIFIER_HEAD(c
    > */
    > static int cpu_hotplug_disabled;
    >
    > -static struct {
    > +static struct cpu_lock {
    > struct task_struct *active_writer;
    > struct mutex lock; /* Synchronizes accesses to refcount, */
    > /*
    > @@ -33,41 +33,65 @@ static struct {
    > * an ongoing cpu hotplug operation.
    > */
    > int refcount;
    > -} cpu_hotplug;
    > +} cpu_hotplug, online_map;
    > +
    > +static inline void __cpu_hotplug_init(struct cpu_lock *cpu_lock)
    > +{
    > + cpu_lock->active_writer = NULL;
    > + mutex_init(&cpu_lock->lock);
    > + cpu_lock->refcount = 0;
    > +}
    >
    > void __init cpu_hotplug_init(void)
    > {
    > - cpu_hotplug.active_writer = NULL;
    > - mutex_init(&cpu_hotplug.lock);
    > - cpu_hotplug.refcount = 0;
    > + __cpu_hotplug_init(&cpu_hotplug);
    > + __cpu_hotplug_init(&online_map);
    > }
    >
    > #ifdef CONFIG_HOTPLUG_CPU
    >
    > -void get_online_cpus(void)
    > +void cpu_read_lock(struct cpu_lock *cpu_lock)
    > {
    > might_sleep();
    > - if (cpu_hotplug.active_writer == current)
    > + if (cpu_lock->active_writer == current)
    > return;
    > - mutex_lock(&cpu_hotplug.lock);
    > - cpu_hotplug.refcount++;
    > - mutex_unlock(&cpu_hotplug.lock);
    > + mutex_lock(&cpu_lock->lock);
    > + cpu_lock->refcount++;
    > + mutex_unlock(&cpu_lock->lock);
    > +}
    >
    > +void get_online_cpus(void)
    > +{
    > + cpu_read_lock(&cpu_hotplug);
    > }
    > EXPORT_SYMBOL_GPL(get_online_cpus);
    >
    > -void put_online_cpus(void)
    > +void get_online_map(void)
    > {
    > - if (cpu_hotplug.active_writer == current)
    > + cpu_read_lock(&online_map);
    > +}
    > +
    > +void cpu_read_unlock(struct cpu_lock *cpu_lock)
    > +{
    > + if (cpu_lock->active_writer == current)
    > return;
    > - mutex_lock(&cpu_hotplug.lock);
    > - if (!--cpu_hotplug.refcount && unlikely(cpu_hotplug.active_writer))
    > - wake_up_process(cpu_hotplug.active_writer);
    > - mutex_unlock(&cpu_hotplug.lock);
    > + mutex_lock(&cpu_lock->lock);
    > + if (!--cpu_lock->refcount && unlikely(cpu_lock->active_writer))
    > + wake_up_process(cpu_lock->active_writer);
    > + mutex_unlock(&cpu_lock->lock);
    > +}
    >
    > +void put_online_cpus(void)
    > +{
    > + cpu_read_unlock(&cpu_hotplug);
    > }
    > EXPORT_SYMBOL_GPL(put_online_cpus);
    >
    > +void put_online_map(void)
    > +{
    > + cpu_read_unlock(&online_map);
    > +}
    > +
    > #endif /* CONFIG_HOTPLUG_CPU */
    >
    > /*
    > @@ -91,7 +115,7 @@ void cpu_maps_update_done(void)
    > * Note that during a cpu-hotplug operation, the new readers, if any,
    > * will be blocked by the cpu_hotplug.lock
    > *
    > - * Since cpu_hotplug_begin() is always called after invoking
    > + * Since cpu_write_lock() is always called after invoking
    > * cpu_maps_update_begin(), we can be sure that only one writer is active.
    > *
    > * Note that theoretically, there is a possibility of a livelock:
    > @@ -106,25 +130,26 @@ void cpu_maps_update_done(void)
    > * get_online_cpus() not an api which is called all that often.
    > *
    > */
    > -static void cpu_hotplug_begin(void)
    > +static void cpu_write_lock(struct cpu_lock *cpu_lock)
    > {
    > - cpu_hotplug.active_writer = current;
    > + cpu_lock->active_writer = current;
    >
    > for (;;) {
    > - mutex_lock(&cpu_hotplug.lock);
    > - if (likely(!cpu_hotplug.refcount))
    > + mutex_lock(&cpu_lock->lock);
    > + if (likely(!cpu_lock->refcount))
    > break;
    > __set_current_state(TASK_UNINTERRUPTIBLE);
    > - mutex_unlock(&cpu_hotplug.lock);
    > + mutex_unlock(&cpu_lock->lock);
    > schedule();
    > }
    > }
    >
    > -static void cpu_hotplug_done(void)
    > +static void cpu_write_unlock(struct cpu_lock *cpu_lock)
    > {
    > - cpu_hotplug.active_writer = NULL;
    > - mutex_unlock(&cpu_hotplug.lock);
    > + cpu_lock->active_writer = NULL;
    > + mutex_unlock(&cpu_lock->lock);
    > }
    > +
    > /* Need to know about CPUs going up/down? */
    > int __cpuinit register_cpu_notifier(struct notifier_block *nb)
    > {
    > @@ -207,7 +232,8 @@ static int _cpu_down(unsigned int cpu, i
    > if (!cpu_online(cpu))
    > return -EINVAL;
    >
    > - cpu_hotplug_begin();
    > + cpu_write_lock(&cpu_hotplug);
    > + cpu_write_lock(&online_map);

    IMO, we should acquire the cpu_write_lock(&online_map)
    just before __stop_machine_run() and release it once
    stop_machine_run() returns.

    IIUC, this lock protects only the cpu_online_map.

    Ditto in case of cpu_up() where we should acquire the lock
    just before calling __cpu_up() and release it immediately thereafter.


    > err = __raw_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE | mod,
    > hcpu, -1, &nr_calls);
    > if (err == NOTIFY_BAD) {
    > @@ -238,6 +264,7 @@ static int _cpu_down(unsigned int cpu, i
    > err = PTR_ERR(p);
    > goto out_allowed;
    > }
    > + err = -EAGAIN;
    > goto out_thread;
    > }
    >
    > @@ -247,6 +274,7 @@ static int _cpu_down(unsigned int cpu, i
    >
    > /* This actually kills the CPU. */
    > __cpu_die(cpu);
    > + cpu_write_unlock(&online_map);
    >
    > /* CPU is completely dead: tell everyone. Too late to complain. */
    > if (raw_notifier_call_chain(&cpu_chain, CPU_DEAD | mod,
    > @@ -260,7 +288,9 @@ out_thread:
    > out_allowed:
    > set_cpus_allowed(current, old_allowed);
    > out_release:
    > - cpu_hotplug_done();
    > + if (err)
    > + cpu_write_unlock(&online_map);
    > + cpu_write_unlock(&cpu_hotplug);
    > return err;
    > }
    >
    > @@ -289,7 +319,8 @@ static int __cpuinit _cpu_up(unsigned in
    > if (cpu_online(cpu) || !cpu_present(cpu))
    > return -EINVAL;
    >
    > - cpu_hotplug_begin();
    > + cpu_write_lock(&cpu_hotplug);
    > + cpu_write_lock(&online_map);
    > ret = __raw_notifier_call_chain(&cpu_chain, CPU_UP_PREPARE | mod, hcpu,
    > -1, &nr_calls);
    > if (ret == NOTIFY_BAD) {
    > @@ -313,7 +344,8 @@ out_notify:
    > if (ret != 0)
    > __raw_notifier_call_chain(&cpu_chain,
    > CPU_UP_CANCELED | mod, hcpu, nr_calls, NULL);
    > - cpu_hotplug_done();
    > + cpu_write_unlock(&online_map);
    > + cpu_write_unlock(&cpu_hotplug);
    >
    > return ret;
    > }

    --
    Thanks and Regards
    gautham


    \
     
     \ /
      Last update: 2008-04-28 14:05    [W:3.083 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site