From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751465AbdISR5g (ORCPT ); Tue, 19 Sep 2017 13:57:36 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:48820 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751307AbdISR5e (ORCPT ); Tue, 19 Sep 2017 13:57:34 -0400 Date: Tue, 19 Sep 2017 10:57:28 -0700 From: "Paul E. McKenney" To: Peter Zijlstra Cc: mingo@kernel.org, tglx@linutronix.de, linux-kernel@vger.kernel.org, Byungchul Park , Sebastian Andrzej Siewior , Mike Galbraith Subject: Re: [PATCH 2/2] smp/hotplug,lockdep: Annotate cpuhp_state Reply-To: paulmck@linux.vnet.ibm.com References: <20170905075218.526515965@infradead.org> <20170905075351.745979795@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170905075351.745979795@infradead.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 x-cbid: 17091917-0048-0000-0000-000001E7CF9F X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007763; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000230; SDB=6.00919373; UDB=6.00461893; IPR=6.00699599; BA=6.00005598; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017213; XFM=3.00000015; UTC=2017-09-19 17:57:32 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17091917-0049-0000-0000-0000429D2E99 Message-Id: <20170919175728.GA27617@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-19_07:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1709190251 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 05, 2017 at 09:52:20AM +0200, Peter Zijlstra wrote: > After the st->done annotation, lockdep cross-release now complains > about: > > CPU0 CPU1 CPU2 > cpuhp_up_callbacks: takedown_cpu: cpuhp_thread_fun: > > cpuhp_state > irq_lock_sparse() > irq_lock_sparse() > wait_for_completion() > cpuhp_state > complete() > > which again spells deadlock, because CPU0 needs to wait for CPU1's > irq_lock_sparse which will wait for CPU2's completion, which in turn > waits for CPU0's cpuhp_state. > > Now, this again mixes up and down chains, but now on cpuhp_state. > > Cc: Thomas Gleixner > Cc: Byungchul Park > Cc: Sebastian Andrzej Siewior > Reported-by: Mike Galbraith > Tested-by: Mike Galbraith > Signed-off-by: Peter Zijlstra (Intel) > --- > kernel/cpu.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > --- a/kernel/cpu.c > +++ b/kernel/cpu.c > @@ -67,11 +67,14 @@ struct cpuhp_cpu_state { > static DEFINE_PER_CPU(struct cpuhp_cpu_state, cpuhp_state); > > #if defined(CONFIG_LOCKDEP) && defined(CONFIG_SMP) > -static struct lock_class_key cpuhp_state_key; > +static struct lock_class_key cpuhp_state_up_key; > +#ifdef CONFIG_HOTPLUG_CPU > +static struct lock_class_key cpuhp_state_down_key; > +#endif These two patches work for me if CONFIG_PROVE_LOCKING=y, but the lockdep_init_map() wants its key argument to exist if CONFIG_PROVE_LOCKING=n. Not sure whether it is better to remove the CONFIG_LOCKDEP #if on the one hand or to make lockdep_init_map() lose the "(void)" things on the other... My tests didn't involve failing CPU hotplug operations, so I didn't run into the issue Thomas was concerned about. Thanx, Paul > static struct lockdep_map cpuhp_state_lock_map = > - STATIC_LOCKDEP_MAP_INIT("cpuhp_state", &cpuhp_state_key); > + STATIC_LOCKDEP_MAP_INIT("cpuhp_state-up", &cpuhp_state_up_key); > #endif > > /** > * cpuhp_step - Hotplug state machine step > * @name: Name of the step > @@ -714,6 +718,8 @@ static int __ref _cpu_down(unsigned int > cpus_write_lock(); > > lockdep_reinit_st_done(); > + lockdep_init_map(&cpuhp_state_lock_map, "cpuhp_state-down", > + &cpuhp_state_down_key, 0); > > cpuhp_tasks_frozen = tasks_frozen; > > @@ -828,6 +834,8 @@ static int _cpu_up(unsigned int cpu, int > cpus_write_lock(); > > lockdep_reinit_st_done(); > + lockdep_init_map(&cpuhp_state_lock_map, "cpuhp_state-up", > + &cpuhp_state_up_key, 0); > > if (!cpu_present(cpu)) { > ret = -EINVAL; > >