All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: "Luck, Tony" <tony.luck@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@ml01.01.org>,
	Borislav Petkov <bp@suse.de>
Subject: Re: [RFC PATCH] x86, mce: change the mce notifier to 'blocking' from 'atomic'
Date: Wed, 12 Apr 2017 15:42:32 -0700	[thread overview]
Message-ID: <20170412224232.GM3956@linux.vnet.ibm.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1704122342130.2548@nanos>

On Wed, Apr 12, 2017 at 11:50:45PM +0200, Thomas Gleixner wrote:
> On Wed, 12 Apr 2017, Borislav Petkov wrote:
> 
> > On Wed, Apr 12, 2017 at 08:27:05PM +0000, Verma, Vishal L wrote:
> > > But isn't the atomic notifier call chain always called in atomic
> > > context?
> > 
> > No, it isn't. We're calling it in normal process context in
> > mce_gen_pool_process() too.
> > 
> > So this early exit will avoid any sleeping in atomic context. And since
> > there's nothing you can do about the errors reported in atomic context,
> > we can actually use that fact.
> 
> No, you can't.
> 
> CONFIG_RCU_PREEMPT=n + CONFIG_PREEMPT_COUNT will disable preemption from
> within __atomic_notifier_call_chain() via rcu_read_lock(). Ergo you wont
> ever enter the handler.
> 
> The behaviour in the RCU code is inconsistent. CONFIG_RCU_PREEMPT=y does
> obviouly not disable preemption, but it should still trigger the
> might_sleep() check when a blocking function is called from within a rcu
> read side critical section.

Maybe something like the (untested) patch below.  Please note that this
would need some help to work correctly in -rt.  This applies only against
-rcu tip, but in that case you can just get it directly from -rcu.

							Thanx, Paul

------------------------------------------------------------------------

commit 122acec803471468d8a453d08219ca2fc94f5556
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Wed Apr 12 15:29:14 2017 -0700

    rcu: Complain if blocking in preemptible RCU read-side critical section
    
    Although preemptible RCU allows its read-side critical sections to be
    preempted, general blocking is forbidden.  The reason for this is that
    excessive preemption times can be handled by CONFIG_RCU_BOOST=y, but a
    voluntarily blocked task doesn't care how high you boost its priority.
    Because preemptible RCU is a global mechanism, one ill-behaved reader
    hurts everyone.  Hence the prohibition against general blocking in
    RCU-preempt read-side critical sections.  Preemption yes, blocking no.
    
    This commit enforces this prohibition.
    
    There is a special exception for the -rt patchset (which they kindly
    volunteered to implement):  It is OK to block (as opposed to merely being
    preempted) within an RCU-preempt read-side critical section, but only if
    the blocking is subject to priority inheritance.  This exception permits
    CONFIG_RCU_BOOST=y to get -rt RCU readers out of trouble.
    
    Why doesn't this exception also apply to mainline's rt_mutex?  Because
    of the possibility that someone does general blocking while holding
    an rt_mutex.  Yes, the priority boosting will affect the rt_mutex,
    but it won't help with the task doing general blocking while holding
    that rt_mutex.
    
    Reported-by: Thomas Gleixner <tglx@linutronix.de>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d013bd4767a7..abc09d368b3a 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -465,7 +465,7 @@ void rcu_note_context_switch(bool preempt)
 	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
-	rcu_preempt_note_context_switch();
+	rcu_preempt_note_context_switch(preempt);
 	/* Load rcu_urgent_qs before other flags. */
 	if (!smp_load_acquire(this_cpu_ptr(&rcu_dynticks.rcu_urgent_qs)))
 		goto out;
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 0e598ab08fea..781fe684f230 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -476,7 +476,7 @@ DECLARE_PER_CPU(char, rcu_cpu_has_work);
 
 /* Forward declarations for rcutree_plugin.h */
 static void rcu_bootup_announce(void);
-static void rcu_preempt_note_context_switch(void);
+static void rcu_preempt_note_context_switch(bool preempt);
 static int rcu_preempt_blocked_readers_cgp(struct rcu_node *rnp);
 #ifdef CONFIG_HOTPLUG_CPU
 static bool rcu_preempt_has_tasks(struct rcu_node *rnp);
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 6d8f7f82259c..67a90158f32e 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -286,12 +286,13 @@ static void rcu_preempt_qs(void)
  *
  * Caller must disable interrupts.
  */
