[PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Srivatsa S. Bhat]

While booting, the following message is seen:

[   21.665087] ===============================
[   21.669439] [ INFO: suspicious RCU usage. ]
[   21.673798] 3.2.0-0.0.0.28.36b5ec9-default #2 Not tainted
[   21.681353] -------------------------------
[   21.685864] arch/x86/kernel/cpu/mcheck/mce.c:194 suspicious rcu_dereference_index_check() usage!
[   21.695013]
[   21.695014] other info that might help us debug this:
[   21.695016]
[   21.703488]
[   21.703489] rcu_scheduler_active = 1, debug_locks = 1
[   21.710426] 3 locks held by modprobe/2139:
[   21.714754]  #0:  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afd3>] __driver_attach+0x53/0xa0
[   21.725020]  #1:
[   21.725323] ioatdma: Intel(R) QuickData Technology Driver 4.00
[   21.733206]  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afe1>] __driver_attach+0x61/0xa0
[   21.743015]  #2:  (i7core_edac_lock){+.+.+.}, at: [<ffffffffa01cfa5f>] i7core_probe+0x1f/0x5c0 [i7core_edac]
[   21.753708]
[   21.753709] stack backtrace:
[   21.758429] Pid: 2139, comm: modprobe Not tainted 3.2.0-0.0.0.28.36b5ec9-default #2
[   21.768253] Call Trace:
[   21.770838]  [<ffffffff810977cd>] lockdep_rcu_suspicious+0xcd/0x100
[   21.777366]  [<ffffffff8101aa41>] drain_mcelog_buffer+0x191/0x1b0
[   21.783715]  [<ffffffff8101aa78>] mce_register_decode_chain+0x18/0x20
[   21.790430]  [<ffffffffa01cf8db>] i7core_register_mci+0x2fb/0x3e4 [i7core_edac]
[   21.798003]  [<ffffffffa01cfb14>] i7core_probe+0xd4/0x5c0 [i7core_edac]
[   21.804809]  [<ffffffff8129566b>] local_pci_probe+0x5b/0xe0
[   21.810631]  [<ffffffff812957c9>] __pci_device_probe+0xd9/0xe0
[   21.816650]  [<ffffffff813362e4>] ? get_device+0x14/0x20
[   21.822178]  [<ffffffff81296916>] pci_device_probe+0x36/0x60
[   21.828061]  [<ffffffff8133ac8a>] really_probe+0x7a/0x2b0
[   21.833676]  [<ffffffff8133af23>] driver_probe_device+0x63/0xc0
[   21.839868]  [<ffffffff8133b01b>] __driver_attach+0x9b/0xa0
[   21.845718]  [<ffffffff8133af80>] ? driver_probe_device+0xc0/0xc0
[   21.852027]  [<ffffffff81339168>] bus_for_each_dev+0x68/0x90
[   21.857876]  [<ffffffff8133aa3c>] driver_attach+0x1c/0x20
[   21.863462]  [<ffffffff8133a64d>] bus_add_driver+0x16d/0x2b0
[   21.869377]  [<ffffffff8133b6dc>] driver_register+0x7c/0x160
[   21.875220]  [<ffffffff81296bda>] __pci_register_driver+0x6a/0xf0
[   21.881494]  [<ffffffffa01fe000>] ? 0xffffffffa01fdfff
[   21.886846]  [<ffffffffa01fe047>] i7core_init+0x47/0x1000 [i7core_edac]
[   21.893737]  [<ffffffff810001ce>] do_one_initcall+0x3e/0x180
[   21.899670]  [<ffffffff810a9b95>] sys_init_module+0xc5/0x220
[   21.905542]  [<ffffffff8149bc39>] system_call_fastpath+0x16/0x1b

Fix this by using rcu_access_index() instead of rcu_dereference_check_mce().
This is similar to what was done in commit a4dd992 (rcu: create new
rcu_access_index() and use in mce) for a similar case in mce_poll.
Link to that discussion: http://thread.gmane.org/gmane.linux.kernel/1058460/

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---
I am no MCE expert. Hence requesting a thorough review of the patch.

 arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index f22a9f7..c191fec 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -191,7 +191,7 @@ static void drain_mcelog_buffer(void)
 {
 	unsigned int next, i, prev = 0;
 
-	next = rcu_dereference_check_mce(mcelog.next);
+	next = rcu_access_index(mcelog.next);
 
 	do {
 		struct mce *m;

Re: [PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Paul E. McKenney]

On Wed, Jan 11, 2012 at 07:54:48PM +0530, Srivatsa S. Bhat wrote:
> While booting, the following message is seen:

Hmmm...  This code is interesting.  It looks to me that access to
each entry is gated by the ->finished field.  If so, there should be
no need for any kind of rcu_dereference() on mcelog.next -- ACCESS_ONCE()
should work just fine.

> [   21.665087] ===============================
> [   21.669439] [ INFO: suspicious RCU usage. ]
> [   21.673798] 3.2.0-0.0.0.28.36b5ec9-default #2 Not tainted
> [   21.681353] -------------------------------
> [   21.685864] arch/x86/kernel/cpu/mcheck/mce.c:194 suspicious rcu_dereference_index_check() usage!
> [   21.695013]
> [   21.695014] other info that might help us debug this:
> [   21.695016]
> [   21.703488]
> [   21.703489] rcu_scheduler_active = 1, debug_locks = 1
> [   21.710426] 3 locks held by modprobe/2139:
> [   21.714754]  #0:  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afd3>] __driver_attach+0x53/0xa0
> [   21.725020]  #1:
> [   21.725323] ioatdma: Intel(R) QuickData Technology Driver 4.00
> [   21.733206]  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afe1>] __driver_attach+0x61/0xa0
> [   21.743015]  #2:  (i7core_edac_lock){+.+.+.}, at: [<ffffffffa01cfa5f>] i7core_probe+0x1f/0x5c0 [i7core_edac]
> [   21.753708]
> [   21.753709] stack backtrace:
> [   21.758429] Pid: 2139, comm: modprobe Not tainted 3.2.0-0.0.0.28.36b5ec9-default #2
> [   21.768253] Call Trace:
> [   21.770838]  [<ffffffff810977cd>] lockdep_rcu_suspicious+0xcd/0x100
> [   21.777366]  [<ffffffff8101aa41>] drain_mcelog_buffer+0x191/0x1b0
> [   21.783715]  [<ffffffff8101aa78>] mce_register_decode_chain+0x18/0x20
> [   21.790430]  [<ffffffffa01cf8db>] i7core_register_mci+0x2fb/0x3e4 [i7core_edac]
> [   21.798003]  [<ffffffffa01cfb14>] i7core_probe+0xd4/0x5c0 [i7core_edac]
> [   21.804809]  [<ffffffff8129566b>] local_pci_probe+0x5b/0xe0
> [   21.810631]  [<ffffffff812957c9>] __pci_device_probe+0xd9/0xe0
> [   21.816650]  [<ffffffff813362e4>] ? get_device+0x14/0x20
> [   21.822178]  [<ffffffff81296916>] pci_device_probe+0x36/0x60
> [   21.828061]  [<ffffffff8133ac8a>] really_probe+0x7a/0x2b0
> [   21.833676]  [<ffffffff8133af23>] driver_probe_device+0x63/0xc0
> [   21.839868]  [<ffffffff8133b01b>] __driver_attach+0x9b/0xa0
> [   21.845718]  [<ffffffff8133af80>] ? driver_probe_device+0xc0/0xc0
> [   21.852027]  [<ffffffff81339168>] bus_for_each_dev+0x68/0x90
> [   21.857876]  [<ffffffff8133aa3c>] driver_attach+0x1c/0x20
> [   21.863462]  [<ffffffff8133a64d>] bus_add_driver+0x16d/0x2b0
> [   21.869377]  [<ffffffff8133b6dc>] driver_register+0x7c/0x160
> [   21.875220]  [<ffffffff81296bda>] __pci_register_driver+0x6a/0xf0
> [   21.881494]  [<ffffffffa01fe000>] ? 0xffffffffa01fdfff
> [   21.886846]  [<ffffffffa01fe047>] i7core_init+0x47/0x1000 [i7core_edac]
> [   21.893737]  [<ffffffff810001ce>] do_one_initcall+0x3e/0x180
> [   21.899670]  [<ffffffff810a9b95>] sys_init_module+0xc5/0x220
> [   21.905542]  [<ffffffff8149bc39>] system_call_fastpath+0x16/0x1b
> 
> Fix this by using rcu_access_index() instead of rcu_dereference_check_mce().
> This is similar to what was done in commit a4dd992 (rcu: create new
> rcu_access_index() and use in mce) for a similar case in mce_poll.
> Link to that discussion: http://thread.gmane.org/gmane.linux.kernel/1058460/
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> I am no MCE expert. Hence requesting a thorough review of the patch.
> 
>  arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index f22a9f7..c191fec 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -191,7 +191,7 @@ static void drain_mcelog_buffer(void)
>  {
>  	unsigned int next, i, prev = 0;
> 
> -	next = rcu_dereference_check_mce(mcelog.next);
> +	next = rcu_access_index(mcelog.next);

So this is not so much a use of RCU as it is a standard mailbox algorithm,
with the ->finished flag controlling access.  So I suggest ACCESS_ONCE()
for the accesses to mcelog.next.

							Thanx, Paul

>  	do {
>  		struct mce *m;
> 

Re: [PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Srivatsa S. Bhat]

On 01/11/2012 09:41 PM, Paul E. McKenney wrote:

> On Wed, Jan 11, 2012 at 07:54:48PM +0530, Srivatsa S. Bhat wrote:
>> While booting, the following message is seen:
> 
> Hmmm...  This code is interesting.  It looks to me that access to
> each entry is gated by the ->finished field.  If so, there should be
> no need for any kind of rcu_dereference() on mcelog.next -- ACCESS_ONCE()
> should work just fine.
> 

[snip]

>>
>> -	next = rcu_dereference_check_mce(mcelog.next);
>> +	next = rcu_access_index(mcelog.next);
> 
> So this is not so much a use of RCU as it is a standard mailbox algorithm,
> with the ->finished flag controlling access.  So I suggest ACCESS_ONCE()
> for the accesses to mcelog.next.
> 


Hi Paul,
Thank you for the suggestion. Here is an updated patch:

------
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH v2] x86, mce: Fix rcu splat in drain_mce_log_buffer()

While booting, the following message is seen:

[   21.665087] ===============================
[   21.669439] [ INFO: suspicious RCU usage. ]
[   21.673798] 3.2.0-0.0.0.28.36b5ec9-default #2 Not tainted
[   21.681353] -------------------------------
[   21.685864] arch/x86/kernel/cpu/mcheck/mce.c:194 suspicious rcu_dereference_index_check() usage!
[   21.695013]
[   21.695014] other info that might help us debug this:
[   21.695016]
[   21.703488]
[   21.703489] rcu_scheduler_active = 1, debug_locks = 1
[   21.710426] 3 locks held by modprobe/2139:
[   21.714754]  #0:  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afd3>] __driver_attach+0x53/0xa0
[   21.725020]  #1:
[   21.725323] ioatdma: Intel(R) QuickData Technology Driver 4.00
[   21.733206]  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afe1>] __driver_attach+0x61/0xa0
[   21.743015]  #2:  (i7core_edac_lock){+.+.+.}, at: [<ffffffffa01cfa5f>] i7core_probe+0x1f/0x5c0 [i7core_edac]
[   21.753708]
[   21.753709] stack backtrace:
[   21.758429] Pid: 2139, comm: modprobe Not tainted 3.2.0-0.0.0.28.36b5ec9-default #2
[   21.768253] Call Trace:
[   21.770838]  [<ffffffff810977cd>] lockdep_rcu_suspicious+0xcd/0x100
[   21.777366]  [<ffffffff8101aa41>] drain_mcelog_buffer+0x191/0x1b0
[   21.783715]  [<ffffffff8101aa78>] mce_register_decode_chain+0x18/0x20
[   21.790430]  [<ffffffffa01cf8db>] i7core_register_mci+0x2fb/0x3e4 [i7core_edac]
[   21.798003]  [<ffffffffa01cfb14>] i7core_probe+0xd4/0x5c0 [i7core_edac]
[   21.804809]  [<ffffffff8129566b>] local_pci_probe+0x5b/0xe0
[   21.810631]  [<ffffffff812957c9>] __pci_device_probe+0xd9/0xe0
[   21.816650]  [<ffffffff813362e4>] ? get_device+0x14/0x20
[   21.822178]  [<ffffffff81296916>] pci_device_probe+0x36/0x60
[   21.828061]  [<ffffffff8133ac8a>] really_probe+0x7a/0x2b0
[   21.833676]  [<ffffffff8133af23>] driver_probe_device+0x63/0xc0
[   21.839868]  [<ffffffff8133b01b>] __driver_attach+0x9b/0xa0
[   21.845718]  [<ffffffff8133af80>] ? driver_probe_device+0xc0/0xc0
[   21.852027]  [<ffffffff81339168>] bus_for_each_dev+0x68/0x90
[   21.857876]  [<ffffffff8133aa3c>] driver_attach+0x1c/0x20
[   21.863462]  [<ffffffff8133a64d>] bus_add_driver+0x16d/0x2b0
[   21.869377]  [<ffffffff8133b6dc>] driver_register+0x7c/0x160
[   21.875220]  [<ffffffff81296bda>] __pci_register_driver+0x6a/0xf0
[   21.881494]  [<ffffffffa01fe000>] ? 0xffffffffa01fdfff
[   21.886846]  [<ffffffffa01fe047>] i7core_init+0x47/0x1000 [i7core_edac]
[   21.893737]  [<ffffffff810001ce>] do_one_initcall+0x3e/0x180
[   21.899670]  [<ffffffff810a9b95>] sys_init_module+0xc5/0x220
[   21.905542]  [<ffffffff8149bc39>] system_call_fastpath+0x16/0x1b

Fix this by using ACCESS_ONCE() instead of rcu_dereference_check_mce()
over mcelog.next. Since the access to each entry is controlled by the
->finished field, ACCESS_ONCE() should work just fine. An rcu_dereference
is unnecessary here.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index f22a9f7..f525f99 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -191,7 +191,7 @@ static void drain_mcelog_buffer(void)
 {
 	unsigned int next, i, prev = 0;
 
-	next = rcu_dereference_check_mce(mcelog.next);
+	next = ACCESS_ONCE(mcelog.next);
 
 	do {
 		struct mce *m;

Re: [PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Paul E. McKenney]

On Fri, Jan 13, 2012 at 05:58:03PM +0530, Srivatsa S. Bhat wrote:
> On 01/11/2012 09:41 PM, Paul E. McKenney wrote:
> 
> > On Wed, Jan 11, 2012 at 07:54:48PM +0530, Srivatsa S. Bhat wrote:
> >> While booting, the following message is seen:
> > 
> > Hmmm...  This code is interesting.  It looks to me that access to
> > each entry is gated by the ->finished field.  If so, there should be
> > no need for any kind of rcu_dereference() on mcelog.next -- ACCESS_ONCE()
> > should work just fine.
> > 
> 
> [snip]
> 
> >>
> >> -	next = rcu_dereference_check_mce(mcelog.next);
> >> +	next = rcu_access_index(mcelog.next);
> > 
> > So this is not so much a use of RCU as it is a standard mailbox algorithm,
> > with the ->finished flag controlling access.  So I suggest ACCESS_ONCE()
> > for the accesses to mcelog.next.
> > 
> 
> 
> Hi Paul,
> Thank you for the suggestion. Here is an updated patch:

Looks good to me, but I do need to defer to people who know this code
better than do I.  The key thing that (from what I can see) makes
rcu_dereference() unnecessary is that the smp_rmb() used in conjunction
with polling the .finished field takes care of ordering.

Note that this also relies on the fact that mcelog is statically
allocated.  If it was dynamically allocated, then the code would need
to wait a grace period between allocating it an initializing it.

							Thanx, Paul

> ------
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH v2] x86, mce: Fix rcu splat in drain_mce_log_buffer()
> 
> While booting, the following message is seen:
> 
> [   21.665087] ===============================
> [   21.669439] [ INFO: suspicious RCU usage. ]
> [   21.673798] 3.2.0-0.0.0.28.36b5ec9-default #2 Not tainted
> [   21.681353] -------------------------------
> [   21.685864] arch/x86/kernel/cpu/mcheck/mce.c:194 suspicious rcu_dereference_index_check() usage!
> [   21.695013]
> [   21.695014] other info that might help us debug this:
> [   21.695016]
> [   21.703488]
> [   21.703489] rcu_scheduler_active = 1, debug_locks = 1
> [   21.710426] 3 locks held by modprobe/2139:
> [   21.714754]  #0:  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afd3>] __driver_attach+0x53/0xa0
> [   21.725020]  #1:
> [   21.725323] ioatdma: Intel(R) QuickData Technology Driver 4.00
> [   21.733206]  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afe1>] __driver_attach+0x61/0xa0
> [   21.743015]  #2:  (i7core_edac_lock){+.+.+.}, at: [<ffffffffa01cfa5f>] i7core_probe+0x1f/0x5c0 [i7core_edac]
> [   21.753708]
> [   21.753709] stack backtrace:
> [   21.758429] Pid: 2139, comm: modprobe Not tainted 3.2.0-0.0.0.28.36b5ec9-default #2
> [   21.768253] Call Trace:
> [   21.770838]  [<ffffffff810977cd>] lockdep_rcu_suspicious+0xcd/0x100
> [   21.777366]  [<ffffffff8101aa41>] drain_mcelog_buffer+0x191/0x1b0
> [   21.783715]  [<ffffffff8101aa78>] mce_register_decode_chain+0x18/0x20
> [   21.790430]  [<ffffffffa01cf8db>] i7core_register_mci+0x2fb/0x3e4 [i7core_edac]
> [   21.798003]  [<ffffffffa01cfb14>] i7core_probe+0xd4/0x5c0 [i7core_edac]
> [   21.804809]  [<ffffffff8129566b>] local_pci_probe+0x5b/0xe0
> [   21.810631]  [<ffffffff812957c9>] __pci_device_probe+0xd9/0xe0
> [   21.816650]  [<ffffffff813362e4>] ? get_device+0x14/0x20
> [   21.822178]  [<ffffffff81296916>] pci_device_probe+0x36/0x60
> [   21.828061]  [<ffffffff8133ac8a>] really_probe+0x7a/0x2b0
> [   21.833676]  [<ffffffff8133af23>] driver_probe_device+0x63/0xc0
> [   21.839868]  [<ffffffff8133b01b>] __driver_attach+0x9b/0xa0
> [   21.845718]  [<ffffffff8133af80>] ? driver_probe_device+0xc0/0xc0
> [   21.852027]  [<ffffffff81339168>] bus_for_each_dev+0x68/0x90
> [   21.857876]  [<ffffffff8133aa3c>] driver_attach+0x1c/0x20
> [   21.863462]  [<ffffffff8133a64d>] bus_add_driver+0x16d/0x2b0
> [   21.869377]  [<ffffffff8133b6dc>] driver_register+0x7c/0x160
> [   21.875220]  [<ffffffff81296bda>] __pci_register_driver+0x6a/0xf0
> [   21.881494]  [<ffffffffa01fe000>] ? 0xffffffffa01fdfff
> [   21.886846]  [<ffffffffa01fe047>] i7core_init+0x47/0x1000 [i7core_edac]
> [   21.893737]  [<ffffffff810001ce>] do_one_initcall+0x3e/0x180
> [   21.899670]  [<ffffffff810a9b95>] sys_init_module+0xc5/0x220
> [   21.905542]  [<ffffffff8149bc39>] system_call_fastpath+0x16/0x1b
> 
> Fix this by using ACCESS_ONCE() instead of rcu_dereference_check_mce()
> over mcelog.next. Since the access to each entry is controlled by the
> ->finished field, ACCESS_ONCE() should work just fine. An rcu_dereference
> is unnecessary here.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index f22a9f7..f525f99 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -191,7 +191,7 @@ static void drain_mcelog_buffer(void)
>  {
>  	unsigned int next, i, prev = 0;
> 
> -	next = rcu_dereference_check_mce(mcelog.next);
> +	next = ACCESS_ONCE(mcelog.next);
> 
>  	do {
>  		struct mce *m;
> 
> 

Re: [PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Srivatsa S. Bhat]

On 01/14/2012 03:14 AM, Paul E. McKenney wrote:

> On Fri, Jan 13, 2012 at 05:58:03PM +0530, Srivatsa S. Bhat wrote:
>> On 01/11/2012 09:41 PM, Paul E. McKenney wrote:
>>
>>> On Wed, Jan 11, 2012 at 07:54:48PM +0530, Srivatsa S. Bhat wrote:
>>>> While booting, the following message is seen:
>>>
>>> Hmmm...  This code is interesting.  It looks to me that access to
>>> each entry is gated by the ->finished field.  If so, there should be
>>> no need for any kind of rcu_dereference() on mcelog.next -- ACCESS_ONCE()
>>> should work just fine.
>>>
>>
>> [snip]
>>
>>>>
>>>> -	next = rcu_dereference_check_mce(mcelog.next);
>>>> +	next = rcu_access_index(mcelog.next);
>>>
>>> So this is not so much a use of RCU as it is a standard mailbox algorithm,
>>> with the ->finished flag controlling access.  So I suggest ACCESS_ONCE()
>>> for the accesses to mcelog.next.
>>>
>>
>>
>> Hi Paul,
>> Thank you for the suggestion. Here is an updated patch:
> 
> Looks good to me, but I do need to defer to people who know this code
> better than do I.  The key thing that (from what I can see) makes
> rcu_dereference() unnecessary is that the smp_rmb() used in conjunction
> with polling the .finished field takes care of ordering.
> 
> Note that this also relies on the fact that mcelog is statically
> allocated.  If it was dynamically allocated, then the code would need
> to wait a grace period between allocating it an initializing it.
> 
> 							Thanx, Paul
>


Ping?

Regards,
Srivatsa S. Bhat

 
>> ------
>> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Subject: [PATCH v2] x86, mce: Fix rcu splat in drain_mce_log_buffer()
>>
>> While booting, the following message is seen:
>>
>> [   21.665087] ===============================
>> [   21.669439] [ INFO: suspicious RCU usage. ]
>> [   21.673798] 3.2.0-0.0.0.28.36b5ec9-default #2 Not tainted
>> [   21.681353] -------------------------------
>> [   21.685864] arch/x86/kernel/cpu/mcheck/mce.c:194 suspicious rcu_dereference_index_check() usage!
>> [   21.695013]
>> [   21.695014] other info that might help us debug this:
>> [   21.695016]
>> [   21.703488]
>> [   21.703489] rcu_scheduler_active = 1, debug_locks = 1
>> [   21.710426] 3 locks held by modprobe/2139:
>> [   21.714754]  #0:  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afd3>] __driver_attach+0x53/0xa0
>> [   21.725020]  #1:
>> [   21.725323] ioatdma: Intel(R) QuickData Technology Driver 4.00
>> [   21.733206]  (&__lockdep_no_validate__){......}, at: [<ffffffff8133afe1>] __driver_attach+0x61/0xa0
>> [   21.743015]  #2:  (i7core_edac_lock){+.+.+.}, at: [<ffffffffa01cfa5f>] i7core_probe+0x1f/0x5c0 [i7core_edac]
>> [   21.753708]
>> [   21.753709] stack backtrace:
>> [   21.758429] Pid: 2139, comm: modprobe Not tainted 3.2.0-0.0.0.28.36b5ec9-default #2
>> [   21.768253] Call Trace:
>> [   21.770838]  [<ffffffff810977cd>] lockdep_rcu_suspicious+0xcd/0x100
>> [   21.777366]  [<ffffffff8101aa41>] drain_mcelog_buffer+0x191/0x1b0
>> [   21.783715]  [<ffffffff8101aa78>] mce_register_decode_chain+0x18/0x20
>> [   21.790430]  [<ffffffffa01cf8db>] i7core_register_mci+0x2fb/0x3e4 [i7core_edac]
>> [   21.798003]  [<ffffffffa01cfb14>] i7core_probe+0xd4/0x5c0 [i7core_edac]
>> [   21.804809]  [<ffffffff8129566b>] local_pci_probe+0x5b/0xe0
>> [   21.810631]  [<ffffffff812957c9>] __pci_device_probe+0xd9/0xe0
>> [   21.816650]  [<ffffffff813362e4>] ? get_device+0x14/0x20
>> [   21.822178]  [<ffffffff81296916>] pci_device_probe+0x36/0x60
>> [   21.828061]  [<ffffffff8133ac8a>] really_probe+0x7a/0x2b0
>> [   21.833676]  [<ffffffff8133af23>] driver_probe_device+0x63/0xc0
>> [   21.839868]  [<ffffffff8133b01b>] __driver_attach+0x9b/0xa0
>> [   21.845718]  [<ffffffff8133af80>] ? driver_probe_device+0xc0/0xc0
>> [   21.852027]  [<ffffffff81339168>] bus_for_each_dev+0x68/0x90
>> [   21.857876]  [<ffffffff8133aa3c>] driver_attach+0x1c/0x20
>> [   21.863462]  [<ffffffff8133a64d>] bus_add_driver+0x16d/0x2b0
>> [   21.869377]  [<ffffffff8133b6dc>] driver_register+0x7c/0x160
>> [   21.875220]  [<ffffffff81296bda>] __pci_register_driver+0x6a/0xf0
>> [   21.881494]  [<ffffffffa01fe000>] ? 0xffffffffa01fdfff
>> [   21.886846]  [<ffffffffa01fe047>] i7core_init+0x47/0x1000 [i7core_edac]
>> [   21.893737]  [<ffffffff810001ce>] do_one_initcall+0x3e/0x180
>> [   21.899670]  [<ffffffff810a9b95>] sys_init_module+0xc5/0x220
>> [   21.905542]  [<ffffffff8149bc39>] system_call_fastpath+0x16/0x1b
>>
>> Fix this by using ACCESS_ONCE() instead of rcu_dereference_check_mce()
>> over mcelog.next. Since the access to each entry is controlled by the
>> ->finished field, ACCESS_ONCE() should work just fine. An rcu_dereference
>> is unnecessary here.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>>  arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> index f22a9f7..f525f99 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -191,7 +191,7 @@ static void drain_mcelog_buffer(void)
>>  {
>>  	unsigned int next, i, prev = 0;
>>
>> -	next = rcu_dereference_check_mce(mcelog.next);
>> +	next = ACCESS_ONCE(mcelog.next);
>>
>>  	do {
>>  		struct mce *m;
>>
>>

Re: [PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Borislav Petkov]

On Fri, Jan 13, 2012 at 01:44:35PM -0800, Paul E. McKenney wrote:
> Looks good to me, but I do need to defer to people who know this code
> better than do I.  The key thing that (from what I can see) makes
> rcu_dereference() unnecessary is that the smp_rmb() used in conjunction
> with polling the .finished field takes care of ordering.

Right, this was me trying hard not to screw up touching mcelog.next,
thus trying to use the rcu_dereference_index_check() primitive without
thinking it through too much. But you're right, I'm polling the
->finished field 4 times (totally arbitrary, btw) which should suffice
while the mce_log() routine above writes those entries.

Although, the question still remains, since mce_log() accesses
mcelog.next through the rcu_dereference_index_check() primitive,
shouldn't I do it the same way?

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Paul E. McKenney]

On Mon, Feb 06, 2012 at 05:18:02PM +0100, Borislav Petkov wrote:
> On Fri, Jan 13, 2012 at 01:44:35PM -0800, Paul E. McKenney wrote:
> > Looks good to me, but I do need to defer to people who know this code
> > better than do I.  The key thing that (from what I can see) makes
> > rcu_dereference() unnecessary is that the smp_rmb() used in conjunction
> > with polling the .finished field takes care of ordering.
> 
> Right, this was me trying hard not to screw up touching mcelog.next,
> thus trying to use the rcu_dereference_index_check() primitive without
> thinking it through too much. But you're right, I'm polling the
> ->finished field 4 times (totally arbitrary, btw) which should suffice
> while the mce_log() routine above writes those entries.
> 
> Although, the question still remains, since mce_log() accesses
> mcelog.next through the rcu_dereference_index_check() primitive,
> shouldn't I do it the same way?

I don't claim to be an mce_log() expert, but when I looked it over,
I didn't see a need for rcu_dereference_index_check().  Unless I am
confused (quite possible), the memory barriers are sufficient.

The rcu_read_lock() and rcu_read_unlock() seem to be needed to avoid
premature freeing, though.

							Thanx, Paul

Re: [PATCH] x86, mce: Fix rcu splat in drain_mce_log_buffer()

[Borislav Petkov]

On Sun, Feb 12, 2012 at 09:41:33AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 06, 2012 at 05:18:02PM +0100, Borislav Petkov wrote:
> > On Fri, Jan 13, 2012 at 01:44:35PM -0800, Paul E. McKenney wrote:
> > > Looks good to me, but I do need to defer to people who know this code
> > > better than do I.  The key thing that (from what I can see) makes
> > > rcu_dereference() unnecessary is that the smp_rmb() used in conjunction
> > > with polling the .finished field takes care of ordering.
> > 
> > Right, this was me trying hard not to screw up touching mcelog.next,
> > thus trying to use the rcu_dereference_index_check() primitive without
> > thinking it through too much. But you're right, I'm polling the
> > ->finished field 4 times (totally arbitrary, btw) which should suffice
> > while the mce_log() routine above writes those entries.
> > 
> > Although, the question still remains, since mce_log() accesses
> > mcelog.next through the rcu_dereference_index_check() primitive,
> > shouldn't I do it the same way?
> 
> I don't claim to be an mce_log() expert, but when I looked it over,
> I didn't see a need for rcu_dereference_index_check().  Unless I am
> confused (quite possible), the memory barriers are sufficient.
> 
> The rcu_read_lock() and rcu_read_unlock() seem to be needed to avoid
> premature freeing, though.

Looka ere:

commit f56e8a0765cc4374e02f4e3a79e2427b5096b075
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Fri Mar 5 15:03:27 2010 -0800

    x86/mce: Fix RCU lockdep splats

    Create an rcu_dereference_check_mce() that checks for RCU-sched
    read side and mce_read_mutex being held on update side.  Replace
    uses of rcu_dereference() in arch/x86/kernel/cpu/mcheck/mce.c
    with this new macro.

...

+#define rcu_dereference_check_mce(p) \
+       rcu_dereference_check((p), \
+                             rcu_read_lock_sched_held() || \
+                             lockdep_is_held(&mce_read_mutex))
+

So I guess the check is to see that we're rather holding the
mce_chrdev_read_mutex when accessing this from userspace (the mcelog
userspace thing).

I can't comment on the RCU-sched thing because I don't know it that
well. It has to do with extended CPU idle times but why do we require to
be in RCU read-side critical section when accessing the index? Hmmm, I
think a guy called Paul should know :-) ...

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551