From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754291AbdDLOsJ (ORCPT ); Wed, 12 Apr 2017 10:48:09 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:44847 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752928AbdDLOsG (ORCPT ); Wed, 12 Apr 2017 10:48:06 -0400 Date: Wed, 12 Apr 2017 07:48:00 -0700 From: "Paul E. McKenney" To: Steven Rostedt Cc: linux-kernel@vger.kernel.org Subject: Re: There is a Tasks RCU stall warning Reply-To: paulmck@linux.vnet.ibm.com References: <20170411173133.52b28cfe@gandalf.local.home> <20170411173447.0bdb9f77@gandalf.local.home> <20170411173900.00f4b6c6@gandalf.local.home> <20170411214443.GH1600@linux.vnet.ibm.com> <20170411174953.46adbf1e@gandalf.local.home> <20170411215656.GI1600@linux.vnet.ibm.com> <20170411181530.27dc21cc@gandalf.local.home> <20170411230154.GA3956@linux.vnet.ibm.com> <20170411230445.GA25951@linux.vnet.ibm.com> <20170411231138.GB25951@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170411231138.GB25951@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 x-cbid: 17041214-0052-0000-0000-000001D3E555 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006923; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000208; SDB=6.00846547; UDB=6.00417573; IPR=6.00624984; BA=6.00005286; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015022; XFM=3.00000013; UTC=2017-04-12 14:48:03 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17041214-0053-0000-0000-00004FE38FDA Message-Id: <20170412144800.GA12365@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-12_11:,, 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-1702020001 definitions=main-1704120123 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 11, 2017 at 04:11:38PM -0700, Paul E. McKenney wrote: > On Tue, Apr 11, 2017 at 04:04:45PM -0700, Paul E. McKenney wrote: > > On Tue, Apr 11, 2017 at 04:01:54PM -0700, Paul E. McKenney wrote: > > > On Tue, Apr 11, 2017 at 06:15:30PM -0400, Steven Rostedt wrote: > > > > On Tue, 11 Apr 2017 14:56:56 -0700 > > > > "Paul E. McKenney" wrote: > > > > > > > > > On Tue, Apr 11, 2017 at 05:49:53PM -0400, Steven Rostedt wrote: > > > > > > On Tue, 11 Apr 2017 14:44:43 -0700 > > > > > > "Paul E. McKenney" wrote: > > > > > > > > > > > > > > > > > > > Works for me! > > > > > > > > > > > > > > Hopefully it will also work for your computer. :-) > > > > > > > > > > > > > > And whew! Glad to see that the stall warnings worked! > > > > > > > > > > > > Ah! but I think I found a bug in synchronize_rcu_tasks()! > > > > > > > > > > > > Calling schedule isn't good enough. For rcu_tasks to continue, the task > > > > > > needs to schedule out. With my updated code, I just triggered: > > > > > > > > > > > > [ 196.276868] INFO: rcu_tasks detected stalls on tasks: > > > > > > [ 196.284294] ffff8800c26f8040: .. nvcsw: 2/2 holdout: 1 idle_cpu: -1/1 > > > > > > [ 196.293175] event_benchmark R running task 30536 1127 2 0x10000000 > > > > > > [ 196.302746] Call Trace: > > > > > > [ 196.307640] ? _raw_spin_unlock_irq+0x1f/0x50 > > > > > > [ 196.314453] __schedule+0x222/0x1210 > > > > > > [ 196.320476] ? pci_mmcfg_check_reserved+0xc0/0xc0 > > > > > > [ 196.327616] ? preempt_count_add+0xb7/0xf0 > > > > > > [ 196.334174] ? __asan_store8+0x15/0x70 > > > > > > [ 196.340384] schedule+0x57/0xe0 > > > > > > [ 196.345888] benchmark_event_kthread+0x2e/0x3c0 > > > > > > [ 196.352823] kthread+0x178/0x1d0 > > > > > > [ 196.358411] ? trace_benchmark_reg+0x80/0x80 > > > > > > [ 196.365073] ? kthread_create_on_node+0xa0/0xa0 > > > > > > [ 196.371999] ret_from_fork+0x2e/0x40 > > > > > > > > > > > > > > > > > > And here my benchmark called schedule(), but nothing scheduled it out, > > > > > > and it still fails on rcu_tasks. > > > > > > > > > > Good point! > > > > > > > > > > Hmmmm... I cannot hook into rcu_note_context_switch() because that gets > > > > > called for preemption as well as for voluntary context switches. > > > > > > > > If you pass in the "preempt" variable, it would work. It's false when > > > > __schedule() was called by a "schedule()" directly, and true when > > > > called by one of the preempt_schedule() functions. > > > > > > Like this? (Untested, but builds at least some of the time.) > > > > Not like that... :-/ Update on its way. > > Perhaps more like this. Started rcutorture on it, will see how it goes. Do you need this patch? If so, I should do some more work on it to eliminate the extra common-case branch on the scheduler fastpath. Thanx, Paul > ------------------------------------------------------------------------ > > commit c9653896be9b79b7227e8b97710ad6475fc72dc1 > Author: Paul E. McKenney > Date: Tue Apr 11 15:50:41 2017 -0700 > > rcu: Make non-preemptive schedule be Tasks RCU quiescent state > > Currently, a call to schedule() acts as a Tasks RCU quiescent state > only if a context switch actually takes place. However, just the > call to schedule() guarantees that the calling task has moved off of > whatever tracing trampoline that it might have been one previously. > This commit therefore plumbs schedule()'s "preempt" parameter into > rcu_note_context_switch(), which then records the Tasks RCU quiescent > state, but only if this call to schedule() was -not- due to a preemption. > > Suggested-by: Steven Rostedt > Signed-off-by: Paul E. McKenney > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index e6146d0074f8..f531b29207da 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -363,15 +363,20 @@ static inline void rcu_init_nohz(void) > #ifdef CONFIG_TASKS_RCU > #define TASKS_RCU(x) x > extern struct srcu_struct tasks_rcu_exit_srcu; > -#define rcu_note_voluntary_context_switch(t) \ > +#define rcu_note_voluntary_context_switch_lite(t) \ > do { \ > - rcu_all_qs(); \ > if (READ_ONCE((t)->rcu_tasks_holdout)) \ > WRITE_ONCE((t)->rcu_tasks_holdout, false); \ > } while (0) > +#define rcu_note_voluntary_context_switch(t) \ > + do { \ > + rcu_all_qs(); \ > + rcu_note_voluntary_context_switch_lite(t); \ > + } while (0) > #else /* #ifdef CONFIG_TASKS_RCU */ > #define TASKS_RCU(x) do { } while (0) > -#define rcu_note_voluntary_context_switch(t) rcu_all_qs() > +#define rcu_note_voluntary_context_switch_lite(t) do { } while (0) > +#define rcu_note_voluntary_context_switch(t) rcu_all_qs() > #endif /* #else #ifdef CONFIG_TASKS_RCU */ > > /** > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > index 5219be250f00..21ea52df651d 100644 > --- a/include/linux/rcutiny.h > +++ b/include/linux/rcutiny.h > @@ -92,10 +92,11 @@ static inline void kfree_call_rcu(struct rcu_head *head, > call_rcu(head, func); > } > > -static inline void rcu_note_context_switch(void) > -{ > - rcu_sched_qs(); > -} > +#define rcu_note_context_switch(preempt) \ > + do { \ > + rcu_sched_qs(); \ > + rcu_note_voluntary_context_switch_lite(current); \ > + } while (0) > > /* > * Take advantage of the fact that there is only one CPU, which > diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h > index 63a4e4cf40a5..04a2caf3ab8b 100644 > --- a/include/linux/rcutree.h > +++ b/include/linux/rcutree.h > @@ -30,7 +30,7 @@ > #ifndef __LINUX_RCUTREE_H > #define __LINUX_RCUTREE_H > > -void rcu_note_context_switch(void); > +void rcu_note_context_switch(bool preempt); > int rcu_needs_cpu(u64 basem, u64 *nextevt); > void rcu_cpu_stall_reset(void); > > @@ -41,7 +41,7 @@ void rcu_cpu_stall_reset(void); > */ > static inline void rcu_virt_note_context_switch(int cpu) > { > - rcu_note_context_switch(); > + rcu_note_context_switch(false); > } > > void synchronize_rcu_bh(void); > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index f13fc4ab2f0d..92cf78fffd31 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -458,13 +458,15 @@ static void rcu_momentary_dyntick_idle(void) > * and requires special handling for preemptible RCU. > * The caller must have disabled interrupts. > */ > -void rcu_note_context_switch(void) > +void rcu_note_context_switch(bool preempt) > { > struct rcu_state *rsp; > > barrier(); /* Avoid RCU read-side critical sections leaking down. */ > trace_rcu_utilization(TPS("Start context switch")); > rcu_sched_qs(); > + if (!preempt) > + rcu_note_voluntary_context_switch_lite(current); > rcu_preempt_note_context_switch(); > /* Load rcu_urgent_qs before other flags. */ > if (!smp_load_acquire(this_cpu_ptr(&rcu_dynticks.rcu_urgent_qs))) > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 9c38e6be4f3e..653ea8cf89e4 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -3378,7 +3378,7 @@ static void __sched notrace __schedule(bool preempt) > hrtick_clear(rq); > > local_irq_disable(); > - rcu_note_context_switch(); > + rcu_note_context_switch(preempt); > > /* > * Make sure that signal_pending_state()->signal_pending() below