[PATCH 1/3] tools/virtio: fix READ_ONCE()

[PATCH 1/3] tools/virtio: fix READ_ONCE()

[Mark Rutland]

The virtio tools implementation of READ_ONCE() has a single parameter called
'var', but erroneously refers to 'val' for its cast, and thus won't work unless
there's a variable of the correct type that happens to be called 'var'.

Fix this with s/var/val/, making READ_ONCE() work as expected regardless.

Fixes: a7c490333df3cff5 ("tools/virtio: use virt_xxx barriers")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: linux-kernel@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
---
 tools/virtio/linux/compiler.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h
index 845960e..c9ccfd4 100644
--- a/tools/virtio/linux/compiler.h
+++ b/tools/virtio/linux/compiler.h
@@ -4,6 +4,6 @@
 #define WRITE_ONCE(var, val) \
 	(*((volatile typeof(val) *)(&(var))) = (val))
 
-#define READ_ONCE(var) (*((volatile typeof(val) *)(&(var))))
+#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
 
 #endif
-- 
2.7.4

[PATCH 2/3] vringh: kill off ACCESS_ONCE()

[Mark Rutland]

Despite living under drivers/ vringh.c is also used as part of the userspace
virtio tools. Before we can kill off the ACCESS_ONCE()definition in the tools,
we must convert vringh.c to use {READ,WRITE}_ONCE().

This patch does so, along with the required include of <linux/compiler.h> for
the relevant definitions. The userspace tools provide their own definitions in
their own <linux/compiler.h>.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
---
 drivers/vhost/vringh.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 3bb02c6..bb8971f 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -3,6 +3,7 @@
  *
  * Since these may be in userspace, we use (inline) accessors.
  */
+#include <linux/compiler.h>
 #include <linux/module.h>
 #include <linux/vringh.h>
 #include <linux/virtio_ring.h>
@@ -820,13 +821,13 @@ EXPORT_SYMBOL(vringh_need_notify_user);
 static inline int getu16_kern(const struct vringh *vrh,
 			      u16 *val, const __virtio16 *p)
 {
-	*val = vringh16_to_cpu(vrh, ACCESS_ONCE(*p));
+	*val = vringh16_to_cpu(vrh, READ_ONCE(*p));
 	return 0;
 }
 
 static inline int putu16_kern(const struct vringh *vrh, __virtio16 *p, u16 val)
 {
-	ACCESS_ONCE(*p) = cpu_to_vringh16(vrh, val);
+	WRITE_ONCE(*p, cpu_to_vringh16(vrh, val));
 	return 0;
 }
 
-- 
2.7.4

[PATCH 3/3] tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h

[Mark Rutland]

As a step towards killing off ACCESS_ONCE, use {READ,WRITE}_ONCE() for the
virtio tools uaccess primitives, pulling these in from <linux/compiler.h>.

With this done, we can kill off the now-unused ACCESS_ONCE() definition.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: linux-kernel@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
---
 tools/virtio/linux/uaccess.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h
index 0a578fe..fa05d01 100644
--- a/tools/virtio/linux/uaccess.h
+++ b/tools/virtio/linux/uaccess.h
@@ -1,8 +1,9 @@
 #ifndef UACCESS_H
 #define UACCESS_H
-extern void *__user_addr_min, *__user_addr_max;
 
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+#include <linux/compiler.h>
+
+extern void *__user_addr_min, *__user_addr_max;
 
 static inline void __chk_user_ptr(const volatile void *p, size_t size)
 {
@@ -13,7 +14,7 @@ static inline void __chk_user_ptr(const volatile void *p, size_t size)
 ({								\
 	typeof(ptr) __pu_ptr = (ptr);				\
 	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
-	ACCESS_ONCE(*(__pu_ptr)) = x;				\
+	WRITE_ONCE(*(__pu_ptr), x);				\
 	0;							\
 })
 
@@ -21,7 +22,7 @@ static inline void __chk_user_ptr(const volatile void *p, size_t size)
 ({								\
 	typeof(ptr) __pu_ptr = (ptr);				\
 	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
-	x = ACCESS_ONCE(*(__pu_ptr));				\
+	x = READ_ONCE(*(__pu_ptr));				\
 	0;							\
 })
 
-- 
2.7.4

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

[Christian Borntraeger]

On 11/24/2016 11:25 AM, Mark Rutland wrote:
> Despite living under drivers/ vringh.c is also used as part of the userspace
> virtio tools. Before we can kill off the ACCESS_ONCE()definition in the tools,
> we must convert vringh.c to use {READ,WRITE}_ONCE().
> 
> This patch does so, along with the required include of <linux/compiler.h> for
> the relevant definitions. The userspace tools provide their own definitions in
> their own <linux/compiler.h>.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> ---
>  drivers/vhost/vringh.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 3bb02c6..bb8971f 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -3,6 +3,7 @@
>   *
>   * Since these may be in userspace, we use (inline) accessors.
>   */
> +#include <linux/compiler.h>
>  #include <linux/module.h>
>  #include <linux/vringh.h>
>  #include <linux/virtio_ring.h>
> @@ -820,13 +821,13 @@ EXPORT_SYMBOL(vringh_need_notify_user);
>  static inline int getu16_kern(const struct vringh *vrh,
>  			      u16 *val, const __virtio16 *p)
>  {
> -	*val = vringh16_to_cpu(vrh, ACCESS_ONCE(*p));
> +	*val = vringh16_to_cpu(vrh, READ_ONCE(*p));
>  	return 0;
>  }
> 
>  static inline int putu16_kern(const struct vringh *vrh, __virtio16 *p, u16 val)
>  {
> -	ACCESS_ONCE(*p) = cpu_to_vringh16(vrh, val);
> +	WRITE_ONCE(*p, cpu_to_vringh16(vrh, val));
>  	return 0;
>  }
> 

Makes sense

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

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

[Cornelia Huck]

On Thu, 24 Nov 2016 10:25:13 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> Despite living under drivers/ vringh.c is also used as part of the userspace
> virtio tools. Before we can kill off the ACCESS_ONCE()definition in the tools,
> we must convert vringh.c to use {READ,WRITE}_ONCE().
> 
> This patch does so, along with the required include of <linux/compiler.h> for
> the relevant definitions. The userspace tools provide their own definitions in
> their own <linux/compiler.h>.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> ---
>  drivers/vhost/vringh.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

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

[Jason Wang]

On 2016年11月24日 18:25, Mark Rutland wrote:
> Despite living under drivers/ vringh.c is also used as part of the userspace
> virtio tools. Before we can kill off the ACCESS_ONCE()definition in the tools,
> we must convert vringh.c to use {READ,WRITE}_ONCE().
>
> This patch does so, along with the required include of <linux/compiler.h> for
> the relevant definitions. The userspace tools provide their own definitions in
> their own <linux/compiler.h>.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> ---
>   drivers/vhost/vringh.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 3bb02c6..bb8971f 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -3,6 +3,7 @@
>    *
>    * Since these may be in userspace, we use (inline) accessors.
>    */
> +#include <linux/compiler.h>
>   #include <linux/module.h>
>   #include <linux/vringh.h>
>   #include <linux/virtio_ring.h>
> @@ -820,13 +821,13 @@ EXPORT_SYMBOL(vringh_need_notify_user);
>   static inline int getu16_kern(const struct vringh *vrh,
>   			      u16 *val, const __virtio16 *p)
>   {
> -	*val = vringh16_to_cpu(vrh, ACCESS_ONCE(*p));
> +	*val = vringh16_to_cpu(vrh, READ_ONCE(*p));
>   	return 0;
>   }
>   
>   static inline int putu16_kern(const struct vringh *vrh, __virtio16 *p, u16 val)
>   {
> -	ACCESS_ONCE(*p) = cpu_to_vringh16(vrh, val);
> +	WRITE_ONCE(*p, cpu_to_vringh16(vrh, val));
>   	return 0;
>   }
>   

Reviewed-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h

[Cornelia Huck]

On Thu, 24 Nov 2016 10:25:14 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> As a step towards killing off ACCESS_ONCE, use {READ,WRITE}_ONCE() for the
> virtio tools uaccess primitives, pulling these in from <linux/compiler.h>.
> 
> With this done, we can kill off the now-unused ACCESS_ONCE() definition.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> ---
>  tools/virtio/linux/uaccess.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH 3/3] tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h

[Jason Wang]

On 2016年11月24日 18:25, Mark Rutland wrote:
> As a step towards killing off ACCESS_ONCE, use {READ,WRITE}_ONCE() for the
> virtio tools uaccess primitives, pulling these in from <linux/compiler.h>.
>
> With this done, we can kill off the now-unused ACCESS_ONCE() definition.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> ---
>   tools/virtio/linux/uaccess.h | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h
> index 0a578fe..fa05d01 100644
> --- a/tools/virtio/linux/uaccess.h
> +++ b/tools/virtio/linux/uaccess.h
> @@ -1,8 +1,9 @@
>   #ifndef UACCESS_H
>   #define UACCESS_H
> -extern void *__user_addr_min, *__user_addr_max;
>   
> -#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> +#include <linux/compiler.h>
> +
> +extern void *__user_addr_min, *__user_addr_max;
>   
>   static inline void __chk_user_ptr(const volatile void *p, size_t size)
>   {
> @@ -13,7 +14,7 @@ static inline void __chk_user_ptr(const volatile void *p, size_t size)
>   ({								\
>   	typeof(ptr) __pu_ptr = (ptr);				\
>   	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
> -	ACCESS_ONCE(*(__pu_ptr)) = x;				\
> +	WRITE_ONCE(*(__pu_ptr), x);				\
>   	0;							\
>   })
>   
> @@ -21,7 +22,7 @@ static inline void __chk_user_ptr(const volatile void *p, size_t size)
>   ({								\
>   	typeof(ptr) __pu_ptr = (ptr);				\
>   	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
> -	x = ACCESS_ONCE(*(__pu_ptr));				\
> +	x = READ_ONCE(*(__pu_ptr));				\
>   	0;							\
>   })
>   

Reviewed-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

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.

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.

So I worry it's kind of like volatile in this respect,
too easy to overuse.


> Mark Rutland (3):
>   tools/virtio: fix READ_ONCE()
>   vringh: kill off ACCESS_ONCE()
>   tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h
> 
>  drivers/vhost/vringh.c        | 5 +++--
>  tools/virtio/linux/compiler.h | 2 +-
>  tools/virtio/linux/uaccess.h  | 9 +++++----
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> -- 
> 2.7.4

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

[Mark Rutland]

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.

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

[Christian Borntraeger]

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---

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

[Mark Rutland]

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.

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

[Peter Zijlstra]

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);					\
})

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

[Peter Zijlstra]

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);					\
> })

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

[Boqun Feng]

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);					\
> > })

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

[Dmitry Vyukov via Virtualization]

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.

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

[Mark Rutland]

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

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

[Peter Zijlstra]

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().

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

[Mark Rutland]

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.

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

[Christian Borntraeger]

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().
> 

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

[Mark Rutland]

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.

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

[Peter Zijlstra]

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().

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

[Christian Borntraeger]

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.

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

[Michael S. Tsirkin]

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().
> > 

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

[Christian Borntraeger]

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().
>>>
> 

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

[Dmitry Vyukov via Virtualization]

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]);
                                 ^
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Mark Rutland]

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.

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

[Linus Torvalds]

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

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

[Mark Rutland]

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.

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

[Linus Torvalds]

On Fri, Nov 25, 2016 at 10:07 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> 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.

It has never really been much of a problem, and quite frankly, the
solution would never be to add _another_ crazy new function that will
just confuse everybody.

If you have code that depends on atomicity of READ_ONCE() and friends,
then you should add the appropriate built-time assert to *your* code.
Not to some random generic function that others care about and that
others do _not_ have problems with.

So if you have a data structure in virtio that is
architecture-dependent and might not be a word size, you add the

   BUILD_BUG_ON(sizeof(mytype) > sizeof(long));

or whatever. With a big comment saying "this needs to actually fit in
a single register so that we can do atomic accesses".

You do not screw it up for everybody else.

                Linus

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

[Mark Rutland]

On Fri, Nov 25, 2016 at 01:40:44PM +0100, Peter Zijlstra wrote:
> 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?

> /*
>  * Provide accessors for Single-Copy atomicy.
>  *
>  * That is, ensure that machine word sized loads/stores to naturally
>  * aligned variables are single instructions.

Minor nit: this sounds like we *only* support the machine word size,
whereas (excluding alpha IIRC) we can generally acccess power-of-two
sizes from byte up to that.

So perhaps:

	That is, ensure that loads/stores are made with single
	instructions, where the machine can perform a tear-free access
	of that size.

>  * 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);					\
> })

Modulo your type comment, and mine above, this looks good to me.

Thanks,
Mark.

Re: [PATCH 1/3] tools/virtio: fix READ_ONCE()

[Cornelia Huck]

On Thu, 24 Nov 2016 10:25:12 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> The virtio tools implementation of READ_ONCE() has a single parameter called
> 'var', but erroneously refers to 'val' for its cast, and thus won't work unless
> there's a variable of the correct type that happens to be called 'var'.
> 
> Fix this with s/var/val/, making READ_ONCE() work as expected regardless.
> 
> Fixes: a7c490333df3cff5 ("tools/virtio: use virt_xxx barriers")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> ---
>  tools/virtio/linux/compiler.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH 1/3] tools/virtio: fix READ_ONCE()

[Jason Wang]

On 2016年11月24日 18:25, Mark Rutland wrote:
> The virtio tools implementation of READ_ONCE() has a single parameter called
> 'var', but erroneously refers to 'val' for its cast, and thus won't work unless
> there's a variable of the correct type that happens to be called 'var'.
>
> Fix this with s/var/val/, making READ_ONCE() work as expected regardless.
>
> Fixes: a7c490333df3cff5 ("tools/virtio: use virt_xxx barriers")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> ---
>   tools/virtio/linux/compiler.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h
> index 845960e..c9ccfd4 100644
> --- a/tools/virtio/linux/compiler.h
> +++ b/tools/virtio/linux/compiler.h
> @@ -4,6 +4,6 @@
>   #define WRITE_ONCE(var, val) \
>   	(*((volatile typeof(val) *)(&(var))) = (val))
>   
> -#define READ_ONCE(var) (*((volatile typeof(val) *)(&(var))))
> +#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
>   
>   #endif

Reviewed-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization