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

Groups > linux.kernel > #1530130 > unrolled thread

Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE()

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 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 12:30 +0100
    Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Christian Borntraeger <borntraeger@de.ibm.com> - 2016-11-25 12:40 +0100
      Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 13:30 +0100
        Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Peter Zijlstra <peterz@infradead.org> - 2016-11-25 13:50 +0100
          Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Peter Zijlstra <peterz@infradead.org> - 2016-11-25 13:50 +0100
            Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Boqun Feng <boqun.feng@gmail.com> - 2016-11-25 16:00 +0100
              Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Dmitry Vyukov <dvyukov@google.com> - 2016-11-25 16:30 +0100
                Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 17:20 +0100
                  Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Peter Zijlstra <peterz@infradead.org> - 2016-11-25 17:20 +0100
                    Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 17:40 +0100
                    Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Christian Borntraeger <borntraeger@de.ibm.com> - 2016-11-25 18:00 +0100
                      Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 18:30 +0100
                        Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Peter Zijlstra <peterz@infradead.org> - 2016-11-25 18:50 +0100
                        Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Christian Borntraeger <borntraeger@de.ibm.com> - 2016-11-25 19:50 +0100
                      Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() "Michael S. Tsirkin" <mst@redhat.com> - 2016-11-25 22:10 +0100
                        Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Christian Borntraeger <borntraeger@de.ibm.com> - 2016-11-25 22:50 +0100
                    Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Dmitry Vyukov <dvyukov@google.com> - 2016-11-25 18:30 +0100
                      Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 18:50 +0100
                      Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-25 19:00 +0100
                        Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 19:10 +0100
                          Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-25 19:50 +0100
          Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-25 15:40 +0100

Page 1 of 2  [1] 2  Next page →

#1530130 — Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE()

On Thu, Nov 24, 2016 at 10:36:58PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 24, 2016 at 10:25:11AM +0000, Mark Rutland wrote:
> > For several reasons, it would be beneficial to kill off ACCESS_ONCE()
> > tree-wide, in favour of {READ,WRITE}_ONCE(). These work with aggregate types,
> > more obviously document their intended behaviour, and are necessary for tools
> > like KTSAN to work correctly (as otherwise reads and writes cannot be
> > instrumented separately).
> > 
> > While it's possible to script the bulk of this tree-wide conversion, some cases
> > such as the virtio code, require some manual intervention. This series moves
> > the virtio and vringh code over to {READ,WRITE}_ONCE(), in the process fixing a
> > bug in the virtio headers.
> > 
> > Thanks,
> > Mark.
> 
> I don't have a problem with this specific patchset.

Good to hear. :)

Does that mean you're happy to queue these patches? Or would you prefer
a new posting at some later point, with ack/review tags accumulated?

> Though I really question the whole _ONCE APIs esp with
> aggregate types - these seem to generate a memcpy and
> an 8-byte read/writes sometimes, and I'm pretty sure this simply
> can't be read/written at once on all architectures.

Yes, in cases where the access is larger than the machine can perform in
a single access, this will result in a memcpy.

My understanding is that this has always been the case with
ACCESS_ONCE(), where multiple accesses were silently/implicitly
generated by the compiler.

We could add some compile-time warnings for those cases. I'm not sure if
there's a reason we avoided doing that so far; perhaps Christian has a
some idea.

Thanks,
Mark.

[toc] | [next] | [standalone]

#1530135

On 11/25/2016 12:22 PM, Mark Rutland wrote:
> On Thu, Nov 24, 2016 at 10:36:58PM +0200, Michael S. Tsirkin wrote:
>> On Thu, Nov 24, 2016 at 10:25:11AM +0000, Mark Rutland wrote:
>>> For several reasons, it would be beneficial to kill off ACCESS_ONCE()
>>> tree-wide, in favour of {READ,WRITE}_ONCE(). These work with aggregate types,
>>> more obviously document their intended behaviour, and are necessary for tools
>>> like KTSAN to work correctly (as otherwise reads and writes cannot be
>>> instrumented separately).
>>>
>>> While it's possible to script the bulk of this tree-wide conversion, some cases
>>> such as the virtio code, require some manual intervention. This series moves
>>> the virtio and vringh code over to {READ,WRITE}_ONCE(), in the process fixing a
>>> bug in the virtio headers.
>>>
>>> Thanks,
>>> Mark.
>>
>> I don't have a problem with this specific patchset.
> 
> Good to hear. :)
> 
> Does that mean you're happy to queue these patches? Or would you prefer
> a new posting at some later point, with ack/review tags accumulated?
> 
>> Though I really question the whole _ONCE APIs esp with
>> aggregate types - these seem to generate a memcpy and
>> an 8-byte read/writes sometimes, and I'm pretty sure this simply
>> can't be read/written at once on all architectures.
> 
> Yes, in cases where the access is larger than the machine can perform in
> a single access, this will result in a memcpy.
> 
> My understanding is that this has always been the case with
> ACCESS_ONCE(), where multiple accesses were silently/implicitly
> generated by the compiler.
> 
> We could add some compile-time warnings for those cases. I'm not sure if
> there's a reason we avoided doing that so far; perhaps Christian has a
> some idea.

