All of lore.kernel.org
 help / color / mirror / Atom feed
From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: aik@ozlabs.ru, shreyas@linux.vnet.ibm.com,
	LKML <linux-kernel@vger.kernel.org>,
	michael@ellerman.id.au, Peter Zijlstra <peterz@infradead.org>,
	Anton Blanchard <anton@samba.org>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH V3] tick/broadcast: Make movement of broadcast hrtimer robust against hotplug
Date: Tue, 27 Jan 2015 09:01:23 +0530	[thread overview]
Message-ID: <54C7068B.3050108@linux.vnet.ibm.com> (raw)
In-Reply-To: <alpine.DEB.2.11.1501221209260.5526@nanos>

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
> 

WARNING: multiple messages have this Message-ID (diff)
From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: aik@ozlabs.ru, shreyas@linux.vnet.ibm.com,
	LKML <linux-kernel@vger.kernel.org>,
	michael@ellerman.id.au, Peter Zijlstra <peterz@infradead.org>,
	Anton Blanchard <anton@samba.org>,
	linuxppc-dev@lists.ozlabs.org,
	Michael Ellerman <mpe@ellerman.id.au>,
	"svaidy@linux.vnet.ibm.com" <svaidy@linux.vnet.ibm.com>
Subject: Re: [PATCH V3] tick/broadcast: Make movement of broadcast hrtimer robust against hotplug
Date: Tue, 27 Jan 2015 09:01:23 +0530	[thread overview]
Message-ID: <54C7068B.3050108@linux.vnet.ibm.com> (raw)
In-Reply-To: <alpine.DEB.2.11.1501221209260.5526@nanos>

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
> 


  reply	other threads:[~2015-01-27  3:31 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-20 10:36 [PATCH V3] tick/broadcast: Make movement of broadcast hrtimer robust against hotplug Preeti U Murthy
2015-01-20 10:36 ` Preeti U Murthy
2015-01-21 11:46 ` Thomas Gleixner
2015-01-21 11:46   ` Thomas Gleixner
2015-01-22  6:07   ` Preeti U Murthy
2015-01-22  6:07     ` Preeti U Murthy
2015-01-22 11:15     ` Thomas Gleixner
2015-01-22 11:15       ` Thomas Gleixner
2015-01-27  3:31       ` Preeti U Murthy [this message]
2015-01-27  3:31         ` Preeti U Murthy
2015-01-28 10:02         ` Preeti U Murthy
2015-01-28 21:31           ` Richard Cochran
2015-01-28 21:31             ` Richard Cochran
2015-01-29  4:52             ` Preeti U Murthy
2015-01-29  4:52               ` Preeti U Murthy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54C7068B.3050108@linux.vnet.ibm.com \
    --to=preeti@linux.vnet.ibm.com \
    --cc=aik@ozlabs.ru \
    --cc=anton@samba.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=michael@ellerman.id.au \
    --cc=peterz@infradead.org \
    --cc=shreyas@linux.vnet.ibm.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.