Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Paul E. McKenney
On Mon, May 11, 2020 at 06:34:13PM +0100, Will Deacon wrote: > On Mon, May 11, 2020 at 10:29:18AM -0700, Paul E. McKenney wrote: > > On Mon, May 11, 2020 at 05:52:17PM +0100, Will Deacon wrote: > > > On Mon, May 11, 2020 at 09:43:19AM -0700, Paul E. McKenney wrote: > > > > On Mon, May 11, 2020 at

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Will Deacon
On Mon, May 11, 2020 at 10:29:18AM -0700, Paul E. McKenney wrote: > On Mon, May 11, 2020 at 05:52:17PM +0100, Will Deacon wrote: > > On Mon, May 11, 2020 at 09:43:19AM -0700, Paul E. McKenney wrote: > > > On Mon, May 11, 2020 at 04:58:13PM +0100, Will Deacon wrote: > > > > On Sat, May 09, 2020 at

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Paul E. McKenney
On Mon, May 11, 2020 at 05:52:17PM +0100, Will Deacon wrote: > On Mon, May 11, 2020 at 09:43:19AM -0700, Paul E. McKenney wrote: > > On Mon, May 11, 2020 at 04:58:13PM +0100, Will Deacon wrote: > > > On Sat, May 09, 2020 at 02:36:54PM -0700, Paul E. McKenney wrote: > > > > diff --git

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Qian Cai
> On May 11, 2020, at 12:54 PM, Will Deacon wrote: > > Hmm, I don't see how it can remove the cmpxchg(). Do you have a link to that > discussion, please? lore.kernel.org/lkml/20200211124753.gp14...@hirez.programming.kicks-ass.net Correction — if compilers could prove ”prev->next != node” is

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Will Deacon
On Mon, May 11, 2020 at 12:44:26PM -0400, Qian Cai wrote: > > > > On May 11, 2020, at 11:58 AM, Will Deacon wrote: > > > > I'm fine with the data_race() placement, but I don't find the comment > > very helpful. We assign the result of a READ_ONCE() to 'prev' in the > > loop, so I don't think

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Will Deacon
On Mon, May 11, 2020 at 09:43:19AM -0700, Paul E. McKenney wrote: > On Mon, May 11, 2020 at 04:58:13PM +0100, Will Deacon wrote: > > On Sat, May 09, 2020 at 02:36:54PM -0700, Paul E. McKenney wrote: > > > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c > > > index

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Paul E. McKenney
On Mon, May 11, 2020 at 04:58:13PM +0100, Will Deacon wrote: > On Sat, May 09, 2020 at 02:36:54PM -0700, Paul E. McKenney wrote: > > On Sat, May 09, 2020 at 12:53:38PM -0400, Qian Cai wrote: > > > > > > > > > > On May 9, 2020, at 12:12 PM, Paul E. McKenney > > > > wrote: > > > > > > > > Ah,

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Qian Cai
> On May 11, 2020, at 11:58 AM, Will Deacon wrote: > > I'm fine with the data_race() placement, but I don't find the comment > very helpful. We assign the result of a READ_ONCE() to 'prev' in the > loop, so I don't think that the cpu_relax() is really relevant. > > The reason we don't need

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-11 Thread Will Deacon
On Sat, May 09, 2020 at 02:36:54PM -0700, Paul E. McKenney wrote: > On Sat, May 09, 2020 at 12:53:38PM -0400, Qian Cai wrote: > > > > > > > On May 9, 2020, at 12:12 PM, Paul E. McKenney wrote: > > > > > > Ah, and I forgot to ask. Why "if (data_race(prev->next == node)" instead > > > of "if

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-09 Thread Paul E. McKenney
On Sat, May 09, 2020 at 12:53:38PM -0400, Qian Cai wrote: > > > > On May 9, 2020, at 12:12 PM, Paul E. McKenney wrote: > > > > Ah, and I forgot to ask. Why "if (data_race(prev->next == node)" instead > > of "if (data_race(prev->next) == node"? > > I think the one you suggested is slightly

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-09 Thread Qian Cai
> On May 9, 2020, at 12:12 PM, Paul E. McKenney wrote: > > Ah, and I forgot to ask. Why "if (data_race(prev->next == node)" instead > of "if (data_race(prev->next) == node"? I think the one you suggested is slightly better to point out the exact race. Do you want me to resend or you could

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-09 Thread Paul E. McKenney
On Sat, May 09, 2020 at 09:01:53AM -0400, Qian Cai wrote: > > > > On May 9, 2020, at 12:33 AM, Paul E. McKenney wrote: > > > > On Fri, May 08, 2020 at 04:59:05PM -0400, Qian Cai wrote: > >> > >> > >>> On Feb 11, 2020, at 8:54 AM, Qian Cai wrote: > >>> > >>> prev->next could be accessed

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-09 Thread Qian Cai
> On May 9, 2020, at 12:33 AM, Paul E. McKenney wrote: > > On Fri, May 08, 2020 at 04:59:05PM -0400, Qian Cai wrote: >> >> >>> On Feb 11, 2020, at 8:54 AM, Qian Cai wrote: >>> >>> prev->next could be accessed concurrently as noticed by KCSAN, >>> >>> write (marked) to 0x9d3370dbbe40

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-08 Thread Paul E. McKenney
On Fri, May 08, 2020 at 04:59:05PM -0400, Qian Cai wrote: > > > > On Feb 11, 2020, at 8:54 AM, Qian Cai wrote: > > > > prev->next could be accessed concurrently as noticed by KCSAN, > > > > write (marked) to 0x9d3370dbbe40 of 8 bytes by task 3294 on cpu 107: > > osq_lock+0x25f/0x350 > >

Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock

2020-05-08 Thread Qian Cai
> On Feb 11, 2020, at 8:54 AM, Qian Cai wrote: > > prev->next could be accessed concurrently as noticed by KCSAN, > > write (marked) to 0x9d3370dbbe40 of 8 bytes by task 3294 on cpu 107: > osq_lock+0x25f/0x350 > osq_wait_next at kernel/locking/osq_lock.c:79 > (inlined by) osq_lock at