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

Groups > linux.kernel > #1529132 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-24 11:30 +0100
    [PATCH 2/3] vringh: kill off ACCESS_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-24 11:30 +0100
      Re: [PATCH 2/3] vringh: kill off ACCESS_ONCE() Christian Borntraeger <borntraeger@de.ibm.com> - 2016-11-24 12:20 +0100
      Re: [PATCH 2/3] vringh: kill off ACCESS_ONCE() Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-11-24 12:40 +0100
      Re: [PATCH 2/3] vringh: kill off ACCESS_ONCE() Jason Wang <jasowang@redhat.com> - 2016-11-25 03:50 +0100
    [PATCH 1/3] tools/virtio: fix READ_ONCE() Mark Rutland <mark.rutland@arm.com> - 2016-11-24 11:30 +0100
      Re: [PATCH 1/3] tools/virtio: fix READ_ONCE() Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-11-24 12:40 +0100
      Re: [PATCH 1/3] tools/virtio: fix READ_ONCE() Jason Wang <jasowang@redhat.com> - 2016-11-25 03:40 +0100
    [PATCH 3/3] tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h Mark Rutland <mark.rutland@arm.com> - 2016-11-24 11:30 +0100
      Re: [PATCH 3/3] tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-11-24 12:40 +0100
      Re: [PATCH 3/3] tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h Jason Wang <jasowang@redhat.com> - 2016-11-25 03:50 +0100
    Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() "Michael S. Tsirkin" <mst@redhat.com> - 2016-11-24 21:40 +0100
      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 →

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

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.

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

[toc] | [next] | [standalone]

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

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

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

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

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>

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

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

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>

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

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

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>

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

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

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

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

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

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>

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

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

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>

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

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

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

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

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

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>

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

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

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>

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

#1529690

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

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

#1530130

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] | [prev] | [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]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web