On Sun, Oct 21, 2007 at 03:39:17PM +0400, Oleg Nesterov wrote:
> On 10/16, Gautham R Shenoy wrote:
> >
> > This patch converts the known per-subsystem cpu_hotplug mutexes to
> > get_online_cpus put_online_cpus.
> > It also eliminates the CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE hotplug
> > notification events.
>
> Personally, I like the changes in workqueue.c very much, a couple of
> minor nits below.
>
> > --- linux-2.6.23.orig/kernel/workqueue.c
> > +++ linux-2.6.23/kernel/workqueue.c
> > @@ -592,8 +592,6 @@ EXPORT_SYMBOL(schedule_delayed_work_on);
> > * Returns zero on success.
> > * Returns -ve errno on failure.
> > *
> > - * Appears to be racy against CPU hotplug.
> > - *
>
> see below,
>
> > * schedule_on_each_cpu() is very slow.
> > */
> > int schedule_on_each_cpu(work_func_t func)
> > @@ -605,7 +603,7 @@ int schedule_on_each_cpu(work_func_t fun
> > if (!works)
> > return -ENOMEM;
> >
> > - preempt_disable(); /* CPU hotplug */
> > + get_online_cpus(); /* CPU hotplug */
> > for_each_online_cpu(cpu) {
> > struct work_struct *work = per_cpu_ptr(works, cpu);
> >
> > @@ -613,7 +611,7 @@ int schedule_on_each_cpu(work_func_t fun
> > set_bit(WORK_STRUCT_PENDING, work_data_bits(work));
> > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu), work);
> > }
> > - preempt_enable();
> > + put_online_cpus();
> > flush_workqueue(keventd_wq);
>
> Still racy. To kill the race, please move flush_workqueue() up, before
> put_online_cpus().
>
> > @@ -809,6 +809,7 @@ void destroy_workqueue(struct workqueue_
> > struct cpu_workqueue_struct *cwq;
> > int cpu;
> >
> > + get_online_cpus();
> > mutex_lock(&workqueue_mutex);
> > list_del(&wq->list);
> > mutex_unlock(&workqueue_mutex);
> > @@ -817,6 +818,7 @@ void destroy_workqueue(struct workqueue_
> > cwq = per_cpu_ptr(wq->cpu_wq, cpu);
> > cleanup_workqueue_thread(cwq, cpu);
> > }
> > + put_online_cpus();
>
> Correct, but I'd suggest to do put_online_cpus() earlier, right after
> mutex_unlock(&workqueue_mutex).
>
> > @@ -830,22 +832,17 @@ static int __devinit workqueue_cpu_callb
> > unsigned int cpu = (unsigned long)hcpu;
> > struct cpu_workqueue_struct *cwq;
> > struct workqueue_struct *wq;
> > + int ret = NOTIFY_OK;
> >
> > action &= ~CPU_TASKS_FROZEN;
> >
> > switch (action) {
> > - case CPU_LOCK_ACQUIRE:
> > - mutex_lock(&workqueue_mutex);
> > - return NOTIFY_OK;
> > -
> > - case CPU_LOCK_RELEASE:
> > - mutex_unlock(&workqueue_mutex);
> > - return NOTIFY_OK;
> >
>
> please remove this emtpy line
>
> > case CPU_UP_PREPARE:
> > cpu_set(cpu, cpu_populated_map);
> > }
> >
> > + mutex_lock(&workqueue_mutex);
>
> We don't need workqueue_mutex here. With your patch workqueue_mutex protects
> list_head, nothing more. But all other callers (create/destroy) should take
> get_online_cpus() anyway. This means that we can convert workqueue_mutex to
> spinlock_t.
Thanks for the review!
Will code these changes up in the next version and post them
sometime soon.
>
> Oleg.
>
Thanks and Regards
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
