Hi,
Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
This does "fix" the message though and I can use MCE normally.
Thanks,
Davidlohr
From: Davidlohr Bueso <[email protected]>
Based on the following message:
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 1
no locks held by mcelog/2350.
stack backtrace:
Pid: 2350, comm: mcelog Tainted: G W 2.6.37-rc1+ #7
Call Trace:
[<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
[<ffffffff810189e9>] mce_poll+0xa9/0xd0
[<ffffffff81160585>] do_sys_poll+0x275/0x550
[<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
[<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
[<ffffffff81121f08>] ? __do_fault+0x128/0x470
[<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
[<ffffffff810ffe75>] ? unlock_page+0x25/0x30
[<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff811608a7>] sys_ppoll+0x47/0x190
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
At this point the arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 1
no locks held by mcelog/2350.
stack backtrace:
Pid: 2350, comm: mcelog Tainted: G W 2.6.37-rc1+ #7
Call Trace:
[<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
[<ffffffff810189e9>] mce_poll+0xa9/0xd0
[<ffffffff81160585>] do_sys_poll+0x275/0x550
[<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
[<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
[<ffffffff81121f08>] ? __do_fault+0x128/0x470
[<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
[<ffffffff810ffe75>] ? unlock_page+0x25/0x30
[<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff811608a7>] sys_ppoll+0x47/0x190
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
At this point the lockdep_is_held(&mce_read_mutex) call is failing.
So check if the mce_read_mutex is held before derefencing instead of using rcu_dereference_check_mce()
Signed-off-by: Davidlohr Bueso <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 7a35b72..6f95b2c 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1625,8 +1625,12 @@ out:
static unsigned int mce_poll(struct file *file, poll_table *wait)
{
poll_wait(file, &mce_wait, wait);
- if (rcu_dereference_check_mce(mcelog.next))
- return POLLIN | POLLRDNORM;
+
+ if (mutex_is_locked(&mce_read_mutex)) {
+ if (rcu_dereference_index_check(mcelog.next,
+ rcu_read_lock_sched_held()))
+ return POLLIN | POLLRDNORM;
+ }
if (!mce_apei_read_done && apei_check_mce())
return POLLIN | POLLRDNORM;
return 0;
--
1.7.1
Sorry, the title should have been:
[PATCH] mce: fix RCU lockdep from mce_poll()
On Fri, 2010-11-05 at 18:44 -0300, Davidlohr Bueso wrote:
> Hi,
>
> Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> This does "fix" the message though and I can use MCE normally.
>
> Thanks,
> Davidlohr
>
>
> From: Davidlohr Bueso <[email protected]>
>
> Based on the following message:
>
> ===================================================
> [ INFO: suspicious rcu_dereference_check() usage. ]
> ---------------------------------------------------
> arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!
>
> other info that might help us debug this:
>
> rcu_scheduler_active = 1, debug_locks = 1
> no locks held by mcelog/2350.
>
> stack backtrace:
> Pid: 2350, comm: mcelog Tainted: G W 2.6.37-rc1+ #7
> Call Trace:
> [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
> [<ffffffff810189e9>] mce_poll+0xa9/0xd0
> [<ffffffff81160585>] do_sys_poll+0x275/0x550
> [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
> [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
> [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
> [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
> [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
> [<ffffffff81121f08>] ? __do_fault+0x128/0x470
> [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
> [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
> [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
> [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
> [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
> [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
> [<ffffffff811608a7>] sys_ppoll+0x47/0x190
> [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
> [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
>
> At this point the arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!
>
> other info that might help us debug this:
>
> rcu_scheduler_active = 1, debug_locks = 1
> no locks held by mcelog/2350.
>
> stack backtrace:
> Pid: 2350, comm: mcelog Tainted: G W 2.6.37-rc1+ #7
> Call Trace:
> [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
> [<ffffffff810189e9>] mce_poll+0xa9/0xd0
> [<ffffffff81160585>] do_sys_poll+0x275/0x550
> [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
> [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
> [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
> [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
> [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
> [<ffffffff81121f08>] ? __do_fault+0x128/0x470
> [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
> [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
> [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
> [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
> [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
> [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
> [<ffffffff811608a7>] sys_ppoll+0x47/0x190
> [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
> [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
>
> At this point the lockdep_is_held(&mce_read_mutex) call is failing.
> So check if the mce_read_mutex is held before derefencing instead of using rcu_dereference_check_mce()
>
> Signed-off-by: Davidlohr Bueso <[email protected]>
> ---
> arch/x86/kernel/cpu/mcheck/mce.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 7a35b72..6f95b2c 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1625,8 +1625,12 @@ out:
> static unsigned int mce_poll(struct file *file, poll_table *wait)
> {
> poll_wait(file, &mce_wait, wait);
> - if (rcu_dereference_check_mce(mcelog.next))
> - return POLLIN | POLLRDNORM;
> +
> + if (mutex_is_locked(&mce_read_mutex)) {
> + if (rcu_dereference_index_check(mcelog.next,
> + rcu_read_lock_sched_held()))
> + return POLLIN | POLLRDNORM;
> + }
> if (!mce_apei_read_done && apei_check_mce())
> return POLLIN | POLLRDNORM;
> return 0;
> --
> 1.7.1
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> Hi,
>
> Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> This does "fix" the message though and I can use MCE normally.
The patch is certainly not correct. The variable needs to be read
independently of the mutex.
-Andi
On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > Hi,
> >
> > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > This does "fix" the message though and I can use MCE normally.
>
> The patch is certainly not correct. The variable needs to be read
> independently of the mutex.
This code is simply checking the value of the pointer, and therefore
need not protect any actual dereferences. So why not replace the
rcu_dereference_check_mce() with rcu_access_pointer()? If this is
OK, please see the patch below.
BTW, assigning the value returned by rcu_access_pointer() into a
variable often indicates a bug. ;-)
Thanx, Paul
Signed-off-by: Paul E. McKenney <[email protected]>
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 7a35b72..4d29d50 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1625,7 +1625,7 @@ out:
static unsigned int mce_poll(struct file *file, poll_table *wait)
{
poll_wait(file, &mce_wait, wait);
- if (rcu_dereference_check_mce(mcelog.next))
+ if (rcu_access_pointer(mcelog.next))
return POLLIN | POLLRDNORM;
if (!mce_apei_read_done && apei_check_mce())
return POLLIN | POLLRDNORM;
On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > > Hi,
> > >
> > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > > This does "fix" the message though and I can use MCE normally.
> >
> > The patch is certainly not correct. The variable needs to be read
> > independently of the mutex.
>
> This code is simply checking the value of the pointer, and therefore
> need not protect any actual dereferences. So why not replace the
> rcu_dereference_check_mce() with rcu_access_pointer()? If this is
> OK, please see the patch below.
>
> BTW, assigning the value returned by rcu_access_pointer() into a
> variable often indicates a bug. ;-)
>
> Thanx, Paul
>
> Signed-off-by: Paul E. McKenney <[email protected]>
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 7a35b72..4d29d50 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1625,7 +1625,7 @@ out:
> static unsigned int mce_poll(struct file *file, poll_table *wait)
> {
> poll_wait(file, &mce_wait, wait);
> - if (rcu_dereference_check_mce(mcelog.next))
> + if (rcu_access_pointer(mcelog.next))
this doesn't compile (mcelog.next is an index):
arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
‘unary *’ (have ‘unsigned int’)
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
in declaration of ‘_________p1’
arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
‘unary *’ (have ‘unsigned int’)
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
in declaration of ‘type name’
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
integer of different size
arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
‘unary *’ (have ‘unsigned int’)
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
in declaration of ‘type name’
make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
Since the mutex is independent, what about this patch?
Signed-off-by: Davidlohr Bueso <[email protected]>
---
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 7a35b72..cc1c673 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1625,7 +1625,7 @@ out:
static unsigned int mce_poll(struct file *file, poll_table *wait)
{
poll_wait(file, &mce_wait, wait);
- if (rcu_dereference_check_mce(mcelog.next))
+ if (rcu_dereference_index_check(mcelog.next,
rcu_read_lock_sched_held()))
return POLLIN | POLLRDNORM;
if (!mce_apei_read_done && apei_check_mce())
return POLLIN | POLLRDNORM;
--
1.7.1
On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
> On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > > > Hi,
> > > >
> > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > > > This does "fix" the message though and I can use MCE normally.
> > >
> > > The patch is certainly not correct. The variable needs to be read
> > > independently of the mutex.
> >
> > This code is simply checking the value of the pointer, and therefore
> > need not protect any actual dereferences. So why not replace the
> > rcu_dereference_check_mce() with rcu_access_pointer()? If this is
> > OK, please see the patch below.
> >
> > BTW, assigning the value returned by rcu_access_pointer() into a
> > variable often indicates a bug. ;-)
> >
> > Thanx, Paul
> >
> > Signed-off-by: Paul E. McKenney <[email protected]>
> >
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > index 7a35b72..4d29d50 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -1625,7 +1625,7 @@ out:
> > static unsigned int mce_poll(struct file *file, poll_table *wait)
> > {
> > poll_wait(file, &mce_wait, wait);
> > - if (rcu_dereference_check_mce(mcelog.next))
> > + if (rcu_access_pointer(mcelog.next))
>
> this doesn't compile (mcelog.next is an index):
>
> arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> ‘unary *’ (have ‘unsigned int’)
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> in declaration of ‘_________p1’
> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> ‘unary *’ (have ‘unsigned int’)
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> in declaration of ‘type name’
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
> integer of different size
> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> ‘unary *’ (have ‘unsigned int’)
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> in declaration of ‘type name’
> make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
>
>
> Since the mutex is independent, what about this patch?
Looks good to me!
Acked-by: Paul E. McKenney <[email protected]>
> Signed-off-by: Davidlohr Bueso <[email protected]>
>
> ---
> 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 7a35b72..cc1c673 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1625,7 +1625,7 @@ out:
> static unsigned int mce_poll(struct file *file, poll_table *wait)
> {
> poll_wait(file, &mce_wait, wait);
> - if (rcu_dereference_check_mce(mcelog.next))
> + if (rcu_dereference_index_check(mcelog.next,
> rcu_read_lock_sched_held()))
> return POLLIN | POLLRDNORM;
> if (!mce_apei_read_done && apei_check_mce())
> return POLLIN | POLLRDNORM;
> --
> 1.7.1
>
>
>
On Mon, 2010-11-08 at 05:17 -0800, Paul E. McKenney wrote:
> On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
> > On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> > > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> > > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > > > > Hi,
> > > > >
> > > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > > > > This does "fix" the message though and I can use MCE normally.
> > > >
> > > > The patch is certainly not correct. The variable needs to be read
> > > > independently of the mutex.
> > >
> > > This code is simply checking the value of the pointer, and therefore
> > > need not protect any actual dereferences. So why not replace the
> > > rcu_dereference_check_mce() with rcu_access_pointer()? If this is
> > > OK, please see the patch below.
> > >
> > > BTW, assigning the value returned by rcu_access_pointer() into a
> > > variable often indicates a bug. ;-)
> > >
> > > Thanx, Paul
> > >
> > > Signed-off-by: Paul E. McKenney <[email protected]>
> > >
> > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > > index 7a35b72..4d29d50 100644
> > > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > > @@ -1625,7 +1625,7 @@ out:
> > > static unsigned int mce_poll(struct file *file, poll_table *wait)
> > > {
> > > poll_wait(file, &mce_wait, wait);
> > > - if (rcu_dereference_check_mce(mcelog.next))
> > > + if (rcu_access_pointer(mcelog.next))
> >
> > this doesn't compile (mcelog.next is an index):
> >
> > arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> > ‘unary *’ (have ‘unsigned int’)
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> > in declaration of ‘_________p1’
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> > ‘unary *’ (have ‘unsigned int’)
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> > in declaration of ‘type name’
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
> > integer of different size
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> > ‘unary *’ (have ‘unsigned int’)
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> > in declaration of ‘type name’
> > make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
> >
> >
> > Since the mutex is independent, what about this patch?
>
> Looks good to me!
>
> Acked-by: Paul E. McKenney <[email protected]>
>
Andi, any further thoughts?
Thanks,
Davidlohr
2010/11/8 Paul E. McKenney <[email protected]>:
> On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
>> On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
>> > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
>> > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
>> > > > Hi,
>> > > >
>> > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
>> > > > This does "fix" the message though and I can use MCE normally.
>> > >
>> > > The patch is certainly not correct. The variable needs to be read
>> > > independently of the mutex.
>> >
>> > This code is simply checking the value of the pointer, and therefore
>> > need not protect any actual dereferences. ?So why not replace the
>> > rcu_dereference_check_mce() with rcu_access_pointer()? ?If this is
>> > OK, please see the patch below.
>> >
>> > BTW, assigning the value returned by rcu_access_pointer() into a
>> > variable often indicates a bug. ?;-)
>> >
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? Thanx, Paul
>> >
>> > Signed-off-by: Paul E. McKenney <[email protected]>
>> >
>> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> > index 7a35b72..4d29d50 100644
>> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> > @@ -1625,7 +1625,7 @@ out:
>> > ?static unsigned int mce_poll(struct file *file, poll_table *wait)
>> > ?{
>> > ? ? poll_wait(file, &mce_wait, wait);
>> > - ? if (rcu_dereference_check_mce(mcelog.next))
>> > + ? if (rcu_access_pointer(mcelog.next))
>>
>> this doesn't compile (mcelog.next is an index):
>>
>> arch/x86/kernel/cpu/mcheck/mce.c: In function ?mce_poll?:
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
>> ?unary *? (have ?unsigned int?)
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ?int?
>> in declaration of ?_________p1?
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
>> ?unary *? (have ?unsigned int?)
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ?int?
>> in declaration of ?type name?
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
>> integer of different size
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
>> ?unary *? (have ?unsigned int?)
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ?int?
>> in declaration of ?type name?
>> make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
>>
>>
>> Since the mutex is independent, what about this patch?
>
> Looks good to me!
>
> Acked-by: Paul E. McKenney <[email protected]>
>
>> ?Signed-off-by: Davidlohr Bueso <[email protected]>
>>
>> ---
>> ?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 7a35b72..cc1c673 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -1625,7 +1625,7 @@ out:
>> ?static unsigned int mce_poll(struct file *file, poll_table *wait)
>> ?{
>> ? ? ? poll_wait(file, &mce_wait, wait);
>> - ? ? if (rcu_dereference_check_mce(mcelog.next))
>> + ? ? if (rcu_dereference_index_check(mcelog.next,
>> rcu_read_lock_sched_held()))
>> ? ? ? ? ? ? ? return POLLIN | POLLRDNORM;
>> ? ? ? if (!mce_apei_read_done && apei_check_mce())
>> ? ? ? ? ? ? ? return POLLIN | POLLRDNORM;
Any chance to have this ever fixed upstream ?
(still happens with today's vanialla build)
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
arch/x86/kernel/cpu/mcheck/mce.c:1629 invoked rcu_dereference_check()
without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0
no locks held by mcelog/772.
stack backtrace:
Pid: 772, comm: mcelog Not tainted 2.6.38-09069-gc7dfeb9 #103
Call Trace:
[<ffffffff8108bb7b>] lockdep_rcu_dereference+0xbb/0xc0
[<ffffffff81017c55>] mce_poll+0xa5/0xd0
[<ffffffff81161be0>] do_sys_poll+0x270/0x500
[<ffffffff81160780>] ? poll_freewait+0xe0/0xe0
[<ffffffff81160870>] ? __pollwait+0xf0/0xf0
[<ffffffff81160870>] ? __pollwait+0xf0/0xf0
[<ffffffff8111d678>] ? __do_fault+0x128/0x490
[<ffffffff81009136>] ? native_sched_clock+0x26/0x70
[<ffffffff8107d9b7>] ? local_clock+0x47/0x60
[<ffffffff8108a558>] ? trace_hardirqs_off_caller+0x28/0xc0
[<ffffffff8108dc00>] ? __lock_acquire+0x410/0x1bb0
[<ffffffff81120284>] ? handle_pte_fault+0x84/0x900
[<ffffffff811049dd>] ? __free_pages+0x2d/0x40
[<ffffffff8111e0c0>] ? __pte_alloc+0xd0/0x120
[<ffffffff81064a81>] ? sigprocmask+0x41/0x100
[<ffffffff81498969>] ? sub_preempt_count+0xa9/0xe0
[<ffffffff8149528b>] ? _raw_spin_unlock_irq+0x3b/0x60
[<ffffffff81064a0b>] ? recalc_sigpending+0x1b/0x50
[<ffffffff811620d4>] sys_ppoll+0xe4/0x180
[<ffffffff812946ce>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff8149c86b>] system_call_fastpath+0x16/0x1b
Zdenek
On Tue, 2011-03-29 at 11:45 +0200, Zdenek Kabelac wrote:
> 2010/11/8 Paul E. McKenney <[email protected]>:
> > On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
> >> On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> >> > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> >> > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> >> > > > Hi,
> >> > > >
> >> > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> >> > > > This does "fix" the message though and I can use MCE normally.
> >> > >
> >> > > The patch is certainly not correct. The variable needs to be read
> >> > > independently of the mutex.
> >> >
> >> > This code is simply checking the value of the pointer, and therefore
> >> > need not protect any actual dereferences. So why not replace the
> >> > rcu_dereference_check_mce() with rcu_access_pointer()? If this is
> >> > OK, please see the patch below.
> >> >
> >> > BTW, assigning the value returned by rcu_access_pointer() into a
> >> > variable often indicates a bug. ;-)
> >> >
> >> > Thanx, Paul
> >> >
> >> > Signed-off-by: Paul E. McKenney <[email protected]>
> >> >
> >> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> >> > index 7a35b72..4d29d50 100644
> >> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> >> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> >> > @@ -1625,7 +1625,7 @@ out:
> >> > static unsigned int mce_poll(struct file *file, poll_table *wait)
> >> > {
> >> > poll_wait(file, &mce_wait, wait);
> >> > - if (rcu_dereference_check_mce(mcelog.next))
> >> > + if (rcu_access_pointer(mcelog.next))
> >>
> >> this doesn't compile (mcelog.next is an index):
> >>
> >> arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> >> ‘unary *’ (have ‘unsigned int’)
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> >> in declaration of ‘_________p1’
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> >> ‘unary *’ (have ‘unsigned int’)
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> >> in declaration of ‘type name’
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
> >> integer of different size
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> >> ‘unary *’ (have ‘unsigned int’)
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> >> in declaration of ‘type name’
> >> make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
> >>
> >>
> >> Since the mutex is independent, what about this patch?
> >
> > Looks good to me!
> >
> > Acked-by: Paul E. McKenney <[email protected]>
> >
> >> Signed-off-by: Davidlohr Bueso <[email protected]>
> >>
> >> ---
> >> 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 7a35b72..cc1c673 100644
> >> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> >> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> >> @@ -1625,7 +1625,7 @@ out:
> >> static unsigned int mce_poll(struct file *file, poll_table *wait)
> >> {
> >> poll_wait(file, &mce_wait, wait);
> >> - if (rcu_dereference_check_mce(mcelog.next))
> >> + if (rcu_dereference_index_check(mcelog.next,
> >> rcu_read_lock_sched_held()))
> >> return POLLIN | POLLRDNORM;
> >> if (!mce_apei_read_done && apei_check_mce())
> >> return POLLIN | POLLRDNORM;
>
>
>
> Any chance to have this ever fixed upstream ?
> (still happens with today's vanialla build)
I'm still quite interested in getting this fixed, I run into it several
times a day.
> I'm still quite interested in getting this fixed, I run into it several
> times a day.
Paul needs to send his fix to the x86 maintainers. They were not cc'ed
so never saw this.
-Andi
Based on the following message
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 1
no locks held by mcelog/2350.
stack backtrace:
Pid: 2350, comm: mcelog Tainted: G W 2.6.37-rc1+ #7
Call Trace:
[<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
[<ffffffff810189e9>] mce_poll+0xa9/0xd0
[<ffffffff81160585>] do_sys_poll+0x275/0x550
[<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
[<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
[<ffffffff81121f08>] ? __do_fault+0x128/0x470
[<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
[<ffffffff810ffe75>] ? unlock_page+0x25/0x30
[<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff811608a7>] sys_ppoll+0x47/0x190
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
This code is simply checking the value of the pointer, and therefore need not protect any actual dereferences.
Replace rcu_dereference_check_mce() with rcu_dereference_index_check().
Signed-off-by: Davidlohr Bueso <[email protected]>
Acked-by: Paul E. McKenney <[email protected]>
---
This patch reflects the summary of the activity in https://lkml.org/lkml/2010/11/5/236
arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 5a05ef6..6cad5dc 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1626,7 +1626,8 @@ out:
static unsigned int mce_poll(struct file *file, poll_table *wait)
{
poll_wait(file, &mce_wait, wait);
- if (rcu_dereference_check_mce(mcelog.next))
+ if (rcu_dereference_index_check(mcelog.next,
+ rcu_read_lock_sched_held()))
return POLLIN | POLLRDNORM;
if (!mce_apei_read_done && apei_check_mce())
return POLLIN | POLLRDNORM;
--
1.7.1
Commit-ID: 706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
Gitweb: http://git.kernel.org/tip/706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
Author: Davidlohr Bueso <[email protected]>
AuthorDate: Wed, 30 Mar 2011 23:13:24 -0300
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 31 Mar 2011 11:06:08 +0200
x86, mce: Fix RCU lockdep from mce_poll()
Based on the following message:
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked
rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 1
no locks held by mcelog/2350.
stack backtrace:
Pid: 2350, comm: mcelog Tainted: G W 2.6.37-rc1+ #7
Call Trace:
[<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
[<ffffffff810189e9>] mce_poll+0xa9/0xd0
[<ffffffff81160585>] do_sys_poll+0x275/0x550
[<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8115f1d0>] ? pollwake+0x0/0x60
[<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
[<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
[<ffffffff81121f08>] ? __do_fault+0x128/0x470
[<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
[<ffffffff810ffe75>] ? unlock_page+0x25/0x30
[<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
[<ffffffff811608a7>] sys_ppoll+0x47/0x190
[<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
[<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
This code is simply checking the value of the pointer, and
therefore need not protect any actual dereferences. Replace
rcu_dereference_check_mce() with rcu_dereference_index_check().
Signed-off-by: Davidlohr Bueso <[email protected]>
Acked-by: Paul E. McKenney <[email protected]>
Cc: Zdenek Kabelac <[email protected]>
Cc: Andi Kleen <[email protected]>
LKML-Reference: <1301537604.2140.21.camel@offworld>
Signed-off-by: Ingo Molnar <[email protected]>
---
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 5a05ef6..a2d664f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1626,7 +1626,7 @@ out:
static unsigned int mce_poll(struct file *file, poll_table *wait)
{
poll_wait(file, &mce_wait, wait);
- if (rcu_dereference_check_mce(mcelog.next))
+ if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
return POLLIN | POLLRDNORM;
if (!mce_apei_read_done && apei_check_mce())
return POLLIN | POLLRDNORM;
However on my machine - I do not a see a difference ?
I've this patch applied - and it still gives me the same error.
(using 6aba74f2791287ec407e0f92487a725a25908067 and this patch)
Here are my RCU config options:
# grep RCU .config
# RCU Subsystem
CONFIG_TREE_PREEMPT_RCU=y
CONFIG_PREEMPT_RCU=y
CONFIG_RCU_TRACE=y
CONFIG_RCU_FANOUT=64
# CONFIG_RCU_FANOUT_EXACT is not set
CONFIG_TREE_RCU_TRACE=y
CONFIG_PROVE_RCU=y
# CONFIG_PROVE_RCU_REPEATEDLY is not set
# CONFIG_SPARSE_RCU_POINTER is not set
# CONFIG_RCU_TORTURE_TEST is not set
CONFIG_RCU_CPU_STALL_DETECTOR=y
CONFIG_RCU_CPU_STALL_TIMEOUT=60
CONFIG_RCU_CPU_STALL_DETECTOR_RUNNABLE=y
CONFIG_RCU_CPU_STALL_VERBOSE=y
2011/3/31 tip-bot for Davidlohr Bueso <[email protected]>:
> Commit-ID: ?706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
> Gitweb: ? ? http://git.kernel.org/tip/706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
> Author: ? ? Davidlohr Bueso <[email protected]>
> AuthorDate: Wed, 30 Mar 2011 23:13:24 -0300
> Committer: ?Ingo Molnar <[email protected]>
> CommitDate: Thu, 31 Mar 2011 11:06:08 +0200
>
> x86, mce: Fix RCU lockdep from mce_poll()
>
> Based on the following message:
>
> ?===================================================
> ?[ INFO: suspicious rcu_dereference_check() usage. ]
> ?---------------------------------------------------
> ?arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked
> ?rcu_dereference_check() without protection!
>
> ?other info that might help us debug this:
>
> ?rcu_scheduler_active = 1, debug_locks = 1
> ?no locks held by mcelog/2350.
>
> ?stack backtrace:
> ?Pid: 2350, comm: mcelog Tainted: G ? ? ? ?W ? 2.6.37-rc1+ #7
> ?Call Trace:
> ? [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
> ? [<ffffffff810189e9>] mce_poll+0xa9/0xd0
> ? [<ffffffff81160585>] do_sys_poll+0x275/0x550
> ? [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
> ? [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
> ? [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
> ? [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
> ? [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
> ? [<ffffffff81121f08>] ? __do_fault+0x128/0x470
> ? [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
> ? [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
> ? [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
> ? [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
> ? [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
> ? [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
> ? [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
> ? [<ffffffff811608a7>] sys_ppoll+0x47/0x190
> ? [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
> ? [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> ? [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
>
> This code is simply checking the value of the pointer, and
> therefore need not protect any actual dereferences. Replace
> rcu_dereference_check_mce() with rcu_dereference_index_check().
>
> Signed-off-by: Davidlohr Bueso <[email protected]>
> Acked-by: Paul E. McKenney <[email protected]>
> Cc: Zdenek Kabelac <[email protected]>
> Cc: Andi Kleen <[email protected]>
> LKML-Reference: <1301537604.2140.21.camel@offworld>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> ?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 5a05ef6..a2d664f 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1626,7 +1626,7 @@ out:
> ?static unsigned int mce_poll(struct file *file, poll_table *wait)
> ?{
> ? ? ? ?poll_wait(file, &mce_wait, wait);
> - ? ? ? if (rcu_dereference_check_mce(mcelog.next))
> + ? ? ? if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
> ? ? ? ? ? ? ? ?return POLLIN | POLLRDNORM;
> ? ? ? ?if (!mce_apei_read_done && apei_check_mce())
> ? ? ? ? ? ? ? ?return POLLIN | POLLRDNORM;
>
On Thu, Mar 31, 2011 at 12:03:48PM +0200, Zdenek Kabelac wrote:
> However on my machine - I do not a see a difference ?
>
> I've this patch applied - and it still gives me the same error.
> (using 6aba74f2791287ec407e0f92487a725a25908067 and this patch)
>
> Here are my RCU config options:
>
> # grep RCU .config
> # RCU Subsystem
> CONFIG_TREE_PREEMPT_RCU=y
> CONFIG_PREEMPT_RCU=y
> CONFIG_RCU_TRACE=y
> CONFIG_RCU_FANOUT=64
> # CONFIG_RCU_FANOUT_EXACT is not set
> CONFIG_TREE_RCU_TRACE=y
> CONFIG_PROVE_RCU=y
> # CONFIG_PROVE_RCU_REPEATEDLY is not set
> # CONFIG_SPARSE_RCU_POINTER is not set
> # CONFIG_RCU_TORTURE_TEST is not set
> CONFIG_RCU_CPU_STALL_DETECTOR=y
> CONFIG_RCU_CPU_STALL_TIMEOUT=60
> CONFIG_RCU_CPU_STALL_DETECTOR_RUNNABLE=y
> CONFIG_RCU_CPU_STALL_VERBOSE=y
What idiot created that patch, anyway??? ;-)
Here is a patch that is much more likely to do something useful.
Could you please test it? It is on top of the patch you just tested.
Thanx, Paul
------------------------------------------------------------------------
rcu: create new rcu_access_index() and use in mce
The MCE subsystem needs to sample an RCU-protected index outside of
any protection for that index. If this was a pointer, we would use
rcu_access_pointer(), but there is no corresponding rcu_access_index().
This commit therefore creates an rcu_access_index() and applies it
to MCE.
Signed-off-by: Paul E. McKenney <[email protected]>
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a2d664f..3385ea2 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1626,7 +1626,7 @@ out:
static unsigned int mce_poll(struct file *file, poll_table *wait)
{
poll_wait(file, &mce_wait, wait);
- if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
+ if (rcu_access_index(mcelog.next))
return POLLIN | POLLRDNORM;
if (!mce_apei_read_done && apei_check_mce())
return POLLIN | POLLRDNORM;
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index af56148..ff422d2 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -339,6 +339,12 @@ extern int rcu_my_thread_group_empty(void);
((typeof(*p) __force __kernel *)(p)); \
})
+#define __rcu_access_index(p, space) \
+ ({ \
+ typeof(p) _________p1 = ACCESS_ONCE(p); \
+ rcu_dereference_sparse(p, space); \
+ (_________p1); \
+ })
#define __rcu_dereference_index_check(p, c) \
({ \
typeof(p) _________p1 = ACCESS_ONCE(p); \
@@ -429,6 +435,20 @@ extern int rcu_my_thread_group_empty(void);
#define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
/**
+ * rcu_access_index() - fetch RCU index with no dereferencing
+ * @p: The index to read
+ *
+ * Return the value of the specified RCU-protected index, but omit the
+ * smp_read_barrier_depends() and keep the ACCESS_ONCE(). This is useful
+ * when the value of this index is accessed, but the index is not
+ * dereferenced, for example, when testing an RCU-protected index against
+ * -1. Although rcu_access_index() may also be used in cases where
+ * update-side locks prevent the value of the index from changing, you
+ * should instead use rcu_dereference_index_protected() for this use case.
+ */
+#define rcu_access_index(p) __rcu_access_index((p), __rcu)
+
+/**
* rcu_dereference_index_check() - rcu_dereference for indices with debug checking
* @p: The pointer to read, prior to dereferencing
* @c: The conditions under which the dereference will take place
2011/3/31 Paul E. McKenney <[email protected]>:
> On Thu, Mar 31, 2011 at 12:03:48PM +0200, Zdenek Kabelac wrote:
>> However on my machine - I do not a see a difference ?
>>
>> I've this patch applied - and it still gives me the same error.
>> (using 6aba74f2791287ec407e0f92487a725a25908067 and this patch)
>>
>> Here are my RCU config options:
>>
>> # grep RCU .config
>> # RCU Subsystem
>> CONFIG_TREE_PREEMPT_RCU=y
>> CONFIG_PREEMPT_RCU=y
>> CONFIG_RCU_TRACE=y
>> CONFIG_RCU_FANOUT=64
>> # CONFIG_RCU_FANOUT_EXACT is not set
>> CONFIG_TREE_RCU_TRACE=y
>> CONFIG_PROVE_RCU=y
>> # CONFIG_PROVE_RCU_REPEATEDLY is not set
>> # CONFIG_SPARSE_RCU_POINTER is not set
>> # CONFIG_RCU_TORTURE_TEST is not set
>> CONFIG_RCU_CPU_STALL_DETECTOR=y
>> CONFIG_RCU_CPU_STALL_TIMEOUT=60
>> CONFIG_RCU_CPU_STALL_DETECTOR_RUNNABLE=y
>> CONFIG_RCU_CPU_STALL_VERBOSE=y
>
> What idiot created that patch, anyway??? ?;-)
>
> Here is a patch that is much more likely to do something useful.
> Could you please test it? ?It is on top of the patch you just tested.
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?Thanx, Paul
>
> ------------------------------------------------------------------------
>
> rcu: create new rcu_access_index() and use in mce
>
> The MCE subsystem needs to sample an RCU-protected index outside of
> any protection for that index. ?If this was a pointer, we would use
> rcu_access_pointer(), but there is no corresponding rcu_access_index().
> This commit therefore creates an rcu_access_index() and applies it
> to MCE.
>
> Signed-off-by: Paul E. McKenney <[email protected]>
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index a2d664f..3385ea2 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1626,7 +1626,7 @@ out:
> ?static unsigned int mce_poll(struct file *file, poll_table *wait)
> ?{
> ? ? ? ?poll_wait(file, &mce_wait, wait);
> - ? ? ? if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
> + ? ? ? if (rcu_access_index(mcelog.next))
> ? ? ? ? ? ? ? ?return POLLIN | POLLRDNORM;
> ? ? ? ?if (!mce_apei_read_done && apei_check_mce())
> ? ? ? ? ? ? ? ?return POLLIN | POLLRDNORM;
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index af56148..ff422d2 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -339,6 +339,12 @@ extern int rcu_my_thread_group_empty(void);
> ? ? ? ? ? ? ? ?((typeof(*p) __force __kernel *)(p)); \
> ? ? ? ?})
>
> +#define __rcu_access_index(p, space) \
> + ? ? ? ({ \
> + ? ? ? ? ? ? ? typeof(p) _________p1 = ACCESS_ONCE(p); \
> + ? ? ? ? ? ? ? rcu_dereference_sparse(p, space); \
> + ? ? ? ? ? ? ? (_________p1); \
> + ? ? ? })
> ?#define __rcu_dereference_index_check(p, c) \
> ? ? ? ?({ \
> ? ? ? ? ? ? ? ?typeof(p) _________p1 = ACCESS_ONCE(p); \
> @@ -429,6 +435,20 @@ extern int rcu_my_thread_group_empty(void);
> ?#define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
>
> ?/**
> + * rcu_access_index() - fetch RCU index with no dereferencing
> + * @p: The index to read
> + *
> + * Return the value of the specified RCU-protected index, but omit the
> + * smp_read_barrier_depends() and keep the ACCESS_ONCE(). ?This is useful
> + * when the value of this index is accessed, but the index is not
> + * dereferenced, for example, when testing an RCU-protected index against
> + * -1. ?Although rcu_access_index() may also be used in cases where
> + * update-side locks prevent the value of the index from changing, you
> + * should instead use rcu_dereference_index_protected() for this use case.
> + */
> +#define rcu_access_index(p) __rcu_access_index((p), __rcu)
> +
> +/**
> ?* rcu_dereference_index_check() - rcu_dereference for indices with debug checking
> ?* @p: The pointer to read, prior to dereferencing
> ?* @c: The conditions under which the dereference will take place
>
Works for me.
Tested-by: Zdenek Kabelac <[email protected]>
Zdenek
On Thu, Mar 31, 2011 at 11:32:20PM +0200, Zdenek Kabelac wrote:
> 2011/3/31 Paul E. McKenney <[email protected]>:
> > On Thu, Mar 31, 2011 at 12:03:48PM +0200, Zdenek Kabelac wrote:
[ . . . ]
> > +/**
> > ?* rcu_dereference_index_check() - rcu_dereference for indices with debug checking
> > ?* @p: The pointer to read, prior to dereferencing
> > ?* @c: The conditions under which the dereference will take place
> >
>
> Works for me.
>
> Tested-by: Zdenek Kabelac <[email protected]>
Thank you, Zdenek!
Andi, did you want to carry this patch, or would you prefer that I did so?
If the latter, could you please revert the busted patch?
Thanx, Paul