Re: [PATCH] arm64: smp: fix smp_send_stop() behaviour

[Cristian Marussi <cristian.marussi@arm.com>]

Hi

On 18/06/2019 13:54, Russell King - ARM Linux admin wrote:
> On Mon, Jun 17, 2019 at 07:09:13PM +0100, Will Deacon wrote:
>> [+James M]
>>
>> On Thu, Jun 13, 2019 at 01:21:46PM +0100, Cristian Marussi wrote:
>>> On a 2-CPUs system, when one CPU is already online if the other
>>> panics while starting-up, smp_send_stop() will fail to send any
>>> STOP message to the other already online core, resulting in a
>>> system still responsive and alive at the end of the panic procedure.
>>> This patch makes smp_send_stop() account also for the online status
>>> of the calling CPU while evaluating how many CPUs are effectively
>>> online: this way, an adequate number of STOPs is sent, so enforcing
>>> a proper freeze of the system at the end of panic even under the
>>> above conditions.
>>>
>>> Reported-by: Dave Martin <Dave.Martin@arm.com>
>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>> ---
>>>
>>> This peculiar panic-procedure behaviour was exposed hitting a BUG()
>>> while running a KSFT cpu-hotplug test on a 2-core ARMv8 model.
>>> Such trigger-BUG() was fixed by a distinct commit already included
>>> in Linux 5.2-rc4 [0]
>>>
>>> [0] https://lore.kernel.org/linux-arm-kernel/1559576102-12156-1-git-send-email-Dave.Martin@arm.com/
>>> ---
>>>  arch/arm64/kernel/smp.c | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>>> index bb4b3f07761a..c7d604427883 100644
>>> --- a/arch/arm64/kernel/smp.c
>>> +++ b/arch/arm64/kernel/smp.c
>>> @@ -971,8 +971,14 @@ void tick_broadcast(const struct cpumask *mask)
>>>  void smp_send_stop(void)
>>>  {
>>>  	unsigned long timeout;
>>> +	unsigned int this_cpu_online = cpu_online(smp_processor_id());
>>>  
>>> -	if (num_online_cpus() > 1) {
>>> +	/*
>>> +	 * If this CPU isn't fully online, it will not be counted in
>>> +	 * num_online_cpus(): on a 2-CPU system this situation will
>>> +	 * result in no message being sent to the other already online CPU.
>>> +	 */
>>> +	if (num_online_cpus() > this_cpu_online) {
>>>  		cpumask_t mask;
>>>  
>>>  		cpumask_copy(&mask, cpu_online_mask);
>>> @@ -985,10 +991,10 @@ void smp_send_stop(void)
>>>  
>>>  	/* Wait up to one second for other CPUs to stop */
>>>  	timeout = USEC_PER_SEC;
>>> -	while (num_online_cpus() > 1 && timeout--)
>>> +	while (num_online_cpus() > this_cpu_online && timeout--)
>>>  		udelay(1);
>>>  
>>> -	if (num_online_cpus() > 1)
>>> +	if (num_online_cpus() > this_cpu_online)
>>>  		pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
>>>  			   cpumask_pr_args(cpu_online_mask));
>>
>> Whilst this looks ok to me, I'm worried about whether or not we have this
>> sort of logic elsewhere. For example, do we need to fix
>> crash_smp_send_stop() (and possibly machine_kexec()) too?
> 
> What about other architectures?  This, or very similar code, is present
> on other architectures too.
> 
Thanks for the review,

indeed glancing quickly at other arch's, there is a lot of common things and
subtle differences in handling these situations...potentially some common logic
to fix once for all and abstract out of arch specific code...(or at least try to
do it)....so now a question arises to me: is it still worth to properly fix on
arm64 at first extending this patch (easier and quicker maybe) or should I opt
directly for the abstraction party ?

Thanks

Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel