Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1400849 > unrolled thread

Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Zijlstra <peterz@infradead.org> - 2016-05-13 17:10 +0200
    Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Hurley <peter@hurleysoftware.com> - 2016-05-13 20:00 +0200
      Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Zijlstra <peterz@infradead.org> - 2016-05-16 13:20 +0200
        Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-16 14:20 +0200
          Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Hurley <peter@hurleysoftware.com> - 2016-05-16 16:20 +0200
            Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-16 19:30 +0200
              Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Hurley <peter@hurleysoftware.com> - 2016-05-17 21:50 +0200
                Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Hurley <peter@hurleysoftware.com> - 2016-05-17 22:00 +0200
            Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Zijlstra <peterz@infradead.org> - 2016-05-16 20:00 +0200
              Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Hurley <peter@hurleysoftware.com> - 2016-05-17 21:20 +0200
                Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-17 21:50 +0200
                  Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Zijlstra <peterz@infradead.org> - 2016-05-18 13:10 +0200
                    Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Waiman Long <waiman.long@hpe.com> - 2016-05-18 18:00 +0200
                      Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-18 19:30 +0200
                    Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-18 19:30 +0200
                      Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field Peter Zijlstra <peterz@infradead.org> - 2016-05-19 11:10 +0200
                        Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner  field "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-19 15:50 +0200

#1400849 — Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field

On Wed, May 11, 2016 at 03:04:20PM -0700, Peter Hurley wrote:
> > +	return !rwsem_is_reader_owned(READ_ONCE(sem->owner));
> 
> It doesn't make sense to force reload sem->owner here; if sem->owner
> is not being reloaded then the loop above will execute forever.
> 
> Arguably, this check should be bumped out to the optimistic spin and
> reload/check the owner there?
> 

Note that barrier() and READ_ONCE() have overlapping but not identical
results and the combined use actually makes sense here.

Yes, a barrier() anywhere in the loop will force a reload of the
variable, _however_ it doesn't force that reload to not suffer from
load tearing.

Using volatile also forces a reload, but also ensures the load cannot
be torn IFF it is of machine word side and naturally aligned.

So while the READ_ONCE() here is pointless for forcing the reload;
that's already ensured, we still need to make sure the load isn't torn.

[toc] | [next] | [standalone]

#1400924

On 05/13/2016 08:07 AM, Peter Zijlstra wrote:
> On Wed, May 11, 2016 at 03:04:20PM -0700, Peter Hurley wrote:
>>> +	return !rwsem_is_reader_owned(READ_ONCE(sem->owner));
>>
>> It doesn't make sense to force reload sem->owner here; if sem->owner
>> is not being reloaded then the loop above will execute forever.
>>
>> Arguably, this check should be bumped out to the optimistic spin and
>> reload/check the owner there?
>>
> 
> Note that barrier() and READ_ONCE() have overlapping but not identical
> results and the combined use actually makes sense here.
> 
> Yes, a barrier() anywhere in the loop will force a reload of the
> variable, _however_ it doesn't force that reload to not suffer from
> load tearing.
> 
> Using volatile also forces a reload, but also ensures the load cannot
> be torn IFF it is of machine word side and naturally aligned.
> 
> So while the READ_ONCE() here is pointless for forcing the reload;
> that's already ensured, we still need to make sure the load isn't torn.

If load tearing a naturally aligned pointer is a real code generation
possibility then the rcu list code is broken too (which loads ->next
directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).

For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc
that had to do with control dependencies and not load tearing.

OTOH, this patch might actually produce store-tearing:

+static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
+{
+	/*
+	 * We check the owner value first to make sure that we will only
+	 * do a write to the rwsem cacheline when it is really necessary
+	 * to minimize cacheline contention.
+	 */
+	if (sem->owner != RWSEM_READER_OWNED)
+		sem->owner = RWSEM_READER_OWNED;
+}


Regards,
Peter Hurley

[toc] | [prev] | [next] | [standalone]

#1401444

On Fri, May 13, 2016 at 10:58:05AM -0700, Peter Hurley wrote:
> > Note that barrier() and READ_ONCE() have overlapping but not identical
> > results and the combined use actually makes sense here.
> > 
> > Yes, a barrier() anywhere in the loop will force a reload of the
> > variable, _however_ it doesn't force that reload to not suffer from
> > load tearing.
> > 
> > Using volatile also forces a reload, but also ensures the load cannot
> > be torn IFF it is of machine word side and naturally aligned.
> > 
> > So while the READ_ONCE() here is pointless for forcing the reload;
> > that's already ensured, we still need to make sure the load isn't torn.
> 
> If load tearing a naturally aligned pointer is a real code generation
> possibility then the rcu list code is broken too (which loads ->next
> directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).
> 
> For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc
> that had to do with control dependencies and not load tearing.

Well, Paul is the one who started the whole load/store tearing thing, so
I suppose he knows what he's doing.

That said; its a fairly recent as things go so lots of code hasn't been
updated yet, and its also a very unlikely thing for a compiler to do;
since it mostly doesn't make sense to emit multiple instructions where
one will do, so its not a very high priority thing either.

But from what I understand, the compiler is free to emit all kinds of
nonsense for !volatile loads/stores.

> OTOH, this patch might actually produce store-tearing:
> 
> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
> +{
> +	/*
> +	 * We check the owner value first to make sure that we will only
> +	 * do a write to the rwsem cacheline when it is really necessary
> +	 * to minimize cacheline contention.
> +	 */
> +	if (sem->owner != RWSEM_READER_OWNED)
> +		sem->owner = RWSEM_READER_OWNED;
> +}

Correct; which is why we should always use {READ,WRITE}_ONCE() for
anything that is used locklessly.

[toc] | [prev] | [next] | [standalone]

#1401468

On Mon, May 16, 2016 at 01:09:48PM +0200, Peter Zijlstra wrote:
> On Fri, May 13, 2016 at 10:58:05AM -0700, Peter Hurley wrote:
> > > Note that barrier() and READ_ONCE() have overlapping but not identical
> > > results and the combined use actually makes sense here.
> > > 
> > > Yes, a barrier() anywhere in the loop will force a reload of the
> > > variable, _however_ it doesn't force that reload to not suffer from
> > > load tearing.
> > > 
> > > Using volatile also forces a reload, but also ensures the load cannot
> > > be torn IFF it is of machine word side and naturally aligned.
> > > 
> > > So while the READ_ONCE() here is pointless for forcing the reload;
> > > that's already ensured, we still need to make sure the load isn't torn.
> > 
> > If load tearing a naturally aligned pointer is a real code generation
> > possibility then the rcu list code is broken too (which loads ->next
> > directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).
> > 
> > For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc
> > that had to do with control dependencies and not load tearing.
> 
> Well, Paul is the one who started the whole load/store tearing thing, so
> I suppose he knows what he's doing.

That had to do with suppressing false positives for one of Dmitry
Vjukov's concurrency checkers.  I suspect that Peter Hurley is right
that continued use of that checker would identify other places needing
READ_ONCE(), but from what I understand that is on hold pending a formal
definition of the Linux-kernel memory model.  (KCC and Dmitry (CCed)
can correct my if I am confused on this point.)

> That said; its a fairly recent as things go so lots of code hasn't been
> updated yet, and its also a very unlikely thing for a compiler to do;
> since it mostly doesn't make sense to emit multiple instructions where
> one will do, so its not a very high priority thing either.
> 
> But from what I understand, the compiler is free to emit all kinds of
> nonsense for !volatile loads/stores.

That is quite true.  :-/

> > OTOH, this patch might actually produce store-tearing:
> > 
> > +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
> > +{
> > +	/*
> > +	 * We check the owner value first to make sure that we will only
> > +	 * do a write to the rwsem cacheline when it is really necessary
> > +	 * to minimize cacheline contention.
> > +	 */
> > +	if (sem->owner != RWSEM_READER_OWNED)
> > +		sem->owner = RWSEM_READER_OWNED;
> > +}
> 
> Correct; which is why we should always use {READ,WRITE}_ONCE() for
> anything that is used locklessly.

Completely agreed.  Improve readability of code by flagging lockless
shared-memory accesses, help checkers better find bugs, and prevent the
occasional compiler mischief!

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1401523

On 05/16/2016 05:17 AM, Paul E. McKenney wrote:
> On Mon, May 16, 2016 at 01:09:48PM +0200, Peter Zijlstra wrote:
>> On Fri, May 13, 2016 at 10:58:05AM -0700, Peter Hurley wrote:
>>>> Note that barrier() and READ_ONCE() have overlapping but not identical
>>>> results and the combined use actually makes sense here.
>>>>
>>>> Yes, a barrier() anywhere in the loop will force a reload of the
>>>> variable, _however_ it doesn't force that reload to not suffer from
>>>> load tearing.
>>>>
>>>> Using volatile also forces a reload, but also ensures the load cannot
>>>> be torn IFF it is of machine word side and naturally aligned.
>>>>
>>>> So while the READ_ONCE() here is pointless for forcing the reload;
>>>> that's already ensured, we still need to make sure the load isn't torn.
>>>
>>> If load tearing a naturally aligned pointer is a real code generation
>>> possibility then the rcu list code is broken too (which loads ->next
>>> directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).
>>>
>>> For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc
>>> that had to do with control dependencies and not load tearing.
>>
>> Well, Paul is the one who started the whole load/store tearing thing, so
>> I suppose he knows what he's doing.
> 
> That had to do with suppressing false positives for one of Dmitry
> Vjukov's concurrency checkers.  I suspect that Peter Hurley is right
> that continued use of that checker would identify other places needing
> READ_ONCE(), but from what I understand that is on hold pending a formal
> definition of the Linux-kernel memory model.  (KCC and Dmitry (CCed)
> can correct my if I am confused on this point.)
> 
>> That said; its a fairly recent as things go so lots of code hasn't been
>> updated yet, and its also a very unlikely thing for a compiler to do;
>> since it mostly doesn't make sense to emit multiple instructions where
>> one will do, so its not a very high priority thing either.
>>
>> But from what I understand, the compiler is free to emit all kinds of
>> nonsense for !volatile loads/stores.
> 
> That is quite true.  :-/
> 
>>> OTOH, this patch might actually produce store-tearing:
>>>
>>> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
>>> +{
>>> +	/*
>>> +	 * We check the owner value first to make sure that we will only
>>> +	 * do a write to the rwsem cacheline when it is really necessary
>>> +	 * to minimize cacheline contention.
>>> +	 */
>>> +	if (sem->owner != RWSEM_READER_OWNED)
>>> +		sem->owner = RWSEM_READER_OWNED;
>>> +}
>>
>> Correct; which is why we should always use {READ,WRITE}_ONCE() for
>> anything that is used locklessly.
> 
> Completely agreed.  Improve readability of code by flagging lockless
> shared-memory accesses, help checkers better find bugs, and prevent the
> occasional compiler mischief!

I think this would be a mistake for 3 reasons:

1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing
   of any normally-atomic type (char/int/long/void*), then _every_ access
   would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility
   of compiler optimization (eg. eliding redundant loads) where it would
   otherwise be possible.

2. Makes a mess of otherwise readable code.

3. Error-prone; ie., easy to overlook in review.

There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE()
(to prevent tearing) and declaring the field volatile.

So we've come full-circle from volatile-considered-harmful.

Regards,
Peter Hurley

[toc] | [prev] | [next] | [standalone]

#1401654

On Mon, May 16, 2016 at 07:17:42AM -0700, Peter Hurley wrote:
> On 05/16/2016 05:17 AM, Paul E. McKenney wrote:
> > On Mon, May 16, 2016 at 01:09:48PM +0200, Peter Zijlstra wrote:
> >> On Fri, May 13, 2016 at 10:58:05AM -0700, Peter Hurley wrote:
> >>>> Note that barrier() and READ_ONCE() have overlapping but not identical
> >>>> results and the combined use actually makes sense here.
> >>>>
> >>>> Yes, a barrier() anywhere in the loop will force a reload of the
> >>>> variable, _however_ it doesn't force that reload to not suffer from
> >>>> load tearing.
> >>>>
> >>>> Using volatile also forces a reload, but also ensures the load cannot
> >>>> be torn IFF it is of machine word side and naturally aligned.
> >>>>
> >>>> So while the READ_ONCE() here is pointless for forcing the reload;
> >>>> that's already ensured, we still need to make sure the load isn't torn.
> >>>
> >>> If load tearing a naturally aligned pointer is a real code generation
> >>> possibility then the rcu list code is broken too (which loads ->next
> >>> directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).
> >>>
> >>> For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc
> >>> that had to do with control dependencies and not load tearing.
> >>
> >> Well, Paul is the one who started the whole load/store tearing thing, so
> >> I suppose he knows what he's doing.
> > 
> > That had to do with suppressing false positives for one of Dmitry
> > Vjukov's concurrency checkers.  I suspect that Peter Hurley is right
> > that continued use of that checker would identify other places needing
> > READ_ONCE(), but from what I understand that is on hold pending a formal
> > definition of the Linux-kernel memory model.  (KCC and Dmitry (CCed)
> > can correct my if I am confused on this point.)
> > 
> >> That said; its a fairly recent as things go so lots of code hasn't been
> >> updated yet, and its also a very unlikely thing for a compiler to do;
> >> since it mostly doesn't make sense to emit multiple instructions where
> >> one will do, so its not a very high priority thing either.
> >>
> >> But from what I understand, the compiler is free to emit all kinds of
> >> nonsense for !volatile loads/stores.
> > 
> > That is quite true.  :-/
> > 
> >>> OTOH, this patch might actually produce store-tearing:
> >>>
> >>> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
> >>> +{
> >>> +	/*
> >>> +	 * We check the owner value first to make sure that we will only
> >>> +	 * do a write to the rwsem cacheline when it is really necessary
> >>> +	 * to minimize cacheline contention.
> >>> +	 */
> >>> +	if (sem->owner != RWSEM_READER_OWNED)
> >>> +		sem->owner = RWSEM_READER_OWNED;
> >>> +}
> >>
> >> Correct; which is why we should always use {READ,WRITE}_ONCE() for
> >> anything that is used locklessly.
> > 
> > Completely agreed.  Improve readability of code by flagging lockless
> > shared-memory accesses, help checkers better find bugs, and prevent the
> > occasional compiler mischief!
> 
> I think this would be a mistake for 3 reasons:
> 
> 1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing
>    of any normally-atomic type (char/int/long/void*), then _every_ access
>    would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility
>    of compiler optimization (eg. eliding redundant loads) where it would
>    otherwise be possible.

The point about eliding redundant loads is a good one, at least in those
cases where it is a reasonable optimization.  Should we ever get to a
point where we no longer use pre-C11 compilers, those use cases could
potentially use memory_order_relaxed loads.  Preferably wrappered in
something that can be typed with fewer characters.  And it could of course
lead to an interesting discussion of what use cases would be required
to justify this change, but what else is new?

> 2. Makes a mess of otherwise readable code.
> 
> 3. Error-prone; ie., easy to overlook in review.

But #2 and #3 are at odds with each other.  It is all too easy to miss a
critically important load or store that has not been flagged in some way.
So #2's readable code can easily be problematic, as the concurrency is
hidden from both the compiler and the poor developer reading the code.

> There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE()
> (to prevent tearing) and declaring the field volatile.

Actually, yes there is a difference.  If you hold the update-side lock,
you don't have to use READ_ONCE() when reading the variable.  If you
have further excluded readers (for example, at initialization time or
at teardown time), then you don't have to use either READ_ONCE() or
WRITE_ONCE().

> So we've come full-circle from volatile-considered-harmful.

Not really.  We are (hopefully) using volatile for jobs that it can do.
In contrast, in the past people were expecting it to do more than it
reasonably can do.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1402555

On 05/16/2016 10:22 AM, Paul E. McKenney wrote:
> On Mon, May 16, 2016 at 07:17:42AM -0700, Peter Hurley wrote:
>> On 05/16/2016 05:17 AM, Paul E. McKenney wrote:
>>> On Mon, May 16, 2016 at 01:09:48PM +0200, Peter Zijlstra wrote:
>>>> On Fri, May 13, 2016 at 10:58:05AM -0700, Peter Hurley wrote:
>>>>>> Note that barrier() and READ_ONCE() have overlapping but not identical
>>>>>> results and the combined use actually makes sense here.
>>>>>>
>>>>>> Yes, a barrier() anywhere in the loop will force a reload of the
>>>>>> variable, _however_ it doesn't force that reload to not suffer from
>>>>>> load tearing.
>>>>>>
>>>>>> Using volatile also forces a reload, but also ensures the load cannot
>>>>>> be torn IFF it is of machine word side and naturally aligned.
>>>>>>
>>>>>> So while the READ_ONCE() here is pointless for forcing the reload;
>>>>>> that's already ensured, we still need to make sure the load isn't torn.
>>>>>
>>>>> If load tearing a naturally aligned pointer is a real code generation
>>>>> possibility then the rcu list code is broken too (which loads ->next
>>>>> directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).
>>>>>
>>>>> For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc
>>>>> that had to do with control dependencies and not load tearing.
>>>>
>>>> Well, Paul is the one who started the whole load/store tearing thing, so
>>>> I suppose he knows what he's doing.
>>>
>>> That had to do with suppressing false positives for one of Dmitry
>>> Vjukov's concurrency checkers.  I suspect that Peter Hurley is right
>>> that continued use of that checker would identify other places needing
>>> READ_ONCE(), but from what I understand that is on hold pending a formal
>>> definition of the Linux-kernel memory model.  (KCC and Dmitry (CCed)
>>> can correct my if I am confused on this point.)
>>>
>>>> That said; its a fairly recent as things go so lots of code hasn't been
>>>> updated yet, and its also a very unlikely thing for a compiler to do;
>>>> since it mostly doesn't make sense to emit multiple instructions where
>>>> one will do, so its not a very high priority thing either.
>>>>
>>>> But from what I understand, the compiler is free to emit all kinds of
>>>> nonsense for !volatile loads/stores.
>>>
>>> That is quite true.  :-/
>>>
>>>>> OTOH, this patch might actually produce store-tearing:
>>>>>
>>>>> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
>>>>> +{
>>>>> +	/*
>>>>> +	 * We check the owner value first to make sure that we will only
>>>>> +	 * do a write to the rwsem cacheline when it is really necessary
>>>>> +	 * to minimize cacheline contention.
>>>>> +	 */
>>>>> +	if (sem->owner != RWSEM_READER_OWNED)
>>>>> +		sem->owner = RWSEM_READER_OWNED;
>>>>> +}
>>>>
>>>> Correct; which is why we should always use {READ,WRITE}_ONCE() for
>>>> anything that is used locklessly.
>>>
>>> Completely agreed.  Improve readability of code by flagging lockless
>>> shared-memory accesses, help checkers better find bugs, and prevent the
>>> occasional compiler mischief!
>>
>> I think this would be a mistake for 3 reasons:
>>
>> 1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing
>>    of any normally-atomic type (char/int/long/void*), then _every_ access
>>    would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility
>>    of compiler optimization (eg. eliding redundant loads) where it would
>>    otherwise be possible.
> 
> The point about eliding redundant loads is a good one, at least in those
> cases where it is a reasonable optimization.  Should we ever get to a
> point where we no longer use pre-C11 compilers, those use cases could
> potentially use memory_order_relaxed loads.  Preferably wrappered in
> something that can be typed with fewer characters.  And it could of course
> lead to an interesting discussion of what use cases would be required
> to justify this change, but what else is new?

I believe lockless access is quite widespread in the kernel, and this
use was based on the previous assumption that loads/stores to
char/short/int/long/void* are atomic, which is generally safe in the
absence of specific circumstances which may cause load- or store-tearing
(are there others besides immediate stores and packed structures?).

So I think it makes more sense to annotate usage that prevents load-
and store-tearing, separately from the forceably load/store READ_ONCE/
WRITE_ONCE macros.


>> 2. Makes a mess of otherwise readable code.
>>
>> 3. Error-prone; ie., easy to overlook in review.
> 
> But #2 and #3 are at odds with each other.  It is all too easy to miss a
> critically important load or store that has not been flagged in some way.
> So #2's readable code can easily be problematic, as the concurrency is
> hidden from both the compiler and the poor developer reading the code.

Not for the purpose of preventing load- and store-tearing; ie., the
vast majority of lockless use now.


>> There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE()
>> (to prevent tearing) and declaring the field volatile.
> 
> Actually, yes there is a difference.  If you hold the update-side lock,
> you don't have to use READ_ONCE() when reading the variable.  If you
> have further excluded readers (for example, at initialization time or
> at teardown time), then you don't have to use either READ_ONCE() or
> WRITE_ONCE().

This cuts both ways; on the one hand, you're saying using volatile modifier
doesn't let us control every use case, and on the other hand, we're adding
volatile access to list primitives that we _know_ are both frequently
used and in update-side locks. Where's the win?


>> So we've come full-circle from volatile-considered-harmful.
> 
> Not really.  We are (hopefully) using volatile for jobs that it can do.
> In contrast, in the past people were expecting it to do more than it
> reasonably can do.

Well, I wasn't referring to the never-did-work ideas and more about
the example I quoted from that document about cpu_relax() being a barrier.

Regards,
Peter Hurley

[toc] | [prev] | [next] | [standalone]

#1402565

Hi Paul,

You can disregard this as I think we're talking about
the same things with the other email thread.

Regards,
Peter Hurley


On 05/17/2016 12:46 PM, Peter Hurley wrote:
> On 05/16/2016 10:22 AM, Paul E. McKenney wrote:
>> On Mon, May 16, 2016 at 07:17:42AM -0700, Peter Hurley wrote:
>>> On 05/16/2016 05:17 AM, Paul E. McKenney wrote:
>>>> On Mon, May 16, 2016 at 01:09:48PM +0200, Peter Zijlstra wrote:
>>>>> On Fri, May 13, 2016 at 10:58:05AM -0700, Peter Hurley wrote:
>>>>>>> Note that barrier() and READ_ONCE() have overlapping but not identical
>>>>>>> results and the combined use actually makes sense here.
>>>>>>>
>>>>>>> Yes, a barrier() anywhere in the loop will force a reload of the
>>>>>>> variable, _however_ it doesn't force that reload to not suffer from
>>>>>>> load tearing.
>>>>>>>
>>>>>>> Using volatile also forces a reload, but also ensures the load cannot
>>>>>>> be torn IFF it is of machine word side and naturally aligned.
>>>>>>>
>>>>>>> So while the READ_ONCE() here is pointless for forcing the reload;
>>>>>>> that's already ensured, we still need to make sure the load isn't torn.
>>>>>>
>>>>>> If load tearing a naturally aligned pointer is a real code generation
>>>>>> possibility then the rcu list code is broken too (which loads ->next
>>>>>> directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()).
>>>>>>
>>>>>> For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc
>>>>>> that had to do with control dependencies and not load tearing.
>>>>>
>>>>> Well, Paul is the one who started the whole load/store tearing thing, so
>>>>> I suppose he knows what he's doing.
>>>>
>>>> That had to do with suppressing false positives for one of Dmitry
>>>> Vjukov's concurrency checkers.  I suspect that Peter Hurley is right
>>>> that continued use of that checker would identify other places needing
>>>> READ_ONCE(), but from what I understand that is on hold pending a formal
>>>> definition of the Linux-kernel memory model.  (KCC and Dmitry (CCed)
>>>> can correct my if I am confused on this point.)
>>>>
>>>>> That said; its a fairly recent as things go so lots of code hasn't been
>>>>> updated yet, and its also a very unlikely thing for a compiler to do;
>>>>> since it mostly doesn't make sense to emit multiple instructions where
>>>>> one will do, so its not a very high priority thing either.
>>>>>
>>>>> But from what I understand, the compiler is free to emit all kinds of
>>>>> nonsense for !volatile loads/stores.
>>>>
>>>> That is quite true.  :-/
>>>>
>>>>>> OTOH, this patch might actually produce store-tearing:
>>>>>>
>>>>>> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
>>>>>> +{
>>>>>> +	/*
>>>>>> +	 * We check the owner value first to make sure that we will only
>>>>>> +	 * do a write to the rwsem cacheline when it is really necessary
>>>>>> +	 * to minimize cacheline contention.
>>>>>> +	 */
>>>>>> +	if (sem->owner != RWSEM_READER_OWNED)
>>>>>> +		sem->owner = RWSEM_READER_OWNED;
>>>>>> +}
>>>>>
>>>>> Correct; which is why we should always use {READ,WRITE}_ONCE() for
>>>>> anything that is used locklessly.
>>>>
>>>> Completely agreed.  Improve readability of code by flagging lockless
>>>> shared-memory accesses, help checkers better find bugs, and prevent the
>>>> occasional compiler mischief!
>>>
>>> I think this would be a mistake for 3 reasons:
>>>
>>> 1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing
>>>    of any normally-atomic type (char/int/long/void*), then _every_ access
>>>    would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility
>>>    of compiler optimization (eg. eliding redundant loads) where it would
>>>    otherwise be possible.
>>
>> The point about eliding redundant loads is a good one, at least in those
>> cases where it is a reasonable optimization.  Should we ever get to a
>> point where we no longer use pre-C11 compilers, those use cases could
>> potentially use memory_order_relaxed loads.  Preferably wrappered in
>> something that can be typed with fewer characters.  And it could of course
>> lead to an interesting discussion of what use cases would be required
>> to justify this change, but what else is new?
> 
> I believe lockless access is quite widespread in the kernel, and this
> use was based on the previous assumption that loads/stores to
> char/short/int/long/void* are atomic, which is generally safe in the
> absence of specific circumstances which may cause load- or store-tearing
> (are there others besides immediate stores and packed structures?).
> 
> So I think it makes more sense to annotate usage that prevents load-
> and store-tearing, separately from the forceably load/store READ_ONCE/
> WRITE_ONCE macros.
> 
> 
>>> 2. Makes a mess of otherwise readable code.
>>>
>>> 3. Error-prone; ie., easy to overlook in review.
>>
>> But #2 and #3 are at odds with each other.  It is all too easy to miss a
>> critically important load or store that has not been flagged in some way.
>> So #2's readable code can easily be problematic, as the concurrency is
>> hidden from both the compiler and the poor developer reading the code.
> 
> Not for the purpose of preventing load- and store-tearing; ie., the
> vast majority of lockless use now.
> 
> 
>>> There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE()
>>> (to prevent tearing) and declaring the field volatile.
>>
>> Actually, yes there is a difference.  If you hold the update-side lock,
>> you don't have to use READ_ONCE() when reading the variable.  If you
>> have further excluded readers (for example, at initialization time or
>> at teardown time), then you don't have to use either READ_ONCE() or
>> WRITE_ONCE().
> 
> This cuts both ways; on the one hand, you're saying using volatile modifier
> doesn't let us control every use case, and on the other hand, we're adding
> volatile access to list primitives that we _know_ are both frequently
> used and in update-side locks. Where's the win?
> 
> 
>>> So we've come full-circle from volatile-considered-harmful.
>>
>> Not really.  We are (hopefully) using volatile for jobs that it can do.
>> In contrast, in the past people were expecting it to do more than it
>> reasonably can do.
> 
> Well, I wasn't referring to the never-did-work ideas and more about
> the example I quoted from that document about cpu_relax() being a barrier.
> 
> Regards,
> Peter Hurley
> 
> 

[toc] | [prev] | [next] | [standalone]

#1401669

On Mon, May 16, 2016 at 07:17:42AM -0700, Peter Hurley wrote:

> >> Correct; which is why we should always use {READ,WRITE}_ONCE() for
> >> anything that is used locklessly.
> > 
> > Completely agreed.  Improve readability of code by flagging lockless
> > shared-memory accesses, help checkers better find bugs, and prevent the
> > occasional compiler mischief!
> 
> I think this would be a mistake for 3 reasons:
> 
> 1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing
>    of any normally-atomic type (char/int/long/void*), then _every_ access
>    would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility
>    of compiler optimization (eg. eliding redundant loads) where it would
>    otherwise be possible.

Should not really be a problem I think; you already have to be very
careful when doing lockless stuff.

> 2. Makes a mess of otherwise readable code.

We have to disagree here; I think code with {READ,WRITE}_ONCE() is more
readable, as their presence is a clear indication something special is
up.

> 3. Error-prone; ie., easy to overlook in review.

lockless stuff is error prone by nature; what's your point?

> There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE()
> (to prevent tearing) and declaring the field volatile.

There is, declaring the field volatile doesn't make it stand out in the
code while reading at all; it also doesn't allow you to omit the
volatile qualifier in places where it doesn't matter.

The whole; variables aren't volatile, accesses are, thing is still very
much in effect.

> So we've come full-circle from volatile-considered-harmful.

I don't think so; the cases that document talks about are still very
much relevant and volatile should not be used for that.

But yes, our understanding of both the memory model and the
language/compiler has come a long way since then.

[toc] | [prev] | [next] | [standalone]

#1402542

On 05/16/2016 10:50 AM, Peter Zijlstra wrote:
> On Mon, May 16, 2016 at 07:17:42AM -0700, Peter Hurley wrote:
> 
>>>> Correct; which is why we should always use {READ,WRITE}_ONCE() for
>>>> anything that is used locklessly.
>>>
>>> Completely agreed.  Improve readability of code by flagging lockless
>>> shared-memory accesses, help checkers better find bugs, and prevent the
>>> occasional compiler mischief!
>>
>> I think this would be a mistake for 3 reasons:
>>
>> 1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing
>>    of any normally-atomic type (char/int/long/void*), then _every_ access
>>    would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility
>>    of compiler optimization (eg. eliding redundant loads) where it would
>>    otherwise be possible.
> 
> Should not really be a problem I think; you already have to be very
> careful when doing lockless stuff.

I think you may not understand my point here.

Consider normal list processing.

Paul added a READ_ONCE() load of head->next to list_empty().
That's because list_empty() is being used locklessly in lots of places,
not just for RCU lists.

Ok, so the load won't be torn. But the store to ->next can, so Paul
added WRITE_ONCE() to some but not all those list primitives too, even
though those aren't being used locklessly.

Note that the compiler _cannot_ optimize those stores even though
they're being used with locks held; IOW, exactly the situation that
volatile-consider-harmful rails against.

Similarly for the load in list_empty() even if it's used with locks held.

As Paul pointed out in his reply, there is no way to address this right now.

What's really needed is separate, not overloaded semantics:

1. "*If* you load or store this value, do it with single memory access, but
    feel free to optimize (elide load/defer store/hoist load/whatever)."

2. "No, seriously, load/store this value regardless of what you think you
    know." == READ_ONCE/WRITE_ONCE


If we start adding READ_ONCE/WRITE_ONCE to every lockless load/store,
worse code will be generated even though the vast majority of current use
is safe.


>> 2. Makes a mess of otherwise readable code.
> 
> We have to disagree here; I think code with {READ,WRITE}_ONCE() is more
> readable, as their presence is a clear indication something special is
> up.

I think you and Paul may wildly underestimate the scale of lockless
use in the kernel not currently annotated by READ_ONCE()/WRITE_ONCE().

Even in primitives designed for lockless concurrency like
rwsem and seqlock and sched/core, much less higher order code like
ipc/msg.c and vfs.

The majority of this lockless use is safe if the assumption
that loads/stores are performed atomically for char/short/int/long/void*
hold; in other words usage #1 above.


>> 3. Error-prone; ie., easy to overlook in review.
> 
> lockless stuff is error prone by nature; what's your point?

That lockless use is very common, even outside core functionality, and
needlessly splattering READ_ONCE/WRITE_ONCE when the existing code
generation is already safe, will not make it better. And that it will be
no safer by adding READ_ONCE/WRITE_ONCE because plenty of accesses will
be overlooked, like the stores to sem->owner are now.


>> There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE()
>> (to prevent tearing) and declaring the field volatile.
> 
> There is, declaring the field volatile doesn't make it stand out in the
> code while reading at all; it also doesn't allow you to omit the
> volatile qualifier in places where it doesn't matter.
> 
> The whole; variables aren't volatile, accesses are, thing is still very
> much in effect.

I'm not really seriously arguing for volatile declarations; I'm pointing
out that READ_ONCE()/WRITE_ONCE() has overloaded meaning that is
counter-productive.

For example, you point out that it doesn't allow you to omit the
volatile qualifier but yet we're adding it to underlying primitives
that may or may not be used locklessly.

Guaranteed list_empty() and list_add() are used way more than INIT_LIST_HEAD().


>> So we've come full-circle from volatile-considered-harmful.
> 
> I don't think so; the cases that document talks about are still very
> much relevant and volatile should not be used for that.

The example below is clearly wrong now.

	"Another situation where one might be tempted to use volatile is
	when the processor is busy-waiting on the value of a variable.  The right
	way to perform a busy wait is:

	    while (my_variable != what_i_want)
        	cpu_relax();

	The cpu_relax() call can lower CPU power consumption or yield to a
	hyperthreaded twin processor; it also happens to serve as a compiler
	barrier, so, once again, volatile is unnecessary.  Of course, busy-
	waiting is generally an anti-social act to begin with."


The fact that cpu_relax() is a barrier is immaterial; the load of my_variable
must now be READ_ONCE() to prevent load-tearing.

Likewise, whatever is writing to 'my_variable' must now be WRITE_ONCE().

Regards,
Peter Hurley

[toc] | [prev] | [next] | [standalone]

#1402553

On Tue, May 17, 2016 at 12:15:20PM -0700, Peter Hurley wrote:
> On 05/16/2016 10:50 AM, Peter Zijlstra wrote:
> > On Mon, May 16, 2016 at 07:17:42AM -0700, Peter Hurley wrote:
> > 
> >>>> Correct; which is why we should always use {READ,WRITE}_ONCE() for
> >>>> anything that is used locklessly.
> >>>
> >>> Completely agreed.  Improve readability of code by flagging lockless
> >>> shared-memory accesses, help checkers better find bugs, and prevent the
> >>> occasional compiler mischief!
> >>
> >> I think this would be a mistake for 3 reasons:
> >>
> >> 1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing
> >>    of any normally-atomic type (char/int/long/void*), then _every_ access
> >>    would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility
> >>    of compiler optimization (eg. eliding redundant loads) where it would
> >>    otherwise be possible.
> > 
> > Should not really be a problem I think; you already have to be very
> > careful when doing lockless stuff.
> 
> I think you may not understand my point here.
> 
> Consider normal list processing.
> 
> Paul added a READ_ONCE() load of head->next to list_empty().
> That's because list_empty() is being used locklessly in lots of places,
> not just for RCU lists.
> 
> Ok, so the load won't be torn. But the store to ->next can, so Paul
> added WRITE_ONCE() to some but not all those list primitives too, even
> though those aren't being used locklessly.
> 
> Note that the compiler _cannot_ optimize those stores even though
> they're being used with locks held; IOW, exactly the situation that
> volatile-consider-harmful rails against.
> 
> Similarly for the load in list_empty() even if it's used with locks held.
> 
> As Paul pointed out in his reply, there is no way to address this right now.

Actually, if you show a case where this makes a visible system-wide
difference, you could create a set of primitives for #1 below.  Have
a compiler version check, and if it is an old compiler, map them to
READ_ONCE() and WRITE_ONCE(), otherwise as follows, though preferably
with better names:

#define READ_NOTEAR(x) __atomic_load_n(&(x), __ATOMIC_RELAXED)
#define WRITE_NOTEAR(x, v) __atomic_store_n(&(x), (v), __ATOMIC_RELAXED)

The ambiguity between "no tear" and "not ear" should help motivate a
better choice of name.

My guess is that the best justification for adding these will come
from the TINY effort.  My addition of ATOMIC_READ() and ATOMIC_WRITE()
did increase the size.  But perhaps there is a benchmark that can see
the difference in performance, response time, or what have you.

The theoreticians won't like it much, as this will introduce the
theoretical possibility of "out of thin air" values, but then again,
there are other theoreticians working on this problem.

> What's really needed is separate, not overloaded semantics:
> 
> 1. "*If* you load or store this value, do it with single memory access, but
>     feel free to optimize (elide load/defer store/hoist load/whatever)."
> 
> 2. "No, seriously, load/store this value regardless of what you think you
>     know." == READ_ONCE/WRITE_ONCE
> 
> 
> If we start adding READ_ONCE/WRITE_ONCE to every lockless load/store,
> worse code will be generated even though the vast majority of current use
> is safe.

There will indeed be some degradation in theory.  I would be surprised
if it is visible at the system level, aside from tiny kernels.  That said,
I have been surprised before.

> >> 2. Makes a mess of otherwise readable code.
> > 
> > We have to disagree here; I think code with {READ,WRITE}_ONCE() is more
> > readable, as their presence is a clear indication something special is
> > up.
> 
> I think you and Paul may wildly underestimate the scale of lockless
> use in the kernel not currently annotated by READ_ONCE()/WRITE_ONCE().

Believe me, I know that READ_ONCE() and WRITE_ONCE() usage would need
to increase by -at- -least- an order of magnitude in order to cover
the cases.  Many of the might be harmless with current compilers, but
compilers have had a habit of getting more aggressive over time.

> Even in primitives designed for lockless concurrency like
> rwsem and seqlock and sched/core, much less higher order code like
> ipc/msg.c and vfs.
> 
> The majority of this lockless use is safe if the assumption
> that loads/stores are performed atomically for char/short/int/long/void*
> hold; in other words usage #1 above.
> 
> 
> >> 3. Error-prone; ie., easy to overlook in review.
> > 
> > lockless stuff is error prone by nature; what's your point?
> 
> That lockless use is very common, even outside core functionality, and
> needlessly splattering READ_ONCE/WRITE_ONCE when the existing code
> generation is already safe, will not make it better. And that it will be
> no safer by adding READ_ONCE/WRITE_ONCE because plenty of accesses will
> be overlooked, like the stores to sem->owner are now.

I expect mechanical aids will help locate the overlooked accesses.

> >> There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE()
> >> (to prevent tearing) and declaring the field volatile.
> > 
> > There is, declaring the field volatile doesn't make it stand out in the
> > code while reading at all; it also doesn't allow you to omit the
> > volatile qualifier in places where it doesn't matter.
> > 
> > The whole; variables aren't volatile, accesses are, thing is still very
> > much in effect.
> 
> I'm not really seriously arguing for volatile declarations; I'm pointing
> out that READ_ONCE()/WRITE_ONCE() has overloaded meaning that is
> counter-productive.
> 
> For example, you point out that it doesn't allow you to omit the
> volatile qualifier but yet we're adding it to underlying primitives
> that may or may not be used locklessly.
> 
> Guaranteed list_empty() and list_add() are used way more than INIT_LIST_HEAD().

No argument on usage frequency, just a question on visible effects.

> >> So we've come full-circle from volatile-considered-harmful.
> > 
> > I don't think so; the cases that document talks about are still very
> > much relevant and volatile should not be used for that.
> 
> The example below is clearly wrong now.
> 
> 	"Another situation where one might be tempted to use volatile is
> 	when the processor is busy-waiting on the value of a variable.  The right
> 	way to perform a busy wait is:
> 
> 	    while (my_variable != what_i_want)
>         	cpu_relax();
> 
> 	The cpu_relax() call can lower CPU power consumption or yield to a
> 	hyperthreaded twin processor; it also happens to serve as a compiler
> 	barrier, so, once again, volatile is unnecessary.  Of course, busy-
> 	waiting is generally an anti-social act to begin with."
> 
> 
> The fact that cpu_relax() is a barrier is immaterial; the load of my_variable
> must now be READ_ONCE() to prevent load-tearing.
> 
> Likewise, whatever is writing to 'my_variable' must now be WRITE_ONCE().

Yow!  That document has not been changed since 2010.  I bet that there
is much else in it that needs a bit of help.  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1402892

On Tue, May 17, 2016 at 12:46:07PM -0700, Paul E. McKenney wrote:
> Actually, if you show a case where this makes a visible system-wide
> difference, you could create a set of primitives for #1 below.  Have
> a compiler version check, and if it is an old compiler, map them to
> READ_ONCE() and WRITE_ONCE(), otherwise as follows, though preferably
> with better names:
> 
> #define READ_NOTEAR(x) __atomic_load_n(&(x), __ATOMIC_RELAXED)
> #define WRITE_NOTEAR(x, v) __atomic_store_n(&(x), (v), __ATOMIC_RELAXED)
> 
> The ambiguity between "no tear" and "not ear" should help motivate a
> better choice of name.

Alternatively, could we try and talk to our GCC friends to make sure GCC
doesn't tear loads/stores irrespective of what the C language spec
allows?

[toc] | [prev] | [next] | [standalone]

#1403113

On 05/18/2016 07:05 AM, Peter Zijlstra wrote:
> On Tue, May 17, 2016 at 12:46:07PM -0700, Paul E. McKenney wrote:
>> Actually, if you show a case where this makes a visible system-wide
>> difference, you could create a set of primitives for #1 below.  Have
>> a compiler version check, and if it is an old compiler, map them to
>> READ_ONCE() and WRITE_ONCE(), otherwise as follows, though preferably
>> with better names:
>>
>> #define READ_NOTEAR(x) __atomic_load_n(&(x), __ATOMIC_RELAXED)
>> #define WRITE_NOTEAR(x, v) __atomic_store_n(&(x), (v), __ATOMIC_RELAXED)
>>
>> The ambiguity between "no tear" and "not ear" should help motivate a
>> better choice of name.
> Alternatively, could we try and talk to our GCC friends to make sure GCC
> doesn't tear loads/stores irrespective of what the C language spec
> allows?
>
>

Maybe the GCC guys can define a tag which can be set in the variable or 
structure field declarations that those variables or field have to be 
read from or written to atomically. This can allow critical data that 
are used by multiple CPUs to be handled correctly while allowing 
compiler the freedom to do what it sees fit for the less critical data. 
This approach is also easier than looking for all the places where the 
data items are accessed and modifying them.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1403179

On Wed, May 18, 2016 at 11:56:39AM -0400, Waiman Long wrote:
> On 05/18/2016 07:05 AM, Peter Zijlstra wrote:
> >On Tue, May 17, 2016 at 12:46:07PM -0700, Paul E. McKenney wrote:
> >>Actually, if you show a case where this makes a visible system-wide
> >>difference, you could create a set of primitives for #1 below.  Have
> >>a compiler version check, and if it is an old compiler, map them to
> >>READ_ONCE() and WRITE_ONCE(), otherwise as follows, though preferably
> >>with better names:
> >>
> >>#define READ_NOTEAR(x) __atomic_load_n(&(x), __ATOMIC_RELAXED)
> >>#define WRITE_NOTEAR(x, v) __atomic_store_n(&(x), (v), __ATOMIC_RELAXED)
> >>
> >>The ambiguity between "no tear" and "not ear" should help motivate a
> >>better choice of name.
> >Alternatively, could we try and talk to our GCC friends to make sure GCC
> >doesn't tear loads/stores irrespective of what the C language spec
> >allows?
> 
> Maybe the GCC guys can define a tag which can be set in the variable
> or structure field declarations that those variables or field have
> to be read from or written to atomically. This can allow critical
> data that are used by multiple CPUs to be handled correctly while
> allowing compiler the freedom to do what it sees fit for the less
> critical data. This approach is also easier than looking for all the
> places where the data items are accessed and modifying them.

Having such a tag on gcc's internal data structures is what I want,
regardless of exactly how that tag gets there.

The sorts of things that I am especially concerned about are cases
where there is a union or other type-punning involved, and gcc has
the pieces of the desired location already in registers.  Of course,
these pieces came from separate accesses.  We really don't need this
kind of thing tripping us up!

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1403174

On Wed, May 18, 2016 at 01:05:55PM +0200, Peter Zijlstra wrote:
> On Tue, May 17, 2016 at 12:46:07PM -0700, Paul E. McKenney wrote:
> > Actually, if you show a case where this makes a visible system-wide
> > difference, you could create a set of primitives for #1 below.  Have
> > a compiler version check, and if it is an old compiler, map them to
> > READ_ONCE() and WRITE_ONCE(), otherwise as follows, though preferably
> > with better names:
> > 
> > #define READ_NOTEAR(x) __atomic_load_n(&(x), __ATOMIC_RELAXED)
> > #define WRITE_NOTEAR(x, v) __atomic_store_n(&(x), (v), __ATOMIC_RELAXED)
> > 
> > The ambiguity between "no tear" and "not ear" should help motivate a
> > better choice of name.
> 
> Alternatively, could we try and talk to our GCC friends to make sure GCC
> doesn't tear loads/stores irrespective of what the C language spec
> allows?

Interestingly enough, they used to make that guarantee, but removed it
when C11 showed up.

Me, I would feel better explicitly telling the compiler what I needed.
It is all too easy for bugs to slip in otherwise, especially when the
gcc guys are adding exciting new optimizations.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1403521

On Wed, May 18, 2016 at 10:26:06AM -0700, Paul E. McKenney wrote:
> On Wed, May 18, 2016 at 01:05:55PM +0200, Peter Zijlstra wrote:

> > Alternatively, could we try and talk to our GCC friends to make sure GCC
> > doesn't tear loads/stores irrespective of what the C language spec
> > allows?
> 
> Interestingly enough, they used to make that guarantee, but removed it
> when C11 showed up.

Did someone tell them this was a regression and have them fix it? They
can't just change things like this.

> Me, I would feel better explicitly telling the compiler what I needed.
> It is all too easy for bugs to slip in otherwise, especially when the
> gcc guys are adding exciting new optimizations.

GCC guys (as opposed to the language guys) should be far more amenable
to our needs, and I don't think they want to break the kernel any more
than we do.

[toc] | [prev] | [next] | [standalone]

#1403752

On Thu, May 19, 2016 at 11:00:13AM +0200, Peter Zijlstra wrote:
> On Wed, May 18, 2016 at 10:26:06AM -0700, Paul E. McKenney wrote:
> > On Wed, May 18, 2016 at 01:05:55PM +0200, Peter Zijlstra wrote:
> 
> > > Alternatively, could we try and talk to our GCC friends to make sure GCC
> > > doesn't tear loads/stores irrespective of what the C language spec
> > > allows?
> > 
> > Interestingly enough, they used to make that guarantee, but removed it
> > when C11 showed up.
> 
> Did someone tell them this was a regression and have them fix it? They
> can't just change things like this.

I did, informally.  I was told that the atomics were to replace them.
I have been bugging them about volatile ever since, given that some
people would dearly like to eliminate volatile from the language.  (I
believe I am making good progress on preventing this, with a lot of help
more recently.)

> > Me, I would feel better explicitly telling the compiler what I needed.
> > It is all too easy for bugs to slip in otherwise, especially when the
> > gcc guys are adding exciting new optimizations.
> 
> GCC guys (as opposed to the language guys) should be far more amenable
> to our needs, and I don't think they want to break the kernel any more
> than we do.

Some are, some aren't.  We should of course cherish the ones who would
like to avoid breaking the kernel.

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web