| Message ID | alpine.DEB.2.11.1501211243270.5526@nanos (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 01/21/2015 05:16 PM, Thomas Gleixner wrote: > On Tue, 20 Jan 2015, Preeti U Murthy wrote: >> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c >> index 5544990..f3907c9 100644 >> --- a/kernel/time/clockevents.c >> +++ b/kernel/time/clockevents.c >> @@ -568,6 +568,7 @@ int clockevents_notify(unsigned long reason, void *arg) >> >> case CLOCK_EVT_NOTIFY_CPU_DYING: >> tick_handover_do_timer(arg); >> + tick_shutdown_broadcast_oneshot(arg); >> break; >> >> case CLOCK_EVT_NOTIFY_SUSPEND: >> @@ -580,7 +581,6 @@ int clockevents_notify(unsigned long reason, void *arg) >> break; >> >> case CLOCK_EVT_NOTIFY_CPU_DEAD: >> - tick_shutdown_broadcast_oneshot(arg); >> tick_shutdown_broadcast(arg); >> tick_shutdown(arg); >> /* >> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c >> index 066f0ec..f983983 100644 >> --- a/kernel/time/tick-broadcast.c >> +++ b/kernel/time/tick-broadcast.c >> @@ -675,8 +675,11 @@ static void broadcast_move_bc(int deadcpu) >> >> if (!bc || !broadcast_needs_cpu(bc, deadcpu)) >> return; >> - /* This moves the broadcast assignment to this cpu */ >> - clockevents_program_event(bc, bc->next_event, 1); >> + /* Since a cpu with the earliest wakeup is nominated as the >> + * standby cpu, the next cpu to invoke BROADCAST_ENTER >> + * will now automatically take up the duty of broadcasting. >> + */ >> + bc->next_event.tv64 = KTIME_MAX; > > So that relies on the fact, that cpu_down() currently forces ALL cpus > into stop_machine(). Of course this is not in any way obvious and any > change to this will cause even more hard to debug issues. Hmm.. true this is a concern. > > And to be honest, the clever 'set next_event to KTIME_MAX' is even > more nonobvious because it's only relevant for your hrtimer based > broadcasting magic. Any real broadcast device does not care about this > at all. bc->next_event is set to max only if CLOCK_EVT_FEATURE_HRTIMER is true. It does not affect the usual broadcast logic. > > This whole random notifier driven hotplug business is just a > trainwreck. I'm still trying to convert this to a well documented > state machine, so I rather prefer to make this an explicit take over > rather than a completely undocumented 'works today' mechanism. > > What about the patch below? > > Thanks, > > tglx > ---- > diff --git a/kernel/cpu.c b/kernel/cpu.c > index 5d220234b3ca..7a9b1ae4a945 100644 > --- a/kernel/cpu.c > +++ b/kernel/cpu.c > @@ -16,6 +16,7 @@ > #include <linux/bug.h> > #include <linux/kthread.h> > #include <linux/stop_machine.h> > +#include <linux/clockchips.h> > #include <linux/mutex.h> > #include <linux/gfp.h> > #include <linux/suspend.h> > @@ -421,6 +422,12 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen) > while (!idle_cpu(cpu)) > cpu_relax(); > > + /* > + * Before waiting for the cpu to enter DEAD state, take over > + * any tick related duties > + */ > + clockevents_notify(CLOCK_EVT_NOTIFY_CPU_DEAD, &cpu); > + > /* This actually kills the CPU. */ > __cpu_die(cpu); > > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c > index 37e50aadd471..3c1bfd0f7074 100644 > --- a/kernel/time/hrtimer.c > +++ b/kernel/time/hrtimer.c > @@ -1721,11 +1721,8 @@ static int hrtimer_cpu_notify(struct notifier_block *self, > break; > case CPU_DEAD: > case CPU_DEAD_FROZEN: > - { > - clockevents_notify(CLOCK_EVT_NOTIFY_CPU_DEAD, &scpu); > migrate_hrtimers(scpu); > break; > - } > #endif > > default: > How about when the cpu that is going offline receives a timer interrupt just before setting its state to CPU_DEAD ? That is still possible right given that its clock devices may not have been shutdown and it is capable of receiving interrupts for a short duration. Even with the above patch, is the following scenario possible ? CPU0 CPU1 t0 Receives timer interrupt t1 Sees that there are hrtimers to be serviced (hrtimers are not yet migrated) t2 calls hrtimer_interrupt() t3 tick_program_event() CPU_DEAD notifiers CPU0's td->evtdev = NULL t4 clockevent_program_event() references NULL tick device pointer So my concern is that since the CLOCK_EVT_NOTIFY_CPU_DEAD callback handles shutting down of devices besides moving tick related duties. it's functions may race with the hotplug cpu still handling tick events. We do check on powerpc if the timer interrupt has arrived on an offline cpu, but that is to avoid an entirely different scenario and not one like the above. I would not expect the arch to check if a timer interrupt arrived on an offline cpu. A timer interrupt may be serviced as long as the tick device is alive. Regards Preeti U Murthy
On Thu, 22 Jan 2015, Preeti U Murthy wrote: > On 01/21/2015 05:16 PM, Thomas Gleixner wrote: > How about when the cpu that is going offline receives a timer interrupt > just before setting its state to CPU_DEAD ? That is still possible right > given that its clock devices may not have been shutdown and it is > capable of receiving interrupts for a short duration. Even with the > above patch, is the following scenario possible ? > > CPU0 CPU1 > t0 Receives timer interrupt > > t1 Sees that there are hrtimers > to be serviced (hrtimers are not yet migrated) > > t2 calls hrtimer_interrupt() > > t3 tick_program_event() CPU_DEAD notifiers > CPU0's td->evtdev = NULL > > t4 clockevent_program_event() > references NULL tick device pointer > > So my concern is that since the CLOCK_EVT_NOTIFY_CPU_DEAD callback > handles shutting down of devices besides moving tick related duties. > it's functions may race with the hotplug cpu still handling tick events. __cpu_disable() is supposed to block interrupts on the dying cpu. But I agree, we should make it more robust. So we want an explicit call for disabling the cpu local stuff and an explicit takeover of the broadcast duty. I'm anyway distangling the clockevents_notify() stuff, so it should be simple to do so. Thanks, tglx
On 01/22/2015 04:45 PM, Thomas Gleixner wrote: > On Thu, 22 Jan 2015, Preeti U Murthy wrote: >> On 01/21/2015 05:16 PM, Thomas Gleixner wrote: >> How about when the cpu that is going offline receives a timer interrupt >> just before setting its state to CPU_DEAD ? That is still possible right >> given that its clock devices may not have been shutdown and it is >> capable of receiving interrupts for a short duration. Even with the >> above patch, is the following scenario possible ? >> >> CPU0 CPU1 >> t0 Receives timer interrupt >> >> t1 Sees that there are hrtimers >> to be serviced (hrtimers are not yet migrated) >> >> t2 calls hrtimer_interrupt() >> >> t3 tick_program_event() CPU_DEAD notifiers >> CPU0's td->evtdev = NULL >> >> t4 clockevent_program_event() >> references NULL tick device pointer >> >> So my concern is that since the CLOCK_EVT_NOTIFY_CPU_DEAD callback >> handles shutting down of devices besides moving tick related duties. >> it's functions may race with the hotplug cpu still handling tick events. > > __cpu_disable() is supposed to block interrupts on the dying cpu. > > But I agree, we should make it more robust. So we want an explicit > call for disabling the cpu local stuff and an explicit takeover of the > broadcast duty. I'm anyway distangling the clockevents_notify() stuff, > so it should be simple to do so. I noticed that tick_handover_do_timer() function also suffers from the issue that the patch I posted for moving the broadcast duty had, in that it relies on all cpus participating in stop_machine(). In a design where all cpus do not participate in stop_machine(), if the freshly nominated do_timer cpu is idle, there is no update of jiffies till that cpu gets back to being busy. So we must do an explicit take over of *both* the broadcast and do_timer duty just before the CPU_DEAD phase. Regards Preeti U Murthy > Thanks, > > tglx > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev >
On 01/27/2015 09:01 AM, Preeti U Murthy wrote: > On 01/22/2015 04:45 PM, Thomas Gleixner wrote: >> On Thu, 22 Jan 2015, Preeti U Murthy wrote: >>> On 01/21/2015 05:16 PM, Thomas Gleixner wrote: >>> How about when the cpu that is going offline receives a timer interrupt >>> just before setting its state to CPU_DEAD ? That is still possible right >>> given that its clock devices may not have been shutdown and it is >>> capable of receiving interrupts for a short duration. Even with the >>> above patch, is the following scenario possible ? >>> >>> CPU0 CPU1 >>> t0 Receives timer interrupt >>> >>> t1 Sees that there are hrtimers >>> to be serviced (hrtimers are not yet migrated) >>> >>> t2 calls hrtimer_interrupt() >>> >>> t3 tick_program_event() CPU_DEAD notifiers >>> CPU0's td->evtdev = NULL >>> >>> t4 clockevent_program_event() >>> references NULL tick device pointer >>> >>> So my concern is that since the CLOCK_EVT_NOTIFY_CPU_DEAD callback >>> handles shutting down of devices besides moving tick related duties. >>> it's functions may race with the hotplug cpu still handling tick events. >> >> __cpu_disable() is supposed to block interrupts on the dying cpu. >> >> But I agree, we should make it more robust. So we want an explicit >> call for disabling the cpu local stuff and an explicit takeover of the >> broadcast duty. I'm anyway distangling the clockevents_notify() stuff, >> so it should be simple to do so. Thomas ping. Would you be posting this patch? > > I noticed that tick_handover_do_timer() function also suffers from the > issue that the patch I posted for moving the broadcast duty had, in that > it relies on all cpus participating in stop_machine(). In a design where > all cpus do not participate in stop_machine(), if the freshly nominated > do_timer cpu is idle, there is no update of jiffies till that cpu gets > back to being busy. So we must do an explicit take over of *both* the > broadcast and do_timer duty just before the CPU_DEAD phase. Regards Preeti u Murthy
On Wed, Jan 28, 2015 at 03:32:58PM +0530, Preeti U Murthy wrote:
> Thomas ping. Would you be posting this patch?
FYI, Thomas is temporarily out of action, in bed with the flu.
Thanks,
Richard
On 01/29/2015 03:01 AM, Richard Cochran wrote: > On Wed, Jan 28, 2015 at 03:32:58PM +0530, Preeti U Murthy wrote: >> Thomas ping. Would you be posting this patch? > > FYI, Thomas is temporarily out of action, in bed with the flu. Oh I am sorry to hear that! Let me post out a patch based on Thomas's suggestions around this. Wishing him a speedy recovery. Regards Preeti U Murthy > > Thanks, > Richard >
diff --git a/kernel/cpu.c b/kernel/cpu.c index 5d220234b3ca..7a9b1ae4a945 100644 --- a/kernel/cpu.c +++ b/kernel/cpu.c @@ -16,6 +16,7 @@ #include <linux/bug.h> #include <linux/kthread.h> #include <linux/stop_machine.h> +#include <linux/clockchips.h> #include <linux/mutex.h> #include <linux/gfp.h> #include <linux/suspend.h> @@ -421,6 +422,12 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen) while (!idle_cpu(cpu)) cpu_relax(); + /* + * Before waiting for the cpu to enter DEAD state, take over + * any tick related duties + */ + clockevents_notify(CLOCK_EVT_NOTIFY_CPU_DEAD, &cpu); + /* This actually kills the CPU. */ __cpu_die(cpu); diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c index 37e50aadd471..3c1bfd0f7074 100644 --- a/kernel/time/hrtimer.c +++ b/kernel/time/hrtimer.c @@ -1721,11 +1721,8 @@ static int hrtimer_cpu_notify(struct notifier_block *self, break; case CPU_DEAD: case CPU_DEAD_FROZEN: - { - clockevents_notify(CLOCK_EVT_NOTIFY_CPU_DEAD, &scpu); migrate_hrtimers(scpu); break; - } #endif default: