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

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

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;
>>
>>