Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Waiman Long]

On 05/23/2016 02:46 PM, Jason Low wrote:

On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:

On 05/18/2016 12:58 PM, Jason Low wrote:

It should be fine to use the standard READ_ONCE here, even if it's just
for documentation, as it's probably not going to cost anything in
practice. It would be better to avoid adding any special macros for this
which may just add more complexity.

See, I don't understand this line of reasoning at all.

I read this as "it's ok to be non-optimal here where were spinning CPU
time but not ok to be non-optimal generally elsewhere where it's
way less important like at init time".

So I think there is a difference between using it during init time and
using it here where we're spinning. During init time, initializing the
owner field locklessly is normal. No other thread should be concurrently
be writing to the field, since the structure is just getting
initialized, so there are no surprises there.

Our access of the owner field in this function is special in that we're
using a bit of "lockless magic" to read and write to a field that gets
concurrently accessed without any serialization. Since we're not taking
the wait_lock in a scenario where we'd normally would take a lock, it
would be good to have this documented.


And by the way, it's not just "here" but _everywhere_.
What about reading ->on_cpu locklessly?

Sure, we could also use READ_ONCE when reading ->on_cpu  :)



As on_cpu is just a boolean, load tearing isn't really a problem. You 
either see the bit 0 set or not, but not something in between (not a 
qbit)  :-)


Cheers,
Longman

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Waiman Long]

On 05/23/2016 02:46 PM, Jason Low wrote:

On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:

On 05/18/2016 12:58 PM, Jason Low wrote:

It should be fine to use the standard READ_ONCE here, even if it's just
for documentation, as it's probably not going to cost anything in
practice. It would be better to avoid adding any special macros for this
which may just add more complexity.

See, I don't understand this line of reasoning at all.

I read this as "it's ok to be non-optimal here where were spinning CPU
time but not ok to be non-optimal generally elsewhere where it's
way less important like at init time".

So I think there is a difference between using it during init time and
using it here where we're spinning. During init time, initializing the
owner field locklessly is normal. No other thread should be concurrently
be writing to the field, since the structure is just getting
initialized, so there are no surprises there.

Our access of the owner field in this function is special in that we're
using a bit of "lockless magic" to read and write to a field that gets
concurrently accessed without any serialization. Since we're not taking
the wait_lock in a scenario where we'd normally would take a lock, it
would be good to have this documented.


And by the way, it's not just "here" but _everywhere_.
What about reading ->on_cpu locklessly?

Sure, we could also use READ_ONCE when reading ->on_cpu  :)



As on_cpu is just a boolean, load tearing isn't really a problem. You 
either see the bit 0 set or not, but not something in between (not a 
qbit)  :-)


Cheers,
Longman

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Davidlohr Bueso]

On Mon, 23 May 2016, Paul E. McKenney wrote:


But rcu_read_lock() does not exclude updates, which is one reason why
pointer reads use rcu_dereference() rather than normal assignments.


Yes, I was referring to readers. With updates, otoh, are done holding a number 
of locks.



So I do not believe that rcu_read_lock() is helping you in this case.

That said, it is a bit hard to imagine the compiler tearing a load from
an int...


Oh, right, good point.

Thanks,
Davidlohr

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Davidlohr Bueso]

On Mon, 23 May 2016, Paul E. McKenney wrote:


But rcu_read_lock() does not exclude updates, which is one reason why
pointer reads use rcu_dereference() rather than normal assignments.


Yes, I was referring to readers. With updates, otoh, are done holding a number 
of locks.



So I do not believe that rcu_read_lock() is helping you in this case.

That said, it is a bit hard to imagine the compiler tearing a load from
an int...


Oh, right, good point.

Thanks,
Davidlohr

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Paul E. McKenney]

On Mon, May 23, 2016 at 12:44:16PM -0700, Davidlohr Bueso wrote:
> On Mon, 23 May 2016, Jason Low wrote:
> 
> >On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:
> >>On 05/18/2016 12:58 PM, Jason Low wrote:
> >>> It should be fine to use the standard READ_ONCE here, even if it's just
> >>> for documentation, as it's probably not going to cost anything in
> >>> practice. It would be better to avoid adding any special macros for this
> >>> which may just add more complexity.
> >>
> >>See, I don't understand this line of reasoning at all.
> >>
> >>I read this as "it's ok to be non-optimal here where were spinning CPU
> >>time but not ok to be non-optimal generally elsewhere where it's
> >>way less important like at init time".
> >
> >So I think there is a difference between using it during init time and
> >using it here where we're spinning. During init time, initializing the
> >owner field locklessly is normal. No other thread should be concurrently
> >be writing to the field, since the structure is just getting
> >initialized, so there are no surprises there.
> >
> >Our access of the owner field in this function is special in that we're
> >using a bit of "lockless magic" to read and write to a field that gets
> >concurrently accessed without any serialization. Since we're not taking
> >the wait_lock in a scenario where we'd normally would take a lock, it
> >would be good to have this documented.
> >
> >>And by the way, it's not just "here" but _everywhere_.
> >>What about reading ->on_cpu locklessly?
> >
> >Sure, we could also use READ_ONCE when reading ->on_cpu  :)
> 
> Locking wise we should be covered with ->on_cpu as we're always under 
> rcu_read_lock
> (barrier, preempt_disable). But I'm not sure if this rule applies throughout 
> the
> scheduler, however, like it does in, say thread_group_cputime(). 
> cpu_clock_sample()
> (from posix timers) seems to mix and match being done under rcu. So 
> ultimately I
> think you're right.

But rcu_read_lock() does not exclude updates, which is one reason why
pointer reads use rcu_dereference() rather than normal assignments.

So I do not believe that rcu_read_lock() is helping you in this case.

That said, it is a bit hard to imagine the compiler tearing a load from
an int...  But compilers have uncovered weaknesses in my imagination
more than once in the past.

Thanx, Paul

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Paul E. McKenney]

On Mon, May 23, 2016 at 12:44:16PM -0700, Davidlohr Bueso wrote:
> On Mon, 23 May 2016, Jason Low wrote:
> 
> >On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:
> >>On 05/18/2016 12:58 PM, Jason Low wrote:
> >>> It should be fine to use the standard READ_ONCE here, even if it's just
> >>> for documentation, as it's probably not going to cost anything in
> >>> practice. It would be better to avoid adding any special macros for this
> >>> which may just add more complexity.
> >>
> >>See, I don't understand this line of reasoning at all.
> >>
> >>I read this as "it's ok to be non-optimal here where were spinning CPU
> >>time but not ok to be non-optimal generally elsewhere where it's
> >>way less important like at init time".
> >
> >So I think there is a difference between using it during init time and
> >using it here where we're spinning. During init time, initializing the
> >owner field locklessly is normal. No other thread should be concurrently
> >be writing to the field, since the structure is just getting
> >initialized, so there are no surprises there.
> >
> >Our access of the owner field in this function is special in that we're
> >using a bit of "lockless magic" to read and write to a field that gets
> >concurrently accessed without any serialization. Since we're not taking
> >the wait_lock in a scenario where we'd normally would take a lock, it
> >would be good to have this documented.
> >
> >>And by the way, it's not just "here" but _everywhere_.
> >>What about reading ->on_cpu locklessly?
> >
> >Sure, we could also use READ_ONCE when reading ->on_cpu  :)
> 
> Locking wise we should be covered with ->on_cpu as we're always under 
> rcu_read_lock
> (barrier, preempt_disable). But I'm not sure if this rule applies throughout 
> the
> scheduler, however, like it does in, say thread_group_cputime(). 
> cpu_clock_sample()
> (from posix timers) seems to mix and match being done under rcu. So 
> ultimately I
> think you're right.

But rcu_read_lock() does not exclude updates, which is one reason why
pointer reads use rcu_dereference() rather than normal assignments.

So I do not believe that rcu_read_lock() is helping you in this case.

That said, it is a bit hard to imagine the compiler tearing a load from
an int...  But compilers have uncovered weaknesses in my imagination
more than once in the past.

Thanx, Paul

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Davidlohr Bueso]

On Mon, 23 May 2016, Jason Low wrote:


On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:

On 05/18/2016 12:58 PM, Jason Low wrote:
> It should be fine to use the standard READ_ONCE here, even if it's just
> for documentation, as it's probably not going to cost anything in
> practice. It would be better to avoid adding any special macros for this
> which may just add more complexity.

See, I don't understand this line of reasoning at all.

I read this as "it's ok to be non-optimal here where were spinning CPU
time but not ok to be non-optimal generally elsewhere where it's
way less important like at init time".


So I think there is a difference between using it during init time and
using it here where we're spinning. During init time, initializing the
owner field locklessly is normal. No other thread should be concurrently
be writing to the field, since the structure is just getting
initialized, so there are no surprises there.

Our access of the owner field in this function is special in that we're
using a bit of "lockless magic" to read and write to a field that gets
concurrently accessed without any serialization. Since we're not taking
the wait_lock in a scenario where we'd normally would take a lock, it
would be good to have this documented.


And by the way, it's not just "here" but _everywhere_.
What about reading ->on_cpu locklessly?


Sure, we could also use READ_ONCE when reading ->on_cpu  :)


Locking wise we should be covered with ->on_cpu as we're always under 
rcu_read_lock
(barrier, preempt_disable). But I'm not sure if this rule applies throughout the
scheduler, however, like it does in, say thread_group_cputime(). 
cpu_clock_sample()
(from posix timers) seems to mix and match being done under rcu. So ultimately I
think you're right.

Thanks,
Davidlohr

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Davidlohr Bueso]

On Mon, 23 May 2016, Jason Low wrote:


On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:

On 05/18/2016 12:58 PM, Jason Low wrote:
> It should be fine to use the standard READ_ONCE here, even if it's just
> for documentation, as it's probably not going to cost anything in
> practice. It would be better to avoid adding any special macros for this
> which may just add more complexity.

See, I don't understand this line of reasoning at all.

I read this as "it's ok to be non-optimal here where were spinning CPU
time but not ok to be non-optimal generally elsewhere where it's
way less important like at init time".


So I think there is a difference between using it during init time and
using it here where we're spinning. During init time, initializing the
owner field locklessly is normal. No other thread should be concurrently
be writing to the field, since the structure is just getting
initialized, so there are no surprises there.

Our access of the owner field in this function is special in that we're
using a bit of "lockless magic" to read and write to a field that gets
concurrently accessed without any serialization. Since we're not taking
the wait_lock in a scenario where we'd normally would take a lock, it
would be good to have this documented.


And by the way, it's not just "here" but _everywhere_.
What about reading ->on_cpu locklessly?


Sure, we could also use READ_ONCE when reading ->on_cpu  :)


Locking wise we should be covered with ->on_cpu as we're always under 
rcu_read_lock
(barrier, preempt_disable). But I'm not sure if this rule applies throughout the
scheduler, however, like it does in, say thread_group_cputime(). 
cpu_clock_sample()
(from posix timers) seems to mix and match being done under rcu. So ultimately I
think you're right.

Thanks,
Davidlohr

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:
> On 05/18/2016 12:58 PM, Jason Low wrote:
> > It should be fine to use the standard READ_ONCE here, even if it's just
> > for documentation, as it's probably not going to cost anything in
> > practice. It would be better to avoid adding any special macros for this
> > which may just add more complexity.
> 
> See, I don't understand this line of reasoning at all.
> 
> I read this as "it's ok to be non-optimal here where were spinning CPU
> time but not ok to be non-optimal generally elsewhere where it's
> way less important like at init time".

So I think there is a difference between using it during init time and
using it here where we're spinning. During init time, initializing the
owner field locklessly is normal. No other thread should be concurrently
be writing to the field, since the structure is just getting
initialized, so there are no surprises there.

Our access of the owner field in this function is special in that we're
using a bit of "lockless magic" to read and write to a field that gets
concurrently accessed without any serialization. Since we're not taking
the wait_lock in a scenario where we'd normally would take a lock, it
would be good to have this documented.

> And by the way, it's not just "here" but _everywhere_.
> What about reading ->on_cpu locklessly?

Sure, we could also use READ_ONCE when reading ->on_cpu  :)

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:
> On 05/18/2016 12:58 PM, Jason Low wrote:
> > It should be fine to use the standard READ_ONCE here, even if it's just
> > for documentation, as it's probably not going to cost anything in
> > practice. It would be better to avoid adding any special macros for this
> > which may just add more complexity.
> 
> See, I don't understand this line of reasoning at all.
> 
> I read this as "it's ok to be non-optimal here where were spinning CPU
> time but not ok to be non-optimal generally elsewhere where it's
> way less important like at init time".

So I think there is a difference between using it during init time and
using it here where we're spinning. During init time, initializing the
owner field locklessly is normal. No other thread should be concurrently
be writing to the field, since the structure is just getting
initialized, so there are no surprises there.

Our access of the owner field in this function is special in that we're
using a bit of "lockless magic" to read and write to a field that gets
concurrently accessed without any serialization. Since we're not taking
the wait_lock in a scenario where we'd normally would take a lock, it
would be good to have this documented.

> And by the way, it's not just "here" but _everywhere_.
> What about reading ->on_cpu locklessly?

Sure, we could also use READ_ONCE when reading ->on_cpu  :)

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Peter Zijlstra]

On Sat, May 21, 2016 at 09:04:14AM -0700, Peter Hurley wrote:
> And by the way, it's not just "here" but _everywhere_.
> What about reading ->on_cpu locklessly?

In the smp_cond_acquire() rework that I have pending that one is
actually 'fixed'.

But yeah, the whole load/store tearing thing is giant pain inflicted
upon us by C language and GCC people :/

And the problem is that the 'regression' is now so old it doesn't matter
they fix it or not, we have to support compilers that think its fine to
generates tears :-(

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Peter Zijlstra]

On Sat, May 21, 2016 at 09:04:14AM -0700, Peter Hurley wrote:
> And by the way, it's not just "here" but _everywhere_.
> What about reading ->on_cpu locklessly?

In the smp_cond_acquire() rework that I have pending that one is
actually 'fixed'.

But yeah, the whole load/store tearing thing is giant pain inflicted
upon us by C language and GCC people :/

And the problem is that the 'regression' is now so old it doesn't matter
they fix it or not, we have to support compilers that think its fine to
generates tears :-(

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Peter Hurley]

On 05/18/2016 12:58 PM, Jason Low wrote:
> On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:
>> On 05/18/2016 01:21 PM, Jason Low wrote:
>>> On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
 On Tue, 17 May 2016, Waiman Long wrote:

> Without using WRITE_ONCE(), the compiler can potentially break a
> write into multiple smaller ones (store tearing). So a read from the
> same data by another task concurrently may return a partial result.
> This can result in a kernel crash if the data is a memory address
> that is being dereferenced.
>
> This patch changes all write to rwsem->owner to use WRITE_ONCE()
> to make sure that store tearing will not happen. READ_ONCE() may
> not be needed for rwsem->owner as long as the value is only used for
> comparison and not dereferencing.
>>> It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
>>> couldn't we include it to at least document that we're performing a
>>> "special" lockless read?
>>>
>>
>> Using READ_ONCE() does have a bit of cost as it limits compiler 
>> optimization. If we changes all access to rwsem->owner to READ_ONCE() 
>> and WRITE_ONCE(), we may as well change its type to volatile and be done 
>> with.
> 
> Right, although there are still places like the init function where
> WRITE_ONCE isn't necessary.

Which doesn't cost anything either.


>> I am not against doing that, but it feels a bit over-reach for me. 
>> On the other hand, we may define a do-nothing macro that designates the 
>> owner as a special variable for documentation purpose, but don't need 
>> protection at that particular call site.
> 
> It should be fine to use the standard READ_ONCE here, even if it's just
> for documentation, as it's probably not going to cost anything in
> practice. It would be better to avoid adding any special macros for this
> which may just add more complexity.

See, I don't understand this line of reasoning at all.

I read this as "it's ok to be non-optimal here where were spinning CPU
time but not ok to be non-optimal generally elsewhere where it's
way less important like at init time".

And by the way, it's not just "here" but _everywhere_.
What about reading ->on_cpu locklessly?

Sure it's a bool, but doesn't the "we need to document lockless access"
argument equally apply here?

Regards,
Peter Hurley

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Peter Hurley]

On 05/18/2016 12:58 PM, Jason Low wrote:
> On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:
>> On 05/18/2016 01:21 PM, Jason Low wrote:
>>> On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
 On Tue, 17 May 2016, Waiman Long wrote:

> Without using WRITE_ONCE(), the compiler can potentially break a
> write into multiple smaller ones (store tearing). So a read from the
> same data by another task concurrently may return a partial result.
> This can result in a kernel crash if the data is a memory address
> that is being dereferenced.
>
> This patch changes all write to rwsem->owner to use WRITE_ONCE()
> to make sure that store tearing will not happen. READ_ONCE() may
> not be needed for rwsem->owner as long as the value is only used for
> comparison and not dereferencing.
>>> It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
>>> couldn't we include it to at least document that we're performing a
>>> "special" lockless read?
>>>
>>
>> Using READ_ONCE() does have a bit of cost as it limits compiler 
>> optimization. If we changes all access to rwsem->owner to READ_ONCE() 
>> and WRITE_ONCE(), we may as well change its type to volatile and be done 
>> with.
> 
> Right, although there are still places like the init function where
> WRITE_ONCE isn't necessary.

Which doesn't cost anything either.


>> I am not against doing that, but it feels a bit over-reach for me. 
>> On the other hand, we may define a do-nothing macro that designates the 
>> owner as a special variable for documentation purpose, but don't need 
>> protection at that particular call site.
> 
> It should be fine to use the standard READ_ONCE here, even if it's just
> for documentation, as it's probably not going to cost anything in
> practice. It would be better to avoid adding any special macros for this
> which may just add more complexity.

