Subject: [RFC][PATCH 2/4] Revert Changes to kernel/workqueue.c


--
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!"

Attachments:
(No filename) (165.00 B)
 2of4.patch (3.84 kB)
 Download all attachments

2006-08-24 10:51:14

by Heiko Carstens

[permalink] [raw]
Subject: Re: [RFC][PATCH 2/4] Revert Changes to kernel/workqueue.c

> @@ -510,13 +515,11 @@ int schedule_on_each_cpu(void (*func)(vo
> if (!works)
> return -ENOMEM;
>
> - mutex_lock(&workqueue_mutex);
> for_each_online_cpu(cpu) {
> INIT_WORK(per_cpu_ptr(works, cpu), func, info);
> __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu),
> per_cpu_ptr(works, cpu));
> }
> - mutex_unlock(&workqueue_mutex);
> flush_workqueue(keventd_wq);
> free_percpu(works);
> return 0;

Removing this lock without adding a lock/unlock_cpu_hotplug seems wrong,
since this function is walking the cpu_online_map.

Subject: Re: [RFC][PATCH 2/4] Revert Changes to kernel/workqueue.c

On Thu, Aug 24, 2006 at 12:51:00PM +0200, Heiko Carstens wrote:
> > @@ -510,13 +515,11 @@ int schedule_on_each_cpu(void (*func)(vo
> > if (!works)
> > return -ENOMEM;
> >
> > - mutex_lock(&workqueue_mutex);
> > for_each_online_cpu(cpu) {
> > INIT_WORK(per_cpu_ptr(works, cpu), func, info);
> > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu),
> > per_cpu_ptr(works, cpu));
> > }
> > - mutex_unlock(&workqueue_mutex);
> > flush_workqueue(keventd_wq);
> > free_percpu(works);
> > return 0;
>
> Removing this lock without adding a lock/unlock_cpu_hotplug seems wrong,
> since this function is walking the cpu_online_map.
I had thought of it. But later decided to retain the same code as 2.6.18-rc4,
where there was no lock_cpu_hotplug surrounding for_each_online_cpu.

Furthermore, it did not create any problems with the test run.
So I thought *may-be* we don't need it.
But looks like I need to investigate further.
Thanks for pointing it out.

Regards
ego.
--
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!"

2006-08-24 11:02:59

by Dipankar Sarma

[permalink] [raw]
Subject: Re: [RFC][PATCH 2/4] Revert Changes to kernel/workqueue.c

On Thu, Aug 24, 2006 at 12:51:00PM +0200, Heiko Carstens wrote:
> > @@ -510,13 +515,11 @@ int schedule_on_each_cpu(void (*func)(vo
> > if (!works)
> > return -ENOMEM;
> >
> > - mutex_lock(&workqueue_mutex);
> > for_each_online_cpu(cpu) {
> > INIT_WORK(per_cpu_ptr(works, cpu), func, info);
> > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu),
> > per_cpu_ptr(works, cpu));
> > }
> > - mutex_unlock(&workqueue_mutex);
> > flush_workqueue(keventd_wq);
> > free_percpu(works);
> > return 0;
>
> Removing this lock without adding a lock/unlock_cpu_hotplug seems wrong,
> since this function is walking the cpu_online_map.

As long as you disable preemption and don't block the critical
section should be safe from cpu hotplug. There is no need to
lock/unlock cpu hotplug.

Thanks
Dipankar

2006-08-24 11:14:05

by Heiko Carstens

[permalink] [raw]
Subject: Re: [RFC][PATCH 2/4] Revert Changes to kernel/workqueue.c

On Thu, Aug 24, 2006 at 04:33:06PM +0530, Dipankar Sarma wrote:
> On Thu, Aug 24, 2006 at 12:51:00PM +0200, Heiko Carstens wrote:
> > > @@ -510,13 +515,11 @@ int schedule_on_each_cpu(void (*func)(vo
> > > if (!works)
> > > return -ENOMEM;
> > >
> > > - mutex_lock(&workqueue_mutex);
> > > for_each_online_cpu(cpu) {
> > > INIT_WORK(per_cpu_ptr(works, cpu), func, info);
> > > __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu),
> > > per_cpu_ptr(works, cpu));
> > > }
> > > - mutex_unlock(&workqueue_mutex);
> > > flush_workqueue(keventd_wq);
> > > free_percpu(works);
> > > return 0;
> >
> > Removing this lock without adding a lock/unlock_cpu_hotplug seems wrong,
> > since this function is walking the cpu_online_map.
>
> As long as you disable preemption and don't block the critical
> section should be safe from cpu hotplug. There is no need to
> lock/unlock cpu hotplug.

What disables preemption here?
Ah.. you probably meant preempt_disable/enable instead of lock/unlock cpu
hotplug would be sufficient. True.