linux-kernel - Re: [PATCH V2] clockevents: Fix cpu down race for hrtimer based broadcasting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <551D2733.7040108@linux.vnet.ibm.com>
Date:	Thu, 02 Apr 2015 16:55:39 +0530
From:	Preeti U Murthy <preeti@...ux.vnet.ibm.com>
To:	Ingo Molnar <mingo@...nel.org>, peterz@...radead.org
CC:	mpe@...erman.id.au, tglx@...utronix.de, rjw@...ysocki.net,
	nicolas.pitre@...aro.org, linuxppc-dev@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] clockevents: Fix cpu down race for hrtimer based broadcasting

On 04/02/2015 04:12 PM, Ingo Molnar wrote:
> 
> * Preeti U Murthy <preeti@...ux.vnet.ibm.com> wrote:
> 
>> It was found when doing a hotplug stress test on POWER, that the machine
>> either hit softlockups or rcu_sched stall warnings.  The issue was
>> traced to commit 7cba160ad789a powernv/cpuidle: Redesign idle states
>> management, which exposed the cpu down race with hrtimer based broadcast
>> mode(Commit 5d1638acb9f6(tick: Introduce hrtimer based broadcast). This
>> is explained below.
>>
>> Assume CPU1 is the CPU which holds the hrtimer broadcasting duty before
>> it is taken down.
>>
>> CPU0					CPU1
>>
>> cpu_down()				take_cpu_down()
>> 					disable_interrupts()
>>
>> cpu_die()
>>
>>  while(CPU1 != CPU_DEAD) {
>>   msleep(100);
>>    switch_to_idle();
>>     stop_cpu_timer();
>>      schedule_broadcast();
>>  }
>>
>> tick_cleanup_cpu_dead()
>> 	take_over_broadcast()
>>
>> So after CPU1 disabled interrupts it cannot handle the broadcast hrtimer
>> anymore, so CPU0 will be stuck forever.
>>
>> Fix this by explicitly taking over broadcast duty before cpu_die().
>> This is a temporary workaround. What we really want is a callback in the
>> clockevent device which allows us to do that from the dying CPU by
>> pushing the hrtimer onto a different cpu. That might involve an IPI and
>> is definitely more complex than this immediate fix.
> 
> So why not use a suitable CPU_DOWN* notifier for this, instead of open 
> coding it all into a random place in the hotplug machinery?

This is because each of them is unsuitable for a reason:

1. CPU_DOWN_PREPARE stage allows for a fail. The cpu in question may not
successfully go down. So we may pull the hrtimer unnecessarily.

2. CPU_DYING notifiers are run on the cpu that is going down. So the
alternative would be to IPI an online cpu to take up the broadcast duty.

3. CPU_DEAD and CPU_POST_DEAD stages both have the drawback described in
the changelog.

I hope I got your question right.

Regards
Preeti U Murthy
> 
> Also, I improved the changelog (attached below), but decided against 
> applying it until these questions are cleared - please use that for 
> future versions of this patch.
> 
> Thanks,
> 
> 	Ingo
> 
> ===================>
> From 413fbf5193b330c5f478ef7aaeaaee08907a993e Mon Sep 17 00:00:00 2001
> From: Preeti U Murthy <preeti@...ux.vnet.ibm.com>
> Date: Mon, 30 Mar 2015 14:59:19 +0530
> Subject: [PATCH] clockevents: Fix cpu_down() race for hrtimer based broadcasting
> 
> It was found when doing a hotplug stress test on POWER, that the
> machine either hit softlockups or rcu_sched stall warnings.  The
> issue was traced to commit:
> 
>   7cba160ad789 ("powernv/cpuidle: Redesign idle states management")
> 
> which exposed the cpu_down() race with hrtimer based broadcast mode:
> 
>   5d1638acb9f6 ("tick: Introduce hrtimer based broadcast")
> 
> The race is the following:
> 
> Assume CPU1 is the CPU which holds the hrtimer broadcasting duty
> before it is taken down.
> 
> 	CPU0					CPU1
> 
> 	cpu_down()				take_cpu_down()
> 						disable_interrupts()
> 
> 	cpu_die()
> 
> 	while (CPU1 != CPU_DEAD) {
> 		msleep(100);
> 		switch_to_idle();
> 		stop_cpu_timer();
> 		schedule_broadcast();
> 	}
> 
> 	tick_cleanup_cpu_dead()
> 		take_over_broadcast()
> 
> So after CPU1 disabled interrupts it cannot handle the broadcast
> hrtimer anymore, so CPU0 will be stuck forever.
> 
> Fix this by explicitly taking over broadcast duty before cpu_die().
> 
> This is a temporary workaround. What we really want is a callback
> in the clockevent device which allows us to do that from the dying
> CPU by pushing the hrtimer onto a different cpu. That might involve
> an IPI and is definitely more complex than this immediate fix.
> 
> Changelog was picked up from:
> 
>     https://lkml.org/lkml/2015/2/16/213
> 
> Suggested-by: Thomas Gleixner <tglx@...utronix.de>
> Tested-by: Nicolas Pitre <nico@...aro.org>
> Signed-off-by: Preeti U. Murthy <preeti@...ux.vnet.ibm.com>
> Cc: linuxppc-dev@...ts.ozlabs.org
> Cc: mpe@...erman.id.au
> Cc: nicolas.pitre@...aro.org
> Cc: peterz@...radead.org
> Cc: rjw@...ysocki.net
> Fixes: http://linuxppc.10917.n7.nabble.com/offlining-cpus-breakage-td88619.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/