-static void rcu_preempt_note_context_switch(void)
+static void rcu_preempt_note_context_switch(bool preempt)
 {
 	struct task_struct *t = current;
 	struct rcu_data *rdp;
 	struct rcu_node *rnp;
 
+	WARN_ON_ONCE(!preempt && t->rcu_read_lock_nesting > 0);
 	if (t->rcu_read_lock_nesting > 0 &&
 	    !t->rcu_read_unlock_special.b.blocked) {
 
@@ -738,7 +739,7 @@ static void __init rcu_bootup_announce(void)
  * Because preemptible RCU does not exist, we never have to check for
  * CPUs being in quiescent states.
  */
-static void rcu_preempt_note_context_switch(void)
+static void rcu_preempt_note_context_switch(bool preempt)
 {
 }
 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

WARNING: multiple messages have this Message-ID (diff)
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@suse.de>,
	"Verma, Vishal L" <vishal.l.verma@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@ml01.01.org>,
	"Luck, Tony" <tony.luck@intel.com>,
	"ross.zwisler@linux.intel.com" <ross.zwisler@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [RFC PATCH] x86, mce: change the mce notifier to 'blocking' from 'atomic'
Date: Wed, 12 Apr 2017 15:42:32 -0700	[thread overview]
Message-ID: <20170412224232.GM3956@linux.vnet.ibm.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1704122342130.2548@nanos>

On Wed, Apr 12, 2017 at 11:50:45PM +0200, Thomas Gleixner wrote:
> On Wed, 12 Apr 2017, Borislav Petkov wrote:
> 
> > On Wed, Apr 12, 2017 at 08:27:05PM +0000, Verma, Vishal L wrote:
> > > But isn't the atomic notifier call chain always called in atomic
> > > context?
> > 
> > No, it isn't. We're calling it in normal process context in
> > mce_gen_pool_process() too.
> > 
> > So this early exit will avoid any sleeping in atomic context. And since
> > there's nothing you can do about the errors reported in atomic context,
> > we can actually use that fact.
> 
> No, you can't.
> 
> CONFIG_RCU_PREEMPT=n + CONFIG_PREEMPT_COUNT will disable preemption from
> within __atomic_notifier_call_chain() via rcu_read_lock(). Ergo you wont
> ever enter the handler.
> 
> The behaviour in the RCU code is inconsistent. CONFIG_RCU_PREEMPT=y does
> obviouly not disable preemption, but it should still trigger the
> might_sleep() check when a blocking function is called from within a rcu
> read side critical section.

Maybe something like the (untested) patch below.  Please note that this
would need some help to work correctly in -rt.  This applies only against
-rcu tip, but in that case you can just get it directly from -rcu.

							Thanx, Paul

------------------------------------------------------------------------

commit 122acec803471468d8a453d08219ca2fc94f5556
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Wed Apr 12 15:29:14 2017 -0700

    rcu: Complain if blocking in preemptible RCU read-side critical section
    
    Although preemptible RCU allows its read-side critical sections to be
    preempted, general blocking is forbidden.  The reason for this is that
    excessive preemption times can be handled by CONFIG_RCU_BOOST=y, but a
    voluntarily blocked task doesn't care how high you boost its priority.
    Because preemptible RCU is a global mechanism, one ill-behaved reader
    hurts everyone.  Hence the prohibition against general blocking in
    RCU-preempt read-side critical sections.  Preemption yes, blocking no.
    
    This commit enforces this prohibition.
    
    There is a special exception for the -rt patchset (which they kindly
    volunteered to implement):  It is OK to block (as opposed to merely being
    preempted) within an RCU-preempt read-side critical section, but only if
    the blocking is subject to priority inheritance.  This exception permits
    CONFIG_RCU_BOOST=y to get -rt RCU readers out of trouble.
    
    Why doesn't this exception also apply to mainline's rt_mutex?  Because
    of the possibility that someone does general blocking while holding
    an rt_mutex.  Yes, the priority boosting will affect the rt_mutex,
    but it won't help with the task doing general blocking while holding
    that rt_mutex.
    
    Reported-by: Thomas Gleixner <tglx@linutronix.de>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d013bd4767a7..abc09d368b3a 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -465,7 +465,7 @@ void rcu_note_context_switch(bool preempt)
 	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
-	rcu_preempt_note_context_switch();
+	rcu_preempt_note_context_switch(preempt);
 	/* Load rcu_urgent_qs before other flags. */
 	if (!smp_load_acquire(this_cpu_ptr(&rcu_dynticks.rcu_urgent_qs)))
 		goto out;
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 0e598ab08fea..781fe684f230 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -476,7 +476,7 @@ DECLARE_PER_CPU(char, rcu_cpu_has_work);
 
 /* Forward declarations for rcutree_plugin.h */
 static void rcu_bootup_announce(void);
-static void rcu_preempt_note_context_switch(void);
+static void rcu_preempt_note_context_switch(bool preempt);
 static int rcu_preempt_blocked_readers_cgp(struct rcu_node *rnp);
 #ifdef CONFIG_HOTPLUG_CPU
 static bool rcu_preempt_has_tasks(struct rcu_node *rnp);
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 6d8f7f82259c..67a90158f32e 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -286,12 +286,13 @@ static void rcu_preempt_qs(void)
  *
  * Caller must disable interrupts.
  */
-static void rcu_preempt_note_context_switch(void)
+static void rcu_preempt_note_context_switch(bool preempt)
 {
 	struct task_struct *t = current;
 	struct rcu_data *rdp;
 	struct rcu_node *rnp;
 
+	WARN_ON_ONCE(!preempt && t->rcu_read_lock_nesting > 0);
 	if (t->rcu_read_lock_nesting > 0 &&
 	    !t->rcu_read_unlock_special.b.blocked) {
 
@@ -738,7 +739,7 @@ static void __init rcu_bootup_announce(void)
  * Because preemptible RCU does not exist, we never have to check for
  * CPUs being in quiescent states.
  */
-static void rcu_preempt_note_context_switch(void)
+static void rcu_preempt_note_context_switch(bool preempt)
 {
 }
 

  reply	other threads:[~2017-04-12 22:42 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-11 22:44 [RFC PATCH] x86, mce: change the mce notifier to 'blocking' from 'atomic' Vishal Verma
2017-04-11 22:44 ` Vishal Verma
2017-04-12  9:14 ` Borislav Petkov
2017-04-12  9:14   ` Borislav Petkov
2017-04-12 19:59   ` Vishal Verma
2017-04-12 19:59     ` Vishal Verma
2017-04-12 20:22     ` Borislav Petkov
2017-04-12 20:22       ` Borislav Petkov
2017-04-12 20:27       ` Verma, Vishal L
2017-04-12 20:27         ` Verma, Vishal L
2017-04-12 20:52         ` Luck, Tony
2017-04-12 20:52           ` Luck, Tony
2017-04-12 20:55           ` Dan Williams
2017-04-12 20:55             ` Dan Williams
2017-04-12 21:12             ` Thomas Gleixner
2017-04-12 21:12               ` Thomas Gleixner
2017-04-12 21:19               ` Luck, Tony
2017-04-12 21:19                 ` Luck, Tony
2017-04-12 21:47                 ` Borislav Petkov
2017-04-12 21:47                   ` Borislav Petkov
2017-04-12 22:16                   ` Borislav Petkov
2017-04-12 22:16                     ` Borislav Petkov
2017-04-12 22:26                     ` Luck, Tony
2017-04-12 22:26                       ` Luck, Tony
2017-04-12 22:29                       ` Borislav Petkov
2017-04-12 22:29                         ` Borislav Petkov
2017-04-13 11:31                         ` Borislav Petkov
2017-04-13 11:31                           ` Borislav Petkov
2017-04-13 12:12                           ` Borislav Petkov
2017-04-13 12:12                             ` Borislav Petkov
2017-04-18 16:28                             ` Luck, Tony
2017-04-18 16:28                               ` Luck, Tony
     [not found]                           ` <20170413113159.rc32ebiswn64nzrr-fF5Pk5pvG8Y@public.gmane.org>
2017-04-21 21:39                             ` Verma, Vishal L
2017-04-21 21:39                               ` Verma, Vishal L
2017-04-12 21:13         ` Borislav Petkov
2017-04-12 21:13           ` Borislav Petkov
2017-04-12 21:50           ` Thomas Gleixner
2017-04-12 21:50             ` Thomas Gleixner
2017-04-12 22:42             ` Paul E. McKenney [this message]
2017-04-12 22:42               ` Paul E. McKenney
2017-04-12 23:45               ` Paul E. McKenney
2017-04-12 23:45                 ` Paul E. McKenney
2017-04-13 14:34                 ` Paul E. McKenney
2017-04-13 14:34                   ` Paul E. McKenney
  -- strict thread matches above, loose matches on Subject: below --
2017-04-18 20:27 [tip:ras/urgent] x86/mce: Make the MCE notifier a blocking one tip-bot for Borislav Petkov
2017-04-18 20:27 ` tip-bot for Vishal Verma

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=20170412224232.GM3956@linux.vnet.ibm.com \
    --to=paulmck@linux.vnet.ibm.com \
    --cc=bp@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@ml01.01.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    /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.