My first version had this warning, but it was removed later on as requested
by Linus

http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02670.html
---snip---

Get rid of the f*cking size checks etc on READ_ONCE() and friends.

They are about - wait for it - "reading a value once".

Note how it doesn't say ANYTHING about "atomic" or anything like that.
It's about reading *ONCE*.

---snip---

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

#1530166

On Fri, Nov 25, 2016 at 12:33:48PM +0100, Christian Borntraeger wrote:
> On 11/25/2016 12:22 PM, Mark Rutland wrote:
> > On Thu, Nov 24, 2016 at 10:36:58PM +0200, Michael S. Tsirkin wrote:
> >> Though I really question the whole _ONCE APIs esp with
> >> aggregate types - these seem to generate a memcpy and
> >> an 8-byte read/writes sometimes, and I'm pretty sure this simply
> >> can't be read/written at once on all architectures.
> > 
> > Yes, in cases where the access is larger than the machine can perform in
> > a single access, this will result in a memcpy.
> > 
> > My understanding is that this has always been the case with
> > ACCESS_ONCE(), where multiple accesses were silently/implicitly
> > generated by the compiler.
> > 
> > We could add some compile-time warnings for those cases. I'm not sure if
> > there's a reason we avoided doing that so far; perhaps Christian has a
> > some idea.
> 
> My first version had this warning, but it was removed later on as requested
> by Linus
> 
> http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02670.html
> ---snip---
> 
> Get rid of the f*cking size checks etc on READ_ONCE() and friends.
> 
> They are about - wait for it - "reading a value once".
> 
> Note how it doesn't say ANYTHING about "atomic" or anything like that.
> It's about reading *ONCE*.
> 
> ---snip---

I see. That's unfortunate, given that practically every use I'm aware of
assumes some atomicity (e.g. freedom from tearing when loading/storing
pointers or values up to the native width of the machine). I believe
that's the case here, for virtio, for example.

Perhaps we can add new accessors that are supposed to guarantee that,
into which we can drop appropriate warnings.

Naming will be problematic; calling them ATOMIC_* makes tham sound like
they work on atomic_t. That and I have no idea how to ensure correct
usage tree-wide; I'm not sure if/how Coccinelle can help.

Peter, thoughts?

Thanks,
Mark.

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

#1530174

On Fri, Nov 25, 2016 at 12:23:56PM +0000, Mark Rutland wrote:
> Naming will be problematic; calling them ATOMIC_* makes tham sound like
> they work on atomic_t. That and I have no idea how to ensure correct
> usage tree-wide; I'm not sure if/how Coccinelle can help.
> 
> Peter, thoughts?

Something like so perhaps?

---

#ifdef CONFIG_DEBUG_ATOMIC_SLEEP
#define WARN_SINGLE_COPY_ALIGNMENT(ptr)	\
	WARN_ON_ONCE(((unsigned long)(ptr)) & (sizeof(*(ptr))-1))
#else
#define WARN_SINGLE_COPY_ALIGNMENT(ptr)
#endif

/*
 * Provide accessors for Single-Copy atomicy.
 *
 * That is, ensure that machine word sized loads/stores to naturally
 * aligned variables are single instructions.
 *
 * By reason of not being able to use C11 atomic crud, use our beloved
 * volatile qualifier. Since volatile tells the compiler the value can
 * be changed behind its back, it must use Single-Copy atomic loads and
 * stores to access them, otherwise it runs the risk of load/store
 * tearing.
 */

#define SINGLE_LOAD(x)						\
{(								\
	compiletime_assert_atomic_type(typeof(x));		\
	WARN_SINGLE_COPY_ALIGNMENT(&(x));			\
	READ_ONCE(x);						\
})

