* + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
@ 2008-07-11 18:40 akpm
2008-07-12 15:32 ` [PATCH] pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on-cleanup Oleg Nesterov
2008-07-12 16:21 ` + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree Oleg Nesterov
0 siblings, 2 replies; 11+ messages in thread
From: akpm @ 2008-07-11 18:40 UTC (permalink / raw)
To: mm-commits; +Cc: rui.zhang, harbour, oleg, pavel, rjw
The patch titled
pm: introduce new interfaces schedule_work_on() and queue_work_on()
has been added to the -mm tree. Its filename is
pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/SubmitChecklist when testing your code ***
See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this
The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
------------------------------------------------------
Subject: pm: introduce new interfaces schedule_work_on() and queue_work_on()
From: Zhang Rui <rui.zhang@intel.com>
This interface allows adding a job on a specific cpu.
Although a work struct on a cpu will be scheduled to other cpu if the cpu
dies, there is a recursion if a work task tries to offline the cpu it's
running on. we need to schedule the task to a specific cpu in this case.
http://bugzilla.kernel.org/show_bug.cgi?id=10897
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Rus <harbour@sfinx.od.ua>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Oleg Nesterov <oleg@tv-sign.ru>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
include/linux/workqueue.h | 3 ++
kernel/workqueue.c | 39 ++++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff -puN include/linux/workqueue.h~pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on include/linux/workqueue.h
--- a/include/linux/workqueue.h~pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on
+++ a/include/linux/workqueue.h
@@ -179,6 +179,8 @@ __create_workqueue_key(const char *name,
extern void destroy_workqueue(struct workqueue_struct *wq);
extern int queue_work(struct workqueue_struct *wq, struct work_struct *work);
+extern int queue_work_on(int cpu, struct workqueue_struct *wq,
+ struct work_struct *work);
extern int queue_delayed_work(struct workqueue_struct *wq,
struct delayed_work *work, unsigned long delay);
extern int queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
@@ -188,6 +190,7 @@ extern void flush_workqueue(struct workq
extern void flush_scheduled_work(void);
extern int schedule_work(struct work_struct *work);
+extern int schedule_work_on(int cpu, struct work_struct *work);
extern int schedule_delayed_work(struct delayed_work *work, unsigned long delay);
extern int schedule_delayed_work_on(int cpu, struct delayed_work *work,
unsigned long delay);
diff -puN kernel/workqueue.c~pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on kernel/workqueue.c
--- a/kernel/workqueue.c~pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on
+++ a/kernel/workqueue.c
@@ -175,6 +175,32 @@ int queue_work(struct workqueue_struct *
}
EXPORT_SYMBOL_GPL(queue_work);
+/**
+ * queue_work_on - queue work on specific cpu
+ * @cpu: CPU number to execute work on
+ * @wq: workqueue to use
+ * @work: work to queue
+ *
+ * Returns 0 if @work was already on a queue, non-zero otherwise.
+ *
+ * We queue the work to a specific CPU
+ */
+int
+queue_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work)
+{
+ int ret = 0;
+
+ if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
+ BUG_ON(!list_empty(&work->entry));
+ preempt_disable();
+ __queue_work(wq_per_cpu(wq, cpu), work);
+ preempt_enable();
+ ret = 1;
+ }
+ return ret;
+}
+EXPORT_SYMBOL_GPL(queue_work_on);
+
static void delayed_work_timer_fn(unsigned long __data)
{
struct delayed_work *dwork = (struct delayed_work *)__data;
@@ -553,6 +579,19 @@ int schedule_work(struct work_struct *wo
}
EXPORT_SYMBOL(schedule_work);
+/*
+ * schedule_work_on - put work task on a specific cpu
+ * @cpu: cpu to put the work task on
+ * @work: job to be done
+ *
+ * This puts a job on a specific cpu
+ */
+int schedule_work_on(int cpu, struct work_struct *work)
+{
+ return queue_work_on(cpu, keventd_wq, work);
+}
+EXPORT_SYMBOL(schedule_work_on);
+
/**
* schedule_delayed_work - put work task in global workqueue after delay
* @dwork: job to be done
_
Patches currently in -mm which might be from rui.zhang@intel.com are
linux-next.patch
pm-ahci-speed-up-resume.patch
pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch
pm-schedule-sysrq-poweroff-on-boot-cpu.patch
dcdbas-use-memory_read_from_buffer.patch
dell_rbu-use-memory_read_from_buffer.patch
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on-cleanup
2008-07-11 18:40 + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree akpm
@ 2008-07-12 15:32 ` Oleg Nesterov
2008-07-12 15:35 ` [PATCH] workqueues: queue_work() can use queue_work_on() Oleg Nesterov
2008-07-12 15:45 ` [PATCH] workqueues: schedule_on_each_cpu() can use schedule_work_on() Oleg Nesterov
2008-07-12 16:21 ` + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree Oleg Nesterov
1 sibling, 2 replies; 11+ messages in thread
From: Oleg Nesterov @ 2008-07-12 15:32 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, rui.zhang, harbour, pavel, rjw
On 07/11, Andrew Morton wrote:
>
> +queue_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work)
> +{
> + int ret = 0;
> +
> + if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
> + BUG_ON(!list_empty(&work->entry));
> + preempt_disable();
> + __queue_work(wq_per_cpu(wq, cpu), work);
> + preempt_enable();
The comment above __queue_work() is wrong, we don't need to disable
preemtion.
What it actually means is: the caller of __queue_work() must ensure
we can't race with CPU_DEAD, but preempt_disable() can't help for
queue_work_on(). CPU can die even before preempt_disable().
Remove preempt_disable() and update the comment.
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
--- 26-rc2/kernel/workqueue.c~WQ_1_QWON_CLEANUP 2008-07-12 19:04:48.000000000 +0400
+++ 26-rc2/kernel/workqueue.c 2008-07-12 19:11:39.000000000 +0400
@@ -137,7 +137,6 @@ static void insert_work(struct cpu_workq
wake_up(&cwq->more_work);
}
-/* Preempt must be disabled. */
static void __queue_work(struct cpu_workqueue_struct *cwq,
struct work_struct *work)
{
@@ -180,7 +179,8 @@ EXPORT_SYMBOL_GPL(queue_work);
*
* Returns 0 if @work was already on a queue, non-zero otherwise.
*
- * We queue the work to a specific CPU
+ * We queue the work to a specific CPU, the caller must ensure it
+ * can't go away.
*/
int
queue_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work)
@@ -189,9 +189,7 @@ queue_work_on(int cpu, struct workqueue_
if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
BUG_ON(!list_empty(&work->entry));
- preempt_disable();
__queue_work(wq_per_cpu(wq, cpu), work);
- preempt_enable();
ret = 1;
}
return ret;
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] workqueues: queue_work() can use queue_work_on()
2008-07-12 15:32 ` [PATCH] pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on-cleanup Oleg Nesterov
@ 2008-07-12 15:35 ` Oleg Nesterov
2008-07-12 15:45 ` [PATCH] workqueues: schedule_on_each_cpu() can use schedule_work_on() Oleg Nesterov
1 sibling, 0 replies; 11+ messages in thread
From: Oleg Nesterov @ 2008-07-12 15:35 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, rui.zhang, harbour, pavel, rjw
queue_work() can use queue_work_on() to avoid the code duplication.
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
--- 26-rc2/kernel/workqueue.c~WQ_2_QWON_FACTOR 2008-07-12 19:11:39.000000000 +0400
+++ 26-rc2/kernel/workqueue.c 2008-07-12 19:19:23.000000000 +0400
@@ -159,14 +159,11 @@ static void __queue_work(struct cpu_work
*/
int queue_work(struct workqueue_struct *wq, struct work_struct *work)
{
- int ret = 0;
+ int ret;
+
+ ret = queue_work_on(get_cpu(), wq, work);
+ put_cpu();
- if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
- BUG_ON(!list_empty(&work->entry));
- __queue_work(wq_per_cpu(wq, get_cpu()), work);
- put_cpu();
- ret = 1;
- }
return ret;
}
EXPORT_SYMBOL_GPL(queue_work);
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] workqueues: schedule_on_each_cpu() can use schedule_work_on()
2008-07-12 15:32 ` [PATCH] pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on-cleanup Oleg Nesterov
2008-07-12 15:35 ` [PATCH] workqueues: queue_work() can use queue_work_on() Oleg Nesterov
@ 2008-07-12 15:45 ` Oleg Nesterov
1 sibling, 0 replies; 11+ messages in thread
From: Oleg Nesterov @ 2008-07-12 15:45 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, rui.zhang, harbour, pavel, rjw
schedule_on_each_cpu() can use schedule_work_on() to avoid the code
duplication.
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
--- 26-rc2/kernel/workqueue.c~WQ_3_SOEC_QWON 2008-07-12 19:19:23.000000000 +0400
+++ 26-rc2/kernel/workqueue.c 2008-07-12 19:40:57.000000000 +0400
@@ -689,8 +689,7 @@ int schedule_on_each_cpu(work_func_t fun
struct work_struct *work = per_cpu_ptr(works, cpu);
INIT_WORK(work, func);
- set_bit(WORK_STRUCT_PENDING, work_data_bits(work));
- __queue_work(per_cpu_ptr(keventd_wq->cpu_wq, cpu), work);
+ schedule_work_on(cpu, work);
}
for_each_online_cpu(cpu)
flush_work(per_cpu_ptr(works, cpu));
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
2008-07-11 18:40 + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree akpm
2008-07-12 15:32 ` [PATCH] pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on-cleanup Oleg Nesterov
@ 2008-07-12 16:21 ` Oleg Nesterov
2008-07-22 16:19 ` Gautham R Shenoy
1 sibling, 1 reply; 11+ messages in thread
From: Oleg Nesterov @ 2008-07-12 16:21 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, rui.zhang, harbour, pavel, rjw, Gautham R Shenoy
(Gautham cc'ed)
On 07/11, Andrew Morton wrote:
>
> Subject: pm: introduce new interfaces schedule_work_on() and queue_work_on()
> From: Zhang Rui <rui.zhang@intel.com>
>
> This interface allows adding a job on a specific cpu.
>
> Although a work struct on a cpu will be scheduled to other cpu if the cpu
> dies, there is a recursion if a work task tries to offline the cpu it's
> running on. we need to schedule the task to a specific cpu in this case.
> http://bugzilla.kernel.org/show_bug.cgi?id=10897
So, this is used in http://bugzilla.kernel.org/attachment.cgi?id=16707
--- linux-2.6.orig/kernel/power/poweroff.c 2008-06-30 16:01:35.000000000 +0800
+++ linux-2.6/kernel/power/poweroff.c 2008-07-03 10:50:05.000000000 +0800
@@ -25,7 +25,8 @@
static void handle_poweroff(int key, struct tty_struct *tty)
{
- schedule_work(&poweroff_work);
+ /* run sysrq poweroff on boot cpu */
+ schedule_work_on(first_cpu(cpu_online_map), &poweroff_work);
}
static struct sysrq_key_op sysrq_poweroff_op = {
A couple of silly questions, I don't understand the low-level details.
This patch (and kernel_power_off() afaics) assumes that the boot cpu
can't be cpu_down()'ed. Is it true in general? For example, grep shows
that arch/s390/kernel/smp.c:topology_init()->smp_add_present_cpu()
sets ->hotpluggable = 1 for all present CPUs?
Another question. I can't understand why first_cpu(cpu_online_map) is
always the boot CPU on every arch. IOW, shouldn't boot_cpu_init() set
some "boot_cpu = smp_processor_id()" which should be use instead of
first_cpu(cpu_online_map) ?
Thanks,
Oleg.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
2008-07-12 16:21 ` + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree Oleg Nesterov
@ 2008-07-22 16:19 ` Gautham R Shenoy
2008-07-24 12:43 ` Oleg Nesterov
0 siblings, 1 reply; 11+ messages in thread
From: Gautham R Shenoy @ 2008-07-22 16:19 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: akpm, linux-kernel, rui.zhang, harbour, pavel, rjw
On Sat, Jul 12, 2008 at 08:21:49PM +0400, Oleg Nesterov wrote:
> (Gautham cc'ed)
>
Sorry for the delay... I'm a bit tied down to other things until aug
20th :(
> On 07/11, Andrew Morton wrote:
> >
> > Subject: pm: introduce new interfaces schedule_work_on() and queue_work_on()
> > From: Zhang Rui <rui.zhang@intel.com>
> >
> > This interface allows adding a job on a specific cpu.
> >
> > Although a work struct on a cpu will be scheduled to other cpu if the cpu
> > dies, there is a recursion if a work task tries to offline the cpu it's
> > running on. we need to schedule the task to a specific cpu in this case.
> > http://bugzilla.kernel.org/show_bug.cgi?id=10897
>
> So, this is used in http://bugzilla.kernel.org/attachment.cgi?id=16707
>
> --- linux-2.6.orig/kernel/power/poweroff.c 2008-06-30 16:01:35.000000000 +0800
> +++ linux-2.6/kernel/power/poweroff.c 2008-07-03 10:50:05.000000000 +0800
> @@ -25,7 +25,8 @@
>
> static void handle_poweroff(int key, struct tty_struct *tty)
> {
> - schedule_work(&poweroff_work);
> + /* run sysrq poweroff on boot cpu */
> + schedule_work_on(first_cpu(cpu_online_map), &poweroff_work);
> }
>
> static struct sysrq_key_op sysrq_poweroff_op = {
>
> A couple of silly questions, I don't understand the low-level details.
>
> This patch (and kernel_power_off() afaics) assumes that the boot cpu
> can't be cpu_down()'ed. Is it true in general? For example, grep shows
> that arch/s390/kernel/smp.c:topology_init()->smp_add_present_cpu()
> sets ->hotpluggable = 1 for all present CPUs?
I tried this on a Power system sometime back and I was able to
offline CPU0. What I am not sure however, is
if that was the boot-cpu.
On x86, I do remember reading somewhere why we cannot offline
CPU0.
/me searches.
Yes, in arch/x86/kernel/topology.c
int __ref arch_register_cpu(int num)
{
/*
* CPU0 cannot be offlined due to several
* restrictions and assumptions in kernel. This basically
* doesnt add a control file, one cannot attempt to offline
* BSP.
*
* Also certain PCI quirks require not to enable hotplug control
* for all CPU's.
*/
if (num)
per_cpu(cpu_devices, num).cpu.hotpluggable = 1;
return register_cpu(&per_cpu(cpu_devices, num).cpu, num);
}
>
> Another question. I can't understand why first_cpu(cpu_online_map) is
> always the boot CPU on every arch. IOW, shouldn't boot_cpu_init() set
> some "boot_cpu = smp_processor_id()" which should be use instead of
> first_cpu(cpu_online_map) ?
>
Not very sure about this one.
> Thanks,
>
> Oleg.
>
--
Thanks and Regards
gautham
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
2008-07-22 16:19 ` Gautham R Shenoy
@ 2008-07-24 12:43 ` Oleg Nesterov
2008-07-25 1:17 ` Zhang Rui
0 siblings, 1 reply; 11+ messages in thread
From: Oleg Nesterov @ 2008-07-24 12:43 UTC (permalink / raw)
To: Gautham R Shenoy; +Cc: akpm, linux-kernel, rui.zhang, harbour, pavel, rjw
On 07/22, Gautham R Shenoy wrote:
>
> On Sat, Jul 12, 2008 at 08:21:49PM +0400, Oleg Nesterov wrote:
> >
> > So, this is used in http://bugzilla.kernel.org/attachment.cgi?id=16707
> >
> > --- linux-2.6.orig/kernel/power/poweroff.c 2008-06-30 16:01:35.000000000 +0800
> > +++ linux-2.6/kernel/power/poweroff.c 2008-07-03 10:50:05.000000000 +0800
> > @@ -25,7 +25,8 @@
> >
> > static void handle_poweroff(int key, struct tty_struct *tty)
> > {
> > - schedule_work(&poweroff_work);
> > + /* run sysrq poweroff on boot cpu */
> > + schedule_work_on(first_cpu(cpu_online_map), &poweroff_work);
> > }
> >
> > static struct sysrq_key_op sysrq_poweroff_op = {
> >
> > A couple of silly questions, I don't understand the low-level details.
> >
> > This patch (and kernel_power_off() afaics) assumes that the boot cpu
> > can't be cpu_down()'ed. Is it true in general? For example, grep shows
> > that arch/s390/kernel/smp.c:topology_init()->smp_add_present_cpu()
> > sets ->hotpluggable = 1 for all present CPUs?
>
> I tried this on a Power system sometime back and I was able to
> offline CPU0.
This means that
pm-schedule-sysrq-poweroff-on-boot-cpu.patch
is not 100% right. It is still possible to hang/deadlock if we race
with cpu_down(first_cpu(cpu_online_map)).
The bug is mostly theoretical, but perhaps should be fixed anyway,
handle_poweroff() can use kthread_run().
Oleg.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
2008-07-24 12:43 ` Oleg Nesterov
@ 2008-07-25 1:17 ` Zhang Rui
2008-07-25 9:42 ` Oleg Nesterov
0 siblings, 1 reply; 11+ messages in thread
From: Zhang Rui @ 2008-07-25 1:17 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Gautham R Shenoy, akpm, linux-kernel, harbour, pavel, rjw
On Thu, 2008-07-24 at 20:43 +0800, Oleg Nesterov wrote:
> On 07/22, Gautham R Shenoy wrote:
> >
> > On Sat, Jul 12, 2008 at 08:21:49PM +0400, Oleg Nesterov wrote:
> > >
> > > So, this is used in
> http://bugzilla.kernel.org/attachment.cgi?id=16707
> > >
> > > --- linux-2.6.orig/kernel/power/poweroff.c 2008-06-30
> 16:01:35.000000000 +0800
> > > +++ linux-2.6/kernel/power/poweroff.c 2008-07-03
> 10:50:05.000000000 +0800
> > > @@ -25,7 +25,8 @@
> > >
> > > static void handle_poweroff(int key, struct tty_struct *tty)
> > > {
> > > - schedule_work(&poweroff_work);
> > > + /* run sysrq poweroff on boot cpu */
> > > + schedule_work_on(first_cpu(cpu_online_map),
> &poweroff_work);
> > > }
> > >
> > > static struct sysrq_key_op sysrq_poweroff_op = {
> > >
> > > A couple of silly questions, I don't understand the low-level
> details.
> > >
> > > This patch (and kernel_power_off() afaics) assumes that the boot
> cpu
> > > can't be cpu_down()'ed. Is it true in general? For example, grep
> shows
> > > that arch/s390/kernel/smp.c:topology_init()->smp_add_present_cpu()
> > > sets ->hotpluggable = 1 for all present CPUs?
> >
> > I tried this on a Power system sometime back and I was able to
> > offline CPU0.
>
> This means that
>
> pm-schedule-sysrq-poweroff-on-boot-cpu.patch
>
> is not 100% right. It is still possible to hang/deadlock if we race
> with cpu_down(first_cpu(cpu_online_map)).
Yes, you're right.
But then should we fix disable_nonboot_cpus as well?
int disable_nonboot_cpus(void)
{
first_cpu = first_cpu(cpu_online_map);
...
for_each_online_cpu(cpu) {
if (cpu == first_cpu)
continue;
error = _cpu_down(cpu, 1);
...
}
...
}
thanks,
rui
> The bug is mostly theoretical, but perhaps should be fixed anyway,
> handle_poweroff() can use kthread_run().
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
2008-07-25 1:17 ` Zhang Rui
@ 2008-07-25 9:42 ` Oleg Nesterov
2008-08-05 19:57 ` Pavel Machek
0 siblings, 1 reply; 11+ messages in thread
From: Oleg Nesterov @ 2008-07-25 9:42 UTC (permalink / raw)
To: Zhang Rui; +Cc: Gautham R Shenoy, akpm, linux-kernel, harbour, pavel, rjw
On 07/25, Zhang Rui wrote:
>
> On Thu, 2008-07-24 at 20:43 +0800, Oleg Nesterov wrote:
> >
> > This means that
> >
> > pm-schedule-sysrq-poweroff-on-boot-cpu.patch
> >
> > is not 100% right. It is still possible to hang/deadlock if we race
> > with cpu_down(first_cpu(cpu_online_map)).
>
> Yes, you're right.
> But then should we fix disable_nonboot_cpus as well?
>
> int disable_nonboot_cpus(void)
> {
> first_cpu = first_cpu(cpu_online_map);
> ...
>
> for_each_online_cpu(cpu) {
> if (cpu == first_cpu)
> continue;
> error = _cpu_down(cpu, 1);
> ...
> }
> ...
> }
Note that disable_nonboot_cpus() does first_cpu = first_cpu() under
cpu_maps_update_begin(), so we can't race with cpu-hotplug.
However, this afaics means that its name is wrong, and
printk("Disabling non-boot CPUs ...\n") is not right too.
What it does is disable_all_but_one_cpus().
And, it is not clear why disable_nonboot_cpus() assumes that
all but first_cpu(cpu_online_map) must have .hotpluggable == 1.
And, if one of the callers really need to preserve the boot CPU,
I don't understand how it is guaranteed it must be first_cpu().
Oleg.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
2008-07-25 9:42 ` Oleg Nesterov
@ 2008-08-05 19:57 ` Pavel Machek
2008-08-06 12:45 ` Oleg Nesterov
0 siblings, 1 reply; 11+ messages in thread
From: Pavel Machek @ 2008-08-05 19:57 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Zhang Rui, Gautham R Shenoy, akpm, linux-kernel, harbour, rjw
Hi!
> > > This means that
> > >
> > > pm-schedule-sysrq-poweroff-on-boot-cpu.patch
> > >
> > > is not 100% right. It is still possible to hang/deadlock if we race
> > > with cpu_down(first_cpu(cpu_online_map)).
> >
> > Yes, you're right.
> > But then should we fix disable_nonboot_cpus as well?
> >
> > int disable_nonboot_cpus(void)
> > {
> > first_cpu = first_cpu(cpu_online_map);
> > ...
> >
> > for_each_online_cpu(cpu) {
> > if (cpu == first_cpu)
> > continue;
> > error = _cpu_down(cpu, 1);
> > ...
> > }
> > ...
> > }
>
> Note that disable_nonboot_cpus() does first_cpu = first_cpu() under
> cpu_maps_update_begin(), so we can't race with cpu-hotplug.
>
> However, this afaics means that its name is wrong, and
> printk("Disabling non-boot CPUs ...\n") is not right too.
> What it does is disable_all_but_one_cpus().
I thought that first cpu is defined to be boot cpu?
> And, it is not clear why disable_nonboot_cpus() assumes that
> all but first_cpu(cpu_online_map) must have .hotpluggable == 1.
Where does it assume that?
It will fail if some CPUs can't be unplugged, and I'm afraid suspend
can't work in such case...
> And, if one of the callers really need to preserve the boot CPU,
> I don't understand how it is guaranteed it must be first_cpu().
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree
2008-08-05 19:57 ` Pavel Machek
@ 2008-08-06 12:45 ` Oleg Nesterov
0 siblings, 0 replies; 11+ messages in thread
From: Oleg Nesterov @ 2008-08-06 12:45 UTC (permalink / raw)
To: Pavel Machek
Cc: Zhang Rui, Gautham R Shenoy, akpm, linux-kernel, harbour, rjw
On 08/05, Pavel Machek wrote:
>
> > > > This means that
> > > >
> > > > pm-schedule-sysrq-poweroff-on-boot-cpu.patch
> > > >
> > > > is not 100% right. It is still possible to hang/deadlock if we race
> > > > with cpu_down(first_cpu(cpu_online_map)).
> > >
> > > Yes, you're right.
> > > But then should we fix disable_nonboot_cpus as well?
> > >
> > > int disable_nonboot_cpus(void)
> > > {
> > > first_cpu = first_cpu(cpu_online_map);
> > > ...
> > >
> > > for_each_online_cpu(cpu) {
> > > if (cpu == first_cpu)
> > > continue;
> > > error = _cpu_down(cpu, 1);
> > > ...
> > > }
> > > ...
> > > }
> >
> > Note that disable_nonboot_cpus() does first_cpu = first_cpu() under
> > cpu_maps_update_begin(), so we can't race with cpu-hotplug.
> >
> > However, this afaics means that its name is wrong, and
> > printk("Disabling non-boot CPUs ...\n") is not right too.
> > What it does is disable_all_but_one_cpus().
>
> I thought that first cpu is defined to be boot cpu?
I don't know, but I don't really understand this low-level code.
Is it documented? This is certainly true on x86, but I don't
understand why this must be true on every arch.
Let's see. start_kernel() does smp_setup_processor_id(). Is it
guaranteed that it chooses the lowest number from cpu_possible_map?
This helper is only defined for voyager, but anyway it is not clear
why start_kernel() must be always called on CPU 0. Otherwise,
the next cpu_up() (from smp_init() or later) can add another CPU
which becomes first_cpu(cpu_online_map).
But, from disable_nonboot_cpus's pov this doesn't matter. Even if
the first cpu must be boot cpu, it can be (in general) cpu_down()'ed.
In that case, when disable_nonboot_cpus() is called, first_cpu()
returns another value.
Once again, I don't claim this all is wrong.
> > And, it is not clear why disable_nonboot_cpus() assumes that
> > all but first_cpu(cpu_online_map) must have .hotpluggable == 1.
>
> Where does it assume that?
>
> It will fail if some CPUs can't be unplugged, and I'm afraid suspend
> can't work in such case...
Yes I see. But disable_nonboot_cpus() doesn't check .hotpluggable,
it just takes CPU down regardless of .hotpluggable, is it always OK?
Oleg.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2008-08-06 12:42 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-11 18:40 + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree akpm
2008-07-12 15:32 ` [PATCH] pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on-cleanup Oleg Nesterov
2008-07-12 15:35 ` [PATCH] workqueues: queue_work() can use queue_work_on() Oleg Nesterov
2008-07-12 15:45 ` [PATCH] workqueues: schedule_on_each_cpu() can use schedule_work_on() Oleg Nesterov
2008-07-12 16:21 ` + pm-introduce-new-interfaces-schedule_work_on-and-queue_work_on.patch added to -mm tree Oleg Nesterov
2008-07-22 16:19 ` Gautham R Shenoy
2008-07-24 12:43 ` Oleg Nesterov
2008-07-25 1:17 ` Zhang Rui
2008-07-25 9:42 ` Oleg Nesterov
2008-08-05 19:57 ` Pavel Machek
2008-08-06 12:45 ` Oleg Nesterov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.