Re: [PATCH 3/8] Use process freezer for cpu-hotplug
Hi!
> > > - raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE, hcpu);
> > > + if (freeze_processes(FE_HOTPLUG_CPU)) {
> > > + thaw_processes(FE_HOTPLUG_CPU);
> > > + return -EBUSY;
> > > + }
> > > +
> >
> > If I'm understanding correctly, this will cause
> >
> > # echo 0 > /sys/devices/system/cpu/cpuX/online
> >
> > to sometimes fail, and userspace is expected to try again? This will
> > break existing applications.
> >
> > Perhaps drivers/base/cpu.c:store_online should retry as long as
> > cpu_up/down return -EBUSY. That would avoid a userspace-visible
> > interface change.
>
> yeah. I'd even suggest a freeze_processes_nofail() API instead, that
> does this internally, without burdening the callsites. (and once the
> freezer becomes complete then freeze_processes_nofail() ==
> freeze_processes())
Not sure if we _can_ do freeze_processes_nofail(). If something is
wrong (process in D state forever because of driver bug?), it looks
better to return error to userspace than looping forever.
You may want to pass higher timeout than 20sec. But if you can't
freeze everything in 1hour, it is unlikely to ever succeed.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
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/
Re: [PATCH 3/8] Use process freezer for cpu-hotplug
Hi.
On Fri, 2007-04-06 at 12:47 -0500, Nathan Lynch wrote:
> Ingo Molnar wrote:
> >
> > * Nathan Lynch <[EMAIL PROTECTED]> wrote:
> >
> > > > - raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE, hcpu);
> > > > + if (freeze_processes(FE_HOTPLUG_CPU)) {
> > > > + thaw_processes(FE_HOTPLUG_CPU);
> > > > + return -EBUSY;
> > > > + }
> > > > +
> > >
> > > If I'm understanding correctly, this will cause
> > >
> > > # echo 0 > /sys/devices/system/cpu/cpuX/online
> > >
> > > to sometimes fail, and userspace is expected to try again? This will
> > > break existing applications.
> > >
> > > Perhaps drivers/base/cpu.c:store_online should retry as long as
> > > cpu_up/down return -EBUSY. That would avoid a userspace-visible
> > > interface change.
> >
> > yeah. I'd even suggest a freeze_processes_nofail() API instead, that
> > does this internally, without burdening the callsites. (and once the
> > freezer becomes complete then freeze_processes_nofail() ==
> > freeze_processes())
>
> Yeah, I just realized that an implementation of my proposal would busy
> loop in the kernel forever if a silly admin tried to offline the last
> cpu (we're already using -EBUSY for that case), so
> freeze_processes_nofail is a better idea :-)
If there's only one online cpu, shouldn't it return -EINVAL?
Regards,
Nigel
-
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/
Re: [PATCH 3/8] Use process freezer for cpu-hotplug
Ingo Molnar wrote:
>
> * Nathan Lynch <[EMAIL PROTECTED]> wrote:
>
> > > - raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE, hcpu);
> > > + if (freeze_processes(FE_HOTPLUG_CPU)) {
> > > + thaw_processes(FE_HOTPLUG_CPU);
> > > + return -EBUSY;
> > > + }
> > > +
> >
> > If I'm understanding correctly, this will cause
> >
> > # echo 0 > /sys/devices/system/cpu/cpuX/online
> >
> > to sometimes fail, and userspace is expected to try again? This will
> > break existing applications.
> >
> > Perhaps drivers/base/cpu.c:store_online should retry as long as
> > cpu_up/down return -EBUSY. That would avoid a userspace-visible
> > interface change.
>
> yeah. I'd even suggest a freeze_processes_nofail() API instead, that
> does this internally, without burdening the callsites. (and once the
> freezer becomes complete then freeze_processes_nofail() ==
> freeze_processes())
Yeah, I just realized that an implementation of my proposal would busy
loop in the kernel forever if a silly admin tried to offline the last
cpu (we're already using -EBUSY for that case), so
freeze_processes_nofail is a better idea :-)
-
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/
Re: [PATCH 3/8] Use process freezer for cpu-hotplug
* Nathan Lynch <[EMAIL PROTECTED]> wrote:
> > - raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE, hcpu);
> > + if (freeze_processes(FE_HOTPLUG_CPU)) {
> > + thaw_processes(FE_HOTPLUG_CPU);
> > + return -EBUSY;
> > + }
> > +
>
> If I'm understanding correctly, this will cause
>
> # echo 0 > /sys/devices/system/cpu/cpuX/online
>
> to sometimes fail, and userspace is expected to try again? This will
> break existing applications.
>
> Perhaps drivers/base/cpu.c:store_online should retry as long as
> cpu_up/down return -EBUSY. That would avoid a userspace-visible
> interface change.
yeah. I'd even suggest a freeze_processes_nofail() API instead, that
does this internally, without burdening the callsites. (and once the
freezer becomes complete then freeze_processes_nofail() ==
freeze_processes())
Ingo
-
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/
Re: [PATCH 3/8] Use process freezer for cpu-hotplug
Gautham R Shenoy wrote:
> This patch implements process_freezer based cpu-hotplug
> core.
> The sailent features are:
> o No more (un)lock_cpu_hotplug.
> o No more CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE. Hence no per-subsystem
> hotcpu mutexes.
> o Calls freeze_process/thaw_processes at the beginning/end of
> the hotplug operation.
...
> @@ -133,7 +111,11 @@ static int _cpu_down(unsigned int cpu)
> if (!cpu_online(cpu))
> return -EINVAL;
>
> - raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE, hcpu);
> + if (freeze_processes(FE_HOTPLUG_CPU)) {
> + thaw_processes(FE_HOTPLUG_CPU);
> + return -EBUSY;
> + }
> +
If I'm understanding correctly, this will cause
# echo 0 > /sys/devices/system/cpu/cpuX/online
to sometimes fail, and userspace is expected to try again? This will
break existing applications.
Perhaps drivers/base/cpu.c:store_online should retry as long as
cpu_up/down return -EBUSY. That would avoid a userspace-visible
interface change.
-
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/
Re: [PATCH 3/8] Use process freezer for cpu-hotplug
On 04/05, Gautham R Shenoy wrote: > > > > @@ -147,6 +147,7 @@ cpu_callback(struct notifier_block *nfb, > > > case CPU_DEAD: > > > p = per_cpu(watchdog_task, hotcpu); > > > per_cpu(watchdog_task, hotcpu) = NULL; > > > + thaw_process(p); > > > kthread_stop(p); > > > > As it was already discussed, this is racy. As Srivatsa (imho rightly) > > suggested, kthread_stop(p) should thaw process itself. This also allows > > us to kill at least some of wait_for_die loops. > > > > Well, in this case this is not racy. Remember, we're doing a > thaw_process(p) in CPU_DEAD where p *is* frozen for cpu hotplug. So > the where we might call a freeze_process(p) after we do a thaw_process > doesn't seem to be feasible. Oops, yes. > > However, the change in kthread_stop(p) in not enough to close the race. > > We can check kthread_should_stop() in refrigerator(), this looks like > > a most simple approach for now. > > > > Why the check kthread_should_stop() refrigerator() ? > As vatsa mentioned, we would be doing > > task_lock(p); > freezer_exempt(p, FE_ALL); /* Doesn't exist as of now, but we can work > it out */ > thaw_process(p); > task_unlock(p); Please look at http://marc.info/?l=linux-kernel&m=117562018530190, we can't change p->flags unless we know it is frozen. > > Alternatively, Srivatsa suggests to introduce a new task_lock() protected > > task_struct->freezer_state (so we can reliably set FE_ALL). Surely this is > > more poweful, but needs more changes. I am not sure. Perhaps we can do > > this later. > > This needs an extra field! We're supposed to be miserly when it comes to > adding new fields to task_struct, now aren't we :-) That is why "Perhaps we can do this later" :) > > In any case, imho "try3" should add thaw_process() to kthread_stop(). > > Gautham, Srivatsa, do you agree? > > > > Completely. Working on it now. Great! Oleg. - 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/
Re: [PATCH 3/8] Use process freezer for cpu-hotplug
On Thu, Apr 05, 2007 at 02:53:56PM +0400, Oleg Nesterov wrote:
> On 04/02, Gautham R Shenoy wrote:
> >
> > + if (freeze_processes(FE_HOTPLUG_CPU)) {
> > + thaw_processes(FE_HOTPLUG_CPU);
> > + return -EBUSY;
> > + }
>
> Off-topic. This is a common pattern. Perhaps it makes sense to
> introduce a try_to_freeze_or_thaw_and_return_an_error() helper.
Not a bad idea.
>
> > @@ -161,10 +141,13 @@ static int _cpu_down(unsigned int cpu)
> > hcpu) == NOTIFY_BAD)
> > BUG();
> >
> > - if (IS_ERR(p)) {
> > + set_cpus_allowed(current, old_allowed);
> > +
> > + if (IS_ERR(p))
> > err = PTR_ERR(p);
> > - goto out_allowed;
> > - }
> > + else
> > + err = kthread_stop(p);
> > +
> > goto out_thread;
> > }
>
> Why this change? We are doing kthread_stop() + set_cpus_allowed() on
> return. Imho,
>
> if (IS_ERR(p))
> goto out_allowed;
> goto out_thread;
>
> looks a bit better. Yes we need a couple of error labels at the end.
Yes, that looks feasible and nice. But I remember making this change for
some subtle reason which I cannot recollect now.
>
> > --- linux-2.6.21-rc5.orig/kernel/softlockup.c
> > +++ linux-2.6.21-rc5/kernel/softlockup.c
> > @@ -147,6 +147,7 @@ cpu_callback(struct notifier_block *nfb,
> > case CPU_DEAD:
> > p = per_cpu(watchdog_task, hotcpu);
> > per_cpu(watchdog_task, hotcpu) = NULL;
> > + thaw_process(p);
> > kthread_stop(p);
>
> As it was already discussed, this is racy. As Srivatsa (imho rightly)
> suggested, kthread_stop(p) should thaw process itself. This also allows
> us to kill at least some of wait_for_die loops.
>
Well, in this case this is not racy. Remember, we're doing a
thaw_process(p) in CPU_DEAD where p *is* frozen for cpu hotplug. So
the where we might call a freeze_process(p) after we do a thaw_process
doesn't seem to be feasible.
But I agree, we should definitely all thaw_process within kthread_stop.
> However, the change in kthread_stop(p) in not enough to close the race.
> We can check kthread_should_stop() in refrigerator(), this looks like
> a most simple approach for now.
>
Why the check kthread_should_stop() refrigerator() ?
As vatsa mentioned, we would be doing
task_lock(p);
freezer_exempt(p, FE_ALL); /* Doesn't exist as of now, but we can work
it out */
thaw_process(p);
task_unlock(p);
wait_for_completion();
So we are serializing the whole thing with task_lock() right?
> Alternatively, Srivatsa suggests to introduce a new task_lock() protected
> task_struct->freezer_state (so we can reliably set FE_ALL). Surely this is
> more poweful, but needs more changes. I am not sure. Perhaps we can do
> this later.
This needs an extra field! We're supposed to be miserly when it comes to
adding new fields to task_struct, now aren't we :-)
>
> In any case, imho "try3" should add thaw_process() to kthread_stop().
> Gautham, Srivatsa, do you agree?
>
Completely. Working on it now.
> Oleg.
>
--
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/
Re: [PATCH 3/8] Use process freezer for cpu-hotplug
On 04/02, Gautham R Shenoy wrote:
>
> + if (freeze_processes(FE_HOTPLUG_CPU)) {
> + thaw_processes(FE_HOTPLUG_CPU);
> + return -EBUSY;
> + }
Off-topic. This is a common pattern. Perhaps it makes sense to
introduce a try_to_freeze_or_thaw_and_return_an_error() helper.
> @@ -161,10 +141,13 @@ static int _cpu_down(unsigned int cpu)
> hcpu) == NOTIFY_BAD)
> BUG();
>
> - if (IS_ERR(p)) {
> + set_cpus_allowed(current, old_allowed);
> +
> + if (IS_ERR(p))
> err = PTR_ERR(p);
> - goto out_allowed;
> - }
> + else
> + err = kthread_stop(p);
> +
> goto out_thread;
> }
Why this change? We are doing kthread_stop() + set_cpus_allowed() on
return. Imho,
if (IS_ERR(p))
goto out_allowed;
goto out_thread;
looks a bit better. Yes we need a couple of error labels at the end.
> --- linux-2.6.21-rc5.orig/kernel/softlockup.c
> +++ linux-2.6.21-rc5/kernel/softlockup.c
> @@ -147,6 +147,7 @@ cpu_callback(struct notifier_block *nfb,
> case CPU_DEAD:
> p = per_cpu(watchdog_task, hotcpu);
> per_cpu(watchdog_task, hotcpu) = NULL;
> + thaw_process(p);
> kthread_stop(p);
As it was already discussed, this is racy. As Srivatsa (imho rightly)
suggested, kthread_stop(p) should thaw process itself. This also allows
us to kill at least some of wait_for_die loops.
However, the change in kthread_stop(p) in not enough to close the race.
We can check kthread_should_stop() in refrigerator(), this looks like
a most simple approach for now.
Alternatively, Srivatsa suggests to introduce a new task_lock() protected
task_struct->freezer_state (so we can reliably set FE_ALL). Surely this is
more poweful, but needs more changes. I am not sure. Perhaps we can do
this later.
In any case, imho "try3" should add thaw_process() to kthread_stop().
Gautham, Srivatsa, do you agree?
Oleg.
-
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/
[PATCH 3/8] Use process freezer for cpu-hotplug
This patch implements process_freezer based cpu-hotplug
core.
The sailent features are:
o No more (un)lock_cpu_hotplug.
o No more CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE. Hence no per-subsystem
hotcpu mutexes.
o Calls freeze_process/thaw_processes at the beginning/end of
the hotplug operation.
Lessons Learnt:
o CPU_UP_PREPARE has to be called with the processes frozen.
If implemented otherwise, the kernel threads which we create
during UP_PREPARE will never be frozen during freeze_processes
since we them up only during CPU_ONLIN.
Points to ponder:
o Can the SYSTEM_RUNNING hack in _cpu_up be avoided by some cleaner means.
Signed-off-by : Srivatsa Vaddagiri <[EMAIL PROTECTED]>
Signed-off-by : Gautham R Shenoy <[EMAIL PROTECTED]>
Signed-off-by : Rafael J. Wysocki <[EMAIL PROTECTED]>
--
include/linux/freezer.h | 10 --
include/linux/notifier.h |2 -
include/linux/sched.h|5 ++-
kernel/cpu.c | 70 ++-
kernel/sched.c | 20 +
kernel/softirq.c | 13
kernel/softlockup.c |1
kernel/workqueue.c | 33 ++
mm/slab.c|6
9 files changed, 63 insertions(+), 97 deletions(-)
Index: linux-2.6.21-rc5/include/linux/sched.h
===
--- linux-2.6.21-rc5.orig/include/linux/sched.h
+++ linux-2.6.21-rc5/include/linux/sched.h
@@ -1195,10 +1195,13 @@ static inline void put_task_struct(struc
#define PF_FE_KPROBES 0x0010 /* This thread should not be frozen
* for Kprobes
*/
+#define PF_FE_HOTPLUG_CPU 0x0020 /* Thread should not be frozen for
+* cpu hotplug.
+*/
#define PF_FROZEN 0x0001 /* frozen for system suspend */
-#define PF_FE_ALL (PF_FE_SUSPEND | PF_FE_KPROBES)
+#define PF_FE_ALL (PF_FE_SUSPEND | PF_FE_KPROBES | PF_FE_HOTPLUG_CPU)
/* Exempt from all kinds freeze chills*/
#define PF_FSTRANS 0x0002 /* inside a filesystem transaction */
Index: linux-2.6.21-rc5/include/linux/freezer.h
===
--- linux-2.6.21-rc5.orig/include/linux/freezer.h
+++ linux-2.6.21-rc5/include/linux/freezer.h
@@ -6,10 +6,12 @@
defined(CONFIG_KPROBES)
/* These are the events which make use of the process freezer */
-#define FE_NONE0
-#define FE_ALL PF_FE_ALL
-#define FE_SUSPEND PF_FE_SUSPEND
-#define FE_KPROBES PF_FE_KPROBES
+#define FE_NONE0
+#define FE_ALL PF_FE_ALL
+#define FE_SUSPEND PF_FE_SUSPEND
+#define FE_KPROBES PF_FE_KPROBES
+#define FE_HOTPLUG_CPU PF_FE_HOTPLUG_CPU
+#define FE_SUSPEND_KPROBES PF_FE_SUSPEND | PF_FE_KPROBES
/*
* Exempt the current process from being frozen for a certain event
Index: linux-2.6.21-rc5/kernel/cpu.c
===
--- linux-2.6.21-rc5.orig/kernel/cpu.c
+++ linux-2.6.21-rc5/kernel/cpu.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
/* This protects CPUs going up and down... */
static DEFINE_MUTEX(cpu_add_remove_lock);
@@ -28,38 +29,15 @@ static int cpu_hotplug_disabled;
#ifdef CONFIG_HOTPLUG_CPU
-/* Crappy recursive lock-takers in cpufreq! Complain loudly about idiots */
-static struct task_struct *recursive;
-static int recursive_depth;
-
void lock_cpu_hotplug(void)
{
- struct task_struct *tsk = current;
-
- if (tsk == recursive) {
- static int warnings = 10;
- if (warnings) {
- printk(KERN_ERR "Lukewarm IQ detected in hotplug
locking\n");
- WARN_ON(1);
- warnings--;
- }
- recursive_depth++;
- return;
- }
- mutex_lock(&cpu_bitmask_lock);
- recursive = tsk;
+ return;
}
EXPORT_SYMBOL_GPL(lock_cpu_hotplug);
void unlock_cpu_hotplug(void)
{
- WARN_ON(recursive != current);
- if (recursive_depth) {
- recursive_depth--;
- return;
- }
- recursive = NULL;
- mutex_unlock(&cpu_bitmask_lock);
+ return;
}
EXPORT_SYMBOL_GPL(unlock_cpu_hotplug);
@@ -133,7 +111,11 @@ static int _cpu_down(unsigned int cpu)
if (!cpu_online(cpu))
return -EINVAL;
- raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE, hcpu);
+ if (freeze_processes(FE_HOTPLUG_CPU)) {
+ thaw_processes(FE_HOTPLUG_CPU);
+ return -EBUSY;
+ }
+
err = __raw_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE,
hcpu, -1,