#define SINGLE_STORE(x, v)					\
({								\
	compiletime_assert_atomic_type(typeof(x));		\
	WARN_SINGLE_COPY_ALIGNMENT(&(x));			\
	WRITE_ONCE(x, v);					\
})

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

#1530187

On Fri, Nov 25, 2016 at 01:40:44PM +0100, Peter Zijlstra wrote:
> #define SINGLE_LOAD(x)						\
> {(								\
> 	compiletime_assert_atomic_type(typeof(x));		\

Should be:

	compiletime_assert_atomic_type(x);

> 	WARN_SINGLE_COPY_ALIGNMENT(&(x));			\
> 	READ_ONCE(x);						\
> })
> 
> #define SINGLE_STORE(x, v)					\
> ({								\
> 	compiletime_assert_atomic_type(typeof(x));		\

idem

> 	WARN_SINGLE_COPY_ALIGNMENT(&(x));			\
> 	WRITE_ONCE(x, v);					\
> })

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

#1530301

On Fri, Nov 25, 2016 at 01:44:04PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 25, 2016 at 01:40:44PM +0100, Peter Zijlstra wrote:
> > #define SINGLE_LOAD(x)						\
> > {(								\
> > 	compiletime_assert_atomic_type(typeof(x));		\
> 
> Should be:
> 
> 	compiletime_assert_atomic_type(x);
> 
> > 	WARN_SINGLE_COPY_ALIGNMENT(&(x));			\

Do we need to worry about the side effect on x? Maybe

#define SINGLE_LOAD(x)					\
({							\
	typeof(x) *_____ptr;				\
							\
	compiletime_assert_atomic_type(typeof(x));	\
							\
	_____ptr = &(x);				\
							\
	WARN_SINGLE_COPY_ALIGNMENT(_____ptr);		\
							\
	READ_ONCE(*_____ptr);				\
})

Ditto for SINGLE_STORE()

Regards,
Boqun

> > 	READ_ONCE(x);						\
> > })
> > 
> > #define SINGLE_STORE(x, v)					\
> > ({								\
> > 	compiletime_assert_atomic_type(typeof(x));		\
> 
> idem
> 
> > 	WARN_SINGLE_COPY_ALIGNMENT(&(x));			\
> > 	WRITE_ONCE(x, v);					\
> > })

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

#1530355

On Fri, Nov 25, 2016 at 3:56 PM, Boqun Feng <boqun.feng@gmail.com> wrote:
> On Fri, Nov 25, 2016 at 01:44:04PM +0100, Peter Zijlstra wrote:
>> On Fri, Nov 25, 2016 at 01:40:44PM +0100, Peter Zijlstra wrote:
>> > #define SINGLE_LOAD(x)                                              \
>> > {(                                                          \
>> >     compiletime_assert_atomic_type(typeof(x));              \
>>
>> Should be:
>>
>>       compiletime_assert_atomic_type(x);
>>
>> >     WARN_SINGLE_COPY_ALIGNMENT(&(x));                       \
>
> Do we need to worry about the side effect on x? Maybe
>
> #define SINGLE_LOAD(x)                                  \
> ({                                                      \
>         typeof(x) *_____ptr;                            \
>                                                         \
>         compiletime_assert_atomic_type(typeof(x));      \
>                                                         \
>         _____ptr = &(x);                                \
>                                                         \
>         WARN_SINGLE_COPY_ALIGNMENT(_____ptr);           \
>                                                         \
>         READ_ONCE(*_____ptr);                           \
> })
>
> Ditto for SINGLE_STORE()
>
> Regards,
> Boqun
>
>> >     READ_ONCE(x);                                           \
>> > })
>> >
>> > #define SINGLE_STORE(x, v)                                  \
>> > ({                                                          \
>> >     compiletime_assert_atomic_type(typeof(x));              \
>>
>> idem
>>
>> >     WARN_SINGLE_COPY_ALIGNMENT(&(x));                       \
>> >     WRITE_ONCE(x, v);                                       \
>> > })


READ/WRITE_ONCE imply atomicity. Even if their names don't spell it (a
function name doesn't have to spell all of its guarantees). Most of
the uses of READ/WRITE_ONCE will be broken if they are not atomic.
"Read once but not necessary atomically" is a very subtle primitive
which is very easy to misuse. What are use cases for such primitive
that won't be OK with "read once _and_ atomically"? Copy to/from user
is obviously one such case, but it is already handled specially.

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

#1530394

On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
> 
> READ/WRITE_ONCE imply atomicity. Even if their names don't spell it (a
> function name doesn't have to spell all of its guarantees). Most of
> the uses of READ/WRITE_ONCE will be broken if they are not atomic.

In practice, this is certainly the assumption made by many/most users of
the *_ONCE() accessors.

Looking again, Linus does seem to agree that word-sized accesses should
result in single instructions (and be single-copy atomic) [1], so in
contrast to [2], that's clearly *part* of the point of the *_ONCE()
accessors...

> "Read once but not necessary atomically" is a very subtle primitive
> which is very easy to misuse.

I agree. Unfortunately, Linus does not appear to [2].

> What are use cases for such primitive that won't be OK with "read once
> _and_ atomically"?

I have none to hand.

Thanks,
Mark.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02674.html
[2] http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02670.html

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

#1530399

On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
> On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:

> > What are use cases for such primitive that won't be OK with "read once
> > _and_ atomically"?
> 
> I have none to hand.

Whatever triggers the __builtin_memcpy() paths, and even the size==8
paths on 32bit.

You could put a WARN in there to easily find them.

The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
they compiletime validate this the size is 'right' and can runtime check
alignment constraints.

IE, they are strictly stronger than {READ,WRITE}_ONCE().

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

#1530415

On Fri, Nov 25, 2016 at 05:17:09PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
> > On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
> 
> > > What are use cases for such primitive that won't be OK with "read once
> > > _and_ atomically"?
> > 
> > I have none to hand.
> 
> Whatever triggers the __builtin_memcpy() paths, and even the size==8
> paths on 32bit.

Lockref, per:

http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02294.html

In that specific case, a torn value just means we'll retry until we get
a non torn value, due to the cmpxchg. For that case, all we need is the
value to be reloaded per invocation of READ_ONCE().

This guy seems to have the full story:

http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02389.html
http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02558.html

Thanks,
Mark.

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

#1530431

On 11/25/2016 05:17 PM, Peter Zijlstra wrote:
> On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
>> On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
> 
>>> What are use cases for such primitive that won't be OK with "read once
>>> _and_ atomically"?
>>
>> I have none to hand.
> 
> Whatever triggers the __builtin_memcpy() paths, and even the size==8
> paths on 32bit.
> 
> You could put a WARN in there to easily find them.

There were several cases that I found during writing the *ONCE stuff.
For example there are some 32bit ppc variants with 64bit PTEs. Some for
others (I think sparc). And the mm/ code is perfectly fine with these
PTE accesses being done NOT atomic.


> 
> The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
> they compiletime validate this the size is 'right' and can runtime check
> alignment constraints.
> 
> IE, they are strictly stronger than {READ,WRITE}_ONCE().
> 

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

#1530462

On Fri, Nov 25, 2016 at 05:49:45PM +0100, Christian Borntraeger wrote:
> On 11/25/2016 05:17 PM, Peter Zijlstra wrote:
> > On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
> >> On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
> > 
> >>> What are use cases for such primitive that won't be OK with "read once
> >>> _and_ atomically"?
> >>
> >> I have none to hand.
> > 
> > Whatever triggers the __builtin_memcpy() paths, and even the size==8
> > paths on 32bit.
> > 
> > You could put a WARN in there to easily find them.
> 
> There were several cases that I found during writing the *ONCE stuff.
> For example there are some 32bit ppc variants with 64bit PTEs. Some for
> others (I think sparc).

We have similar on 32-bit ARM w/ LPAE. LPAE implies that a naturally
aligned 64-bit access is single-copy atomic, which is what makes that
ok.

> And the mm/ code is perfectly fine with these PTE accesses being done
> NOT atomic.

That strikes me as surprising. Is there some mutual exclusion that
prevents writes from occuring wherever a READ_ONCE() happens to a PTE?

Otherwise, how is tearing not a problem? Does it have some pattern like
the lockref cmpxchg?

Thanks,
Mark.

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

#1530474

On Fri, Nov 25, 2016 at 05:28:01PM +0000, Mark Rutland wrote:
> On Fri, Nov 25, 2016 at 05:49:45PM +0100, Christian Borntraeger wrote:
> > On 11/25/2016 05:17 PM, Peter Zijlstra wrote:

> > There were several cases that I found during writing the *ONCE stuff.
> > For example there are some 32bit ppc variants with 64bit PTEs. Some for
> > others (I think sparc).
> 
> We have similar on 32-bit ARM w/ LPAE. LPAE implies that a naturally
> aligned 64-bit access is single-copy atomic, which is what makes that
> ok.
> 
> > And the mm/ code is perfectly fine with these PTE accesses being done
> > NOT atomic.
> 
> That strikes me as surprising. Is there some mutual exclusion that
> prevents writes from occuring wherever a READ_ONCE() happens to a PTE?
> 
> Otherwise, how is tearing not a problem? Does it have some pattern like
> the lockref cmpxchg?

On x86 PAE we play silly games, see arch/x86/mm/gup.c:gup_get_ptr().

Those two loads really should be READ_ONCE()/LOAD_SINGLE().

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

#1530491

On 11/25/2016 06:28 PM, Mark Rutland wrote:
> On Fri, Nov 25, 2016 at 05:49:45PM +0100, Christian Borntraeger wrote:
>> On 11/25/2016 05:17 PM, Peter Zijlstra wrote:
>>> On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
>>>> On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
>>>
>>>>> What are use cases for such primitive that won't be OK with "read once
>>>>> _and_ atomically"?
>>>>
>>>> I have none to hand.
>>>
>>> Whatever triggers the __builtin_memcpy() paths, and even the size==8
>>> paths on 32bit.
>>>
>>> You could put a WARN in there to easily find them.
>>
>> There were several cases that I found during writing the *ONCE stuff.
>> For example there are some 32bit ppc variants with 64bit PTEs. Some for
>> others (I think sparc).
> 
> We have similar on 32-bit ARM w/ LPAE. LPAE implies that a naturally
> aligned 64-bit access is single-copy atomic, which is what makes that
> ok.
> 
>> And the mm/ code is perfectly fine with these PTE accesses being done
>> NOT atomic.
> 
> That strikes me as surprising. Is there some mutual exclusion that
> prevents writes from occuring wherever a READ_ONCE() happens to a PTE?

See for example mm/memory.c handle_pte_fault.

---snip----

                /*
                 * some architectures can have larger ptes than wordsize,
                 * e.g.ppc44x-defconfig has CONFIG_PTE_64BIT=y and
                 * CONFIG_32BIT=y, so READ_ONCE or ACCESS_ONCE cannot guarantee
                 * atomic accesses.  The code below just needs a consistent
                 * view for the ifs and we later double check anyway with the
                 * ptl lock held. So here a barrier will do.
                 */
---snip---

The trick is that the code only does a specific check, but all other accesses are under
the pte lock.

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

#1530530

On Fri, Nov 25, 2016 at 05:49:45PM +0100, Christian Borntraeger wrote:
> On 11/25/2016 05:17 PM, Peter Zijlstra wrote:
> > On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
> >> On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
> > 
> >>> What are use cases for such primitive that won't be OK with "read once
> >>> _and_ atomically"?
> >>
> >> I have none to hand.
> > 
> > Whatever triggers the __builtin_memcpy() paths, and even the size==8
> > paths on 32bit.
> > 
> > You could put a WARN in there to easily find them.
> 
> There were several cases that I found during writing the *ONCE stuff.
> For example there are some 32bit ppc variants with 64bit PTEs. Some for
> others (I think sparc). And the mm/ code is perfectly fine with these
> PTE accesses being done NOT atomic.

In that case do we even need _ONCE at all?
Are there assumptions these are two 32 bit reads?


> 
> > 
> > The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
> > they compiletime validate this the size is 'right' and can runtime check
> > alignment constraints.
> > 
> > IE, they are strictly stronger than {READ,WRITE}_ONCE().
> > 

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

#1530544

On 11/25/2016 10:08 PM, Michael S. Tsirkin wrote:
> On Fri, Nov 25, 2016 at 05:49:45PM +0100, Christian Borntraeger wrote:
>> On 11/25/2016 05:17 PM, Peter Zijlstra wrote:
>>> On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
>>>> On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
>>>
>>>>> What are use cases for such primitive that won't be OK with "read once
>>>>> _and_ atomically"?
>>>>
>>>> I have none to hand.
>>>
>>> Whatever triggers the __builtin_memcpy() paths, and even the size==8
>>> paths on 32bit.
>>>
>>> You could put a WARN in there to easily find them.
>>
>> There were several cases that I found during writing the *ONCE stuff.
>> For example there are some 32bit ppc variants with 64bit PTEs. Some for
>> others (I think sparc). And the mm/ code is perfectly fine with these
>> PTE accesses being done NOT atomic.
> 
> In that case do we even need _ONCE at all?

Yes. For example look at gup_pmd_range. Here several checks are made on the pmd.
It is important the the check for pmd_none is made on the same value than
the check for pmd_trans_huge, but it is not important that the value is still up
to date. 
And there are really cases where we cannot read the  thing atomically, e.g. on 
m68k and sparc(32bit) pmd_t is defined as array of longs.

Another problem is that a compiler can implement the following code as 2 memory
reads (e.g. if you have compare instructions that work on memory) instead of a 
memory read and 2 compares

int check(unsigned long *value_p) {
	unsigned long value = *value_p;
	if (condition_a(value))
		return 1;
	if (condition_b(value))
		return 2;
	return 3;
}

With READ_ONCE you forbid that. In past times you would have used barrier() after 
the assignment to achieve the same goal.


> Are there assumptions these are two 32 bit reads?

It depends on the code. Some places (e.g. in gup) assumes that the access via
READ_ONCE is atomic (which it is for sane compilers as long as the pointer
is <= word size). In some others places just one bit is tested.
> 
> 
>>
>>>
>>> The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
>>> they compiletime validate this the size is 'right' and can runtime check
>>> alignment constraints.
>>>
>>> IE, they are strictly stronger than {READ,WRITE}_ONCE().
>>>
> 

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

#1530460

On Fri, Nov 25, 2016 at 5:17 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > What are use cases for such primitive that won't be OK with "read once
>> > _and_ atomically"?
>>
>> I have none to hand.
>
> Whatever triggers the __builtin_memcpy() paths, and even the size==8
> paths on 32bit.
>
> You could put a WARN in there to easily find them.
>
> The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
> they compiletime validate this the size is 'right' and can runtime check
> alignment constraints.
>
> IE, they are strictly stronger than {READ,WRITE}_ONCE().


Uh, so, READ/WRITE_ONCE are non-atomic now. I missed that.

If READ/WRITE_ONCE are non-atomic, half of kernel is broken. All these
loads of flags, ringbuffer positions, pointers, etc are broken.

What about restoring READ/WRITE_ONCE as atomic, and introducing
separate primitives for _non_ atomic loads/stores?
It seems to me that there is just a dozen of cases that don't need
atomicity and where performance is any important (though, some of
these should probably try to write to shared memory less frequently
and save hundreds of cycles, rather than try to save few cycles on
local instructions).

I've compiled kernel with restored size checks in
READ/WRITE/ACCESS_ONCE and the following places seem to expect that
access is actually atomic (while it is not).
But if we don't guarantee that word-sized READ/WRITE_ONCE are atomic,
then I am sure we can find a hundred more of broken places.


arch/x86/entry/vdso/vdso32/../vclock_gettime.c:297:18: note: in
expansion of macro ‘ACCESS_ONCE’
  time_t result = ACCESS_ONCE(gtod->wall_time_sec);

kernel/events/ring_buffer.c:160:10: error: call to
‘__compiletime_assert_160’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   tail = READ_ONCE(rb->user_page->data_tail);

kernel/events/core.c:5145:16: error: call to
‘__compiletime_assert_5145’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   aux_offset = ACCESS_ONCE(rb->user_page->aux_offset);
                ^
kernel/events/core.c:5146:14: error: call to
‘__compiletime_assert_5146’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   aux_size = ACCESS_ONCE(rb->user_page->aux_size);

drivers/cpufreq/cpufreq_governor.c:283:8: error: call to
‘__compiletime_assert_283’ declared with attribute error: Need native
word sized stores/loads for atomicity.
  lst = READ_ONCE(policy_dbs->last_sample_time);
        ^
drivers/cpufreq/cpufreq_governor.c:301:7: error: call to
‘__compiletime_assert_301’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   if (unlikely(lst != READ_ONCE(policy_dbs->last_sample_time))) {

net/core/gen_estimator.c:136:3: error: call to
‘__compiletime_assert_136’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   WRITE_ONCE(e->rate_est->bps, (e->avbps + 0xF) >> 5);
   ^
net/core/gen_estimator.c:142:3: error: call to
‘__compiletime_assert_142’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   WRITE_ONCE(e->rate_est->pps, (e->avpps + 0xF) >> 5);

fs/proc_namespace.c:28:10: error: call to ‘__compiletime_assert_28’
declared with attribute error: Need native word sized stores/loads for
atomicity.
  event = ACCESS_ONCE(ns->event);

drivers/md/dm-stats.c:700:32: error: call to
‘__compiletime_assert_700’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   shared->tmp.sectors[READ] += ACCESS_ONCE(p->sectors[READ]);
                                ^
drivers/md/dm-stats.c:701:33: error: call to
‘__compiletime_assert_701’ declared with attribute error: Need native
word sized stores/loads for atomicity.
   shared->tmp.sectors[WRITE] += ACCESS_ONCE(p->sectors[WRITE]);
                                 ^

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

#1530473

On Fri, Nov 25, 2016 at 06:28:53PM +0100, Dmitry Vyukov wrote:
> On Fri, Nov 25, 2016 at 5:17 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> >> > What are use cases for such primitive that won't be OK with "read once
> >> > _and_ atomically"?
> >>
> >> I have none to hand.
> >
> > Whatever triggers the __builtin_memcpy() paths, and even the size==8
> > paths on 32bit.
> >
> > You could put a WARN in there to easily find them.
> >
> > The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
> > they compiletime validate this the size is 'right' and can runtime check
> > alignment constraints.
> >
> > IE, they are strictly stronger than {READ,WRITE}_ONCE().
> 
> Uh, so, READ/WRITE_ONCE are non-atomic now. I missed that.

Yes, but *only* for types larger than word size. That has *always* been
the case.

It's still assumed that *_ONCE are single-copy-atomic for word size (or
smaller). Some architectures may also provide that guarnatee for
accesses larger than word size (e.g. 32-bit ARM w/ LPAE).

... It's just that as things stand we can't put checks in *_ONCE() for
the access size, since they're *also* used for larger accesses that
don't need atomicity.

> If READ/WRITE_ONCE are non-atomic, half of kernel is broken. All these
> loads of flags, ringbuffer positions, pointers, etc are broken.

Most of these will be fine, as above.

> What about restoring READ/WRITE_ONCE as atomic, and introducing
> separate primitives for _non_ atomic loads/stores?

Having a separate *_ONCE_TEARABLE() would certainly limit the number of
things we have to fix up, and would also make it clear that atomicity is
not expected.

... but we might have to go with SINGLE_*() if we can't convince Linus.

Thanks,
Mark.

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

#1530476

On Fri, Nov 25, 2016 at 9:28 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
On Fri, Nov 25, 2016 at 5:17 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> IE, they are strictly stronger than {READ,WRITE}_ONCE().

No, they are strictly bullshit.

Stop this idiocy. We went through this once already.

> Uh, so, READ/WRITE_ONCE are non-atomic now. I missed that.

No.

READ/WRITE_ONCE() are atomic *WHEN*THAT*IS*POSSIBLE*. So for something
that fits in a register, it will read it in one atomic access. For
something that fits in a register and is _possible_ to write
atomically, it will do so.

But sometimes it's not going to be atomic. We do not for a moment try
to make multi-word accesses be atomic. Not even if you could try to
use some magic cmpxchg16b thing. It's not "atomic" in that sense: it
will be doing multiple accesses.

Similarly, if you try to write a 8- or 16-bit word on alpha with
WRITE_ONCE() or you try to do other things, you have what's coming to
you.

And they just force some "copy to stable storage" when it isn't (ie a
"memcpy()" is not necessarily a single access and might be done as
multiple overlapping reads, but the end result is stable).

So trying to make anything else out of them is f*cking stupid.

READ_ONCE() and friends do the right thing. Trying to limit them is
*wrong*, because the restrictions would simply make them less useful.
And trying to make up something new is pointless and stupid.

So leave this code alone. Don't add some stupid "SINGLE_LOAD()" crap.
That's just moronic. READ_ONCE() is that, and so much more.

               Linus

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

#1530480

On Fri, Nov 25, 2016 at 09:52:50AM -0800, Linus Torvalds wrote:
> READ/WRITE_ONCE() are atomic *WHEN*THAT*IS*POSSIBLE*.

> But sometimes it's not going to be atomic.

That's the problem.

Common code may rely on something being atomic when that's only true on
a subset of platforms. On others, it's silently "fixed" into something
that isn't atomic, and we get no diagnostic. The bug lurks beneath the
surface.

Thanks,
Mark.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web