See, I don't understand this line of reasoning at all.

I read this as "it's ok to be non-optimal here where were spinning CPU
time but not ok to be non-optimal generally elsewhere where it's
way less important like at init time".

And by the way, it's not just "here" but _everywhere_.
What about reading ->on_cpu locklessly?

Sure it's a bool, but doesn't the "we need to document lockless access"
argument equally apply here?

Regards,
Peter Hurley

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Waiman Long]

On 05/19/2016 06:21 PM, Jason Low wrote:

On Wed, 2016-05-18 at 12:58 -0700, Jason Low wrote:

On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:

On 05/18/2016 01:21 PM, Jason Low wrote:

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:

On Tue, 17 May 2016, Waiman Long wrote:


Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
couldn't we include it to at least document that we're performing a
"special" lockless read?


Using READ_ONCE() does have a bit of cost as it limits compiler
optimization. If we changes all access to rwsem->owner to READ_ONCE()
and WRITE_ONCE(), we may as well change its type to volatile and be done
with.

Right, although there are still places like the init function where
WRITE_ONCE isn't necessary.


I am not against doing that, but it feels a bit over-reach for me.
On the other hand, we may define a do-nothing macro that designates the
owner as a special variable for documentation purpose, but don't need
protection at that particular call site.

It should be fine to use the standard READ_ONCE here, even if it's just
for documentation, as it's probably not going to cost anything in
practice. It would be better to avoid adding any special macros for this
which may just add more complexity.

By the way, this potential "partial write" issue may also apply to
mutexes as well, so we should also make a similar change to
mutex_set_owner() and mutex_clear_owner().

Jason

 Yes, I am aware of that. I just don't have the time to to do a mutex 
patch yet. As you have sent out a patch on that, this is now covered.


Cheers,
Longman

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Waiman Long]

On 05/19/2016 06:21 PM, Jason Low wrote:

On Wed, 2016-05-18 at 12:58 -0700, Jason Low wrote:

On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:

On 05/18/2016 01:21 PM, Jason Low wrote:

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:

On Tue, 17 May 2016, Waiman Long wrote:


Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
couldn't we include it to at least document that we're performing a
"special" lockless read?


Using READ_ONCE() does have a bit of cost as it limits compiler
optimization. If we changes all access to rwsem->owner to READ_ONCE()
and WRITE_ONCE(), we may as well change its type to volatile and be done
with.

Right, although there are still places like the init function where
WRITE_ONCE isn't necessary.


I am not against doing that, but it feels a bit over-reach for me.
On the other hand, we may define a do-nothing macro that designates the
owner as a special variable for documentation purpose, but don't need
protection at that particular call site.

It should be fine to use the standard READ_ONCE here, even if it's just
for documentation, as it's probably not going to cost anything in
practice. It would be better to avoid adding any special macros for this
which may just add more complexity.

By the way, this potential "partial write" issue may also apply to
mutexes as well, so we should also make a similar change to
mutex_set_owner() and mutex_clear_owner().

Jason

 Yes, I am aware of that. I just don't have the time to to do a mutex 
patch yet. As you have sent out a patch on that, this is now covered.


Cheers,
Longman

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Wed, 2016-05-18 at 12:58 -0700, Jason Low wrote:
> On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:
> > On 05/18/2016 01:21 PM, Jason Low wrote:
> > > On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> > >> On Tue, 17 May 2016, Waiman Long wrote:
> > >>
> > >>> Without using WRITE_ONCE(), the compiler can potentially break a
> > >>> write into multiple smaller ones (store tearing). So a read from the
> > >>> same data by another task concurrently may return a partial result.
> > >>> This can result in a kernel crash if the data is a memory address
> > >>> that is being dereferenced.
> > >>>
> > >>> This patch changes all write to rwsem->owner to use WRITE_ONCE()
> > >>> to make sure that store tearing will not happen. READ_ONCE() may
> > >>> not be needed for rwsem->owner as long as the value is only used for
> > >>> comparison and not dereferencing.
> > > It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
> > > couldn't we include it to at least document that we're performing a
> > > "special" lockless read?
> > >
> > 
> > Using READ_ONCE() does have a bit of cost as it limits compiler 
> > optimization. If we changes all access to rwsem->owner to READ_ONCE() 
> > and WRITE_ONCE(), we may as well change its type to volatile and be done 
> > with.
> 
> Right, although there are still places like the init function where
> WRITE_ONCE isn't necessary.
> 
> > I am not against doing that, but it feels a bit over-reach for me. 
> > On the other hand, we may define a do-nothing macro that designates the 
> > owner as a special variable for documentation purpose, but don't need 
> > protection at that particular call site.
> 
> It should be fine to use the standard READ_ONCE here, even if it's just
> for documentation, as it's probably not going to cost anything in
> practice. It would be better to avoid adding any special macros for this
> which may just add more complexity.

By the way, this potential "partial write" issue may also apply to
mutexes as well, so we should also make a similar change to
mutex_set_owner() and mutex_clear_owner().

Jason

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Wed, 2016-05-18 at 12:58 -0700, Jason Low wrote:
> On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:
> > On 05/18/2016 01:21 PM, Jason Low wrote:
> > > On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> > >> On Tue, 17 May 2016, Waiman Long wrote:
> > >>
> > >>> Without using WRITE_ONCE(), the compiler can potentially break a
> > >>> write into multiple smaller ones (store tearing). So a read from the
> > >>> same data by another task concurrently may return a partial result.
> > >>> This can result in a kernel crash if the data is a memory address
> > >>> that is being dereferenced.
> > >>>
> > >>> This patch changes all write to rwsem->owner to use WRITE_ONCE()
> > >>> to make sure that store tearing will not happen. READ_ONCE() may
> > >>> not be needed for rwsem->owner as long as the value is only used for
> > >>> comparison and not dereferencing.
> > > It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
> > > couldn't we include it to at least document that we're performing a
> > > "special" lockless read?
> > >
> > 
> > Using READ_ONCE() does have a bit of cost as it limits compiler 
> > optimization. If we changes all access to rwsem->owner to READ_ONCE() 
> > and WRITE_ONCE(), we may as well change its type to volatile and be done 
> > with.
> 
> Right, although there are still places like the init function where
> WRITE_ONCE isn't necessary.
> 
> > I am not against doing that, but it feels a bit over-reach for me. 
> > On the other hand, we may define a do-nothing macro that designates the 
> > owner as a special variable for documentation purpose, but don't need 
> > protection at that particular call site.
> 
> It should be fine to use the standard READ_ONCE here, even if it's just
> for documentation, as it's probably not going to cost anything in
> practice. It would be better to avoid adding any special macros for this
> which may just add more complexity.

By the way, this potential "partial write" issue may also apply to
mutexes as well, so we should also make a similar change to
mutex_set_owner() and mutex_clear_owner().

Jason

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:
> On 05/18/2016 01:21 PM, Jason Low wrote:
> > On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> >> On Tue, 17 May 2016, Waiman Long wrote:
> >>
> >>> Without using WRITE_ONCE(), the compiler can potentially break a
> >>> write into multiple smaller ones (store tearing). So a read from the
> >>> same data by another task concurrently may return a partial result.
> >>> This can result in a kernel crash if the data is a memory address
> >>> that is being dereferenced.
> >>>
> >>> This patch changes all write to rwsem->owner to use WRITE_ONCE()
> >>> to make sure that store tearing will not happen. READ_ONCE() may
> >>> not be needed for rwsem->owner as long as the value is only used for
> >>> comparison and not dereferencing.
> > It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
> > couldn't we include it to at least document that we're performing a
> > "special" lockless read?
> >
> 
> Using READ_ONCE() does have a bit of cost as it limits compiler 
> optimization. If we changes all access to rwsem->owner to READ_ONCE() 
> and WRITE_ONCE(), we may as well change its type to volatile and be done 
> with.

Right, although there are still places like the init function where
WRITE_ONCE isn't necessary.

> I am not against doing that, but it feels a bit over-reach for me. 
> On the other hand, we may define a do-nothing macro that designates the 
> owner as a special variable for documentation purpose, but don't need 
> protection at that particular call site.

It should be fine to use the standard READ_ONCE here, even if it's just
for documentation, as it's probably not going to cost anything in
practice. It would be better to avoid adding any special macros for this
which may just add more complexity.

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:
> On 05/18/2016 01:21 PM, Jason Low wrote:
> > On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> >> On Tue, 17 May 2016, Waiman Long wrote:
> >>
> >>> Without using WRITE_ONCE(), the compiler can potentially break a
> >>> write into multiple smaller ones (store tearing). So a read from the
> >>> same data by another task concurrently may return a partial result.
> >>> This can result in a kernel crash if the data is a memory address
> >>> that is being dereferenced.
> >>>
> >>> This patch changes all write to rwsem->owner to use WRITE_ONCE()
> >>> to make sure that store tearing will not happen. READ_ONCE() may
> >>> not be needed for rwsem->owner as long as the value is only used for
> >>> comparison and not dereferencing.
> > It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
> > couldn't we include it to at least document that we're performing a
> > "special" lockless read?
> >
> 
> Using READ_ONCE() does have a bit of cost as it limits compiler 
> optimization. If we changes all access to rwsem->owner to READ_ONCE() 
> and WRITE_ONCE(), we may as well change its type to volatile and be done 
> with.

Right, although there are still places like the init function where
WRITE_ONCE isn't necessary.

> I am not against doing that, but it feels a bit over-reach for me. 
> On the other hand, we may define a do-nothing macro that designates the 
> owner as a special variable for documentation purpose, but don't need 
> protection at that particular call site.

It should be fine to use the standard READ_ONCE here, even if it's just
for documentation, as it's probably not going to cost anything in
practice. It would be better to avoid adding any special macros for this
which may just add more complexity.

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Waiman Long]

On 05/18/2016 01:21 PM, Jason Low wrote:

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:

On Tue, 17 May 2016, Waiman Long wrote:


Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
couldn't we include it to at least document that we're performing a
"special" lockless read?



Using READ_ONCE() does have a bit of cost as it limits compiler 
optimization. If we changes all access to rwsem->owner to READ_ONCE() 
and WRITE_ONCE(), we may as well change its type to volatile and be done 
with. I am not against doing that, but it feels a bit over-reach for me. 
On the other hand, we may define a do-nothing macro that designates the 
owner as a special variable for documentation purpose, but don't need 
protection at that particular call site.


Cheers,
Longman

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Waiman Long]

On 05/18/2016 01:21 PM, Jason Low wrote:

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:

On Tue, 17 May 2016, Waiman Long wrote:


Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
couldn't we include it to at least document that we're performing a
"special" lockless read?



Using READ_ONCE() does have a bit of cost as it limits compiler 
optimization. If we changes all access to rwsem->owner to READ_ONCE() 
and WRITE_ONCE(), we may as well change its type to volatile and be done 
with. I am not against doing that, but it feels a bit over-reach for me. 
On the other hand, we may define a do-nothing macro that designates the 
owner as a special variable for documentation purpose, but don't need 
protection at that particular call site.


Cheers,
Longman

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE()

[Jason Low]

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> On Tue, 17 May 2016, Waiman Long wrote:
> 
> >Without using WRITE_ONCE(), the compiler can potentially break a
> >write into multiple smaller ones (store tearing). So a read from the
> >same data by another task concurrently may return a partial result.
> >This can result in a kernel crash if the data is a memory address
> >that is being dereferenced.
> >
> >This patch changes all write to rwsem->owner to use WRITE_ONCE()
> >to make sure that store tearing will not happen. READ_ONCE() may
> >not be needed for rwsem->owner as long as the value is only used for
> >comparison and not dereferencing.
> >
> >Signed-off-by: Waiman Long 
> 
> Yes, ->owner can obviously be handled locklessly during optimistic
> spinning.
> 
> Acked-by: Davidlohr Bueso 

Acked-by: Jason Low 

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE()

[Jason Low]

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> On Tue, 17 May 2016, Waiman Long wrote:
> 
> >Without using WRITE_ONCE(), the compiler can potentially break a
> >write into multiple smaller ones (store tearing). So a read from the
> >same data by another task concurrently may return a partial result.
> >This can result in a kernel crash if the data is a memory address
> >that is being dereferenced.
> >
> >This patch changes all write to rwsem->owner to use WRITE_ONCE()
> >to make sure that store tearing will not happen. READ_ONCE() may
> >not be needed for rwsem->owner as long as the value is only used for
> >comparison and not dereferencing.
> >
> >Signed-off-by: Waiman Long 
> 
> Yes, ->owner can obviously be handled locklessly during optimistic
> spinning.
> 
> Acked-by: Davidlohr Bueso 

Acked-by: Jason Low 

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> On Tue, 17 May 2016, Waiman Long wrote:
> 
> >Without using WRITE_ONCE(), the compiler can potentially break a
> >write into multiple smaller ones (store tearing). So a read from the
> >same data by another task concurrently may return a partial result.
> >This can result in a kernel crash if the data is a memory address
> >that is being dereferenced.
> >
> >This patch changes all write to rwsem->owner to use WRITE_ONCE()
> >to make sure that store tearing will not happen. READ_ONCE() may
> >not be needed for rwsem->owner as long as the value is only used for
> >comparison and not dereferencing.

It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
couldn't we include it to at least document that we're performing a
"special" lockless read?

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> On Tue, 17 May 2016, Waiman Long wrote:
> 
> >Without using WRITE_ONCE(), the compiler can potentially break a
> >write into multiple smaller ones (store tearing). So a read from the
> >same data by another task concurrently may return a partial result.
> >This can result in a kernel crash if the data is a memory address
> >that is being dereferenced.
> >
> >This patch changes all write to rwsem->owner to use WRITE_ONCE()
> >to make sure that store tearing will not happen. READ_ONCE() may
> >not be needed for rwsem->owner as long as the value is only used for
> >comparison and not dereferencing.

It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
couldn't we include it to at least document that we're performing a
"special" lockless read?

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE()

[Davidlohr Bueso]

On Tue, 17 May 2016, Waiman Long wrote:


Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

Signed-off-by: Waiman Long 


Yes, ->owner can obviously be handled locklessly during optimistic
spinning.

Acked-by: Davidlohr Bueso 

Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE()

[Davidlohr Bueso]

On Tue, 17 May 2016, Waiman Long wrote:


Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

Signed-off-by: Waiman Long 


Yes, ->owner can obviously be handled locklessly during optimistic
spinning.

Acked-by: Davidlohr Bueso 

[PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE()

[Waiman Long]

Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

Signed-off-by: Waiman Long 
---
 kernel/locking/rwsem.h |   13 ++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h
index 8f43ba2..a699f40 100644
--- a/kernel/locking/rwsem.h
+++ b/kernel/locking/rwsem.h
@@ -16,14 +16,21 @@
 #define RWSEM_READER_OWNED ((struct task_struct *)1UL)
 
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
+/*
+ * All writes to owner are protected by WRITE_ONCE() to make sure that
+ * store tearing can't happen as optimistic spinners may read and use
+ * the owner value concurrently without lock. Read from owner, however,
+ * may not need READ_ONCE() as long as the pointer value is only used
+ * for comparison and isn't being dereferenced.
+ */
 static inline void rwsem_set_owner(struct rw_semaphore *sem)
 {
-   sem->owner = current;
+   WRITE_ONCE(sem->owner, current);
 }
 
 static inline void rwsem_clear_owner(struct rw_semaphore *sem)
 {
-   sem->owner = NULL;
+   WRITE_ONCE(sem->owner, NULL);
 }
 
 static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
@@ -34,7 +41,7 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore 
*sem)
 * to minimize cacheline contention.
 */
if (sem->owner != RWSEM_READER_OWNED)
-   sem->owner = RWSEM_READER_OWNED;
+   WRITE_ONCE(sem->owner, RWSEM_READER_OWNED);
 }
 
 static inline bool rwsem_owner_is_writer(struct task_struct *owner)
-- 
1.7.1

[PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE()

[Waiman Long]

Without using WRITE_ONCE(), the compiler can potentially break a
write into multiple smaller ones (store tearing). So a read from the
same data by another task concurrently may return a partial result.
This can result in a kernel crash if the data is a memory address
that is being dereferenced.

This patch changes all write to rwsem->owner to use WRITE_ONCE()
to make sure that store tearing will not happen. READ_ONCE() may
not be needed for rwsem->owner as long as the value is only used for
comparison and not dereferencing.

Signed-off-by: Waiman Long 
---
 kernel/locking/rwsem.h |   13 ++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h
index 8f43ba2..a699f40 100644
--- a/kernel/locking/rwsem.h
+++ b/kernel/locking/rwsem.h
@@ -16,14 +16,21 @@
 #define RWSEM_READER_OWNED ((struct task_struct *)1UL)
 
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
+/*
+ * All writes to owner are protected by WRITE_ONCE() to make sure that
+ * store tearing can't happen as optimistic spinners may read and use
+ * the owner value concurrently without lock. Read from owner, however,
+ * may not need READ_ONCE() as long as the pointer value is only used
+ * for comparison and isn't being dereferenced.
+ */
 static inline void rwsem_set_owner(struct rw_semaphore *sem)
 {
-   sem->owner = current;
+   WRITE_ONCE(sem->owner, current);
 }
 
 static inline void rwsem_clear_owner(struct rw_semaphore *sem)
 {
-   sem->owner = NULL;
+   WRITE_ONCE(sem->owner, NULL);
 }
 
 static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
@@ -34,7 +41,7 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore 
*sem)
 * to minimize cacheline contention.
 */
if (sem->owner != RWSEM_READER_OWNED)
-   sem->owner = RWSEM_READER_OWNED;
+   WRITE_ONCE(sem->owner, RWSEM_READER_OWNED);
 }
 
 static inline bool rwsem_owner_is_writer(struct task_struct *owner)
-- 
1.7.1