The VMCI driver is abusing atomic64_t and atomic_t, there is no actual
atomic RmW operations around.
Rewrite the code to use a regular u64 with READ_ONCE() and
WRITE_ONCE() and a cast to 'unsigned long'. This fully preserves
whatever broken there was (it's not endian-safe for starters, and also
looks to be missing ordering).
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
include/linux/vmw_vmci_defs.h | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
--- a/include/linux/vmw_vmci_defs.h
+++ b/include/linux/vmw_vmci_defs.h
@@ -438,8 +438,8 @@ enum {
struct vmci_queue_header {
/* All fields are 64bit and aligned. */
struct vmci_handle handle; /* Identifier. */
- atomic64_t producer_tail; /* Offset in this queue. */
- atomic64_t consumer_head; /* Offset in peer queue. */
+ u64 producer_tail; /* Offset in this queue. */
+ u64 consumer_head; /* Offset in peer queue. */
};
/*
@@ -740,13 +740,9 @@ static inline void *vmci_event_data_payl
* prefix will be used, so correctness isn't an issue, but using a
* 64bit operation still adds unnecessary overhead.
*/
-static inline u64 vmci_q_read_pointer(atomic64_t *var)
+static inline u64 vmci_q_read_pointer(u64 *var)
{
-#if defined(CONFIG_X86_32)
- return atomic_read((atomic_t *)var);
-#else
- return atomic64_read(var);
-#endif
+ return READ_ONCE(*(unsigned long *)var);
}
/*
@@ -755,23 +751,17 @@ static inline u64 vmci_q_read_pointer(at
* never exceeds a 32bit value in this case. On 32bit SMP, using a
* locked cmpxchg8b adds unnecessary overhead.
*/
-static inline void vmci_q_set_pointer(atomic64_t *var,
- u64 new_val)
+static inline void vmci_q_set_pointer(u64 *var, u64 new_val)
{
-#if defined(CONFIG_X86_32)
- return atomic_set((atomic_t *)var, (u32)new_val);
-#else
- return atomic64_set(var, new_val);
-#endif
+ /* XXX buggered on big-endian */
+ WRITE_ONCE(*(unsigned long *)var, (unsigned long)new_val);
}
/*
* Helper to add a given offset to a head or tail pointer. Wraps the
* value of the pointer around the max size of the queue.
*/
-static inline void vmci_qp_add_pointer(atomic64_t *var,
- size_t add,
- u64 size)
+static inline void vmci_qp_add_pointer(u64 *var, size_t add, u64 size)
{
u64 new_val = vmci_q_read_pointer(var);
@@ -848,8 +838,8 @@ static inline void vmci_q_header_init(st
const struct vmci_handle handle)
{
q_header->handle = handle;
- atomic64_set(&q_header->producer_tail, 0);
- atomic64_set(&q_header->consumer_head, 0);
+ q_header->producer_tail = 0;
+ q_header->consumer_head = 0;
}
/*
> On 6 Jun 2019, at 11:34, Peter Zijlstra <[email protected]> wrote:
>
>
> The VMCI driver is abusing atomic64_t and atomic_t, there is no actual
> atomic RmW operations around.
>
> Rewrite the code to use a regular u64 with READ_ONCE() and
> WRITE_ONCE() and a cast to 'unsigned long'. This fully preserves
> whatever broken there was (it's not endian-safe for starters, and also
> looks to be missing ordering).
Thanks for the cleanup.
This code is only intended for use with the vmci device driver, and that is X86 only, so during the original upstreaming no effort was made to make this work correctly on anything else.
With that in mind, it should be fine to drop the unsigned long * type casts, since the introduction of the 32 bit operations were only done to avoid an issue with cmpxchg8b on 32-bit, and just doing straight writes avoids that too.
We’ll be updating the vmci device driver to work on other architectures soonish, so will be adding barriers to enforce ordering as well at that point. If you want to leave your patch as is, we can address the type casting then.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> include/linux/vmw_vmci_defs.h | 30 ++++++++++--------------------
> 1 file changed, 10 insertions(+), 20 deletions(-)
>
> --- a/include/linux/vmw_vmci_defs.h
> +++ b/include/linux/vmw_vmci_defs.h
> @@ -438,8 +438,8 @@ enum {
> struct vmci_queue_header {
> /* All fields are 64bit and aligned. */
> struct vmci_handle handle; /* Identifier. */
> - atomic64_t producer_tail; /* Offset in this queue. */
> - atomic64_t consumer_head; /* Offset in peer queue. */
> + u64 producer_tail; /* Offset in this queue. */
> + u64 consumer_head; /* Offset in peer queue. */
> };
>
> /*
> @@ -740,13 +740,9 @@ static inline void *vmci_event_data_payl
> * prefix will be used, so correctness isn't an issue, but using a
> * 64bit operation still adds unnecessary overhead.
> */
> -static inline u64 vmci_q_read_pointer(atomic64_t *var)
> +static inline u64 vmci_q_read_pointer(u64 *var)
> {
> -#if defined(CONFIG_X86_32)
> - return atomic_read((atomic_t *)var);
> -#else
> - return atomic64_read(var);
> -#endif
> + return READ_ONCE(*(unsigned long *)var);
> }
>
> /*
> @@ -755,23 +751,17 @@ static inline u64 vmci_q_read_pointer(at
> * never exceeds a 32bit value in this case. On 32bit SMP, using a
> * locked cmpxchg8b adds unnecessary overhead.
> */
> -static inline void vmci_q_set_pointer(atomic64_t *var,
> - u64 new_val)
> +static inline void vmci_q_set_pointer(u64 *var, u64 new_val)
> {
> -#if defined(CONFIG_X86_32)
> - return atomic_set((atomic_t *)var, (u32)new_val);
> -#else
> - return atomic64_set(var, new_val);
> -#endif
> + /* XXX buggered on big-endian */
> + WRITE_ONCE(*(unsigned long *)var, (unsigned long)new_val);
> }
>
> /*
> * Helper to add a given offset to a head or tail pointer. Wraps the
> * value of the pointer around the max size of the queue.
> */
> -static inline void vmci_qp_add_pointer(atomic64_t *var,
> - size_t add,
> - u64 size)
> +static inline void vmci_qp_add_pointer(u64 *var, size_t add, u64 size)
> {
> u64 new_val = vmci_q_read_pointer(var);
>
> @@ -848,8 +838,8 @@ static inline void vmci_q_header_init(st
> const struct vmci_handle handle)
> {
> q_header->handle = handle;
> - atomic64_set(&q_header->producer_tail, 0);
> - atomic64_set(&q_header->consumer_head, 0);
> + q_header->producer_tail = 0;
> + q_header->consumer_head = 0;
> }
>
> /*
On Thu, Jun 06, 2019 at 03:54:24PM +0000, Jorgen Hansen wrote:
>
>
> > On 6 Jun 2019, at 11:34, Peter Zijlstra <[email protected]> wrote:
> >
> >
> > The VMCI driver is abusing atomic64_t and atomic_t, there is no actual
> > atomic RmW operations around.
> >
> > Rewrite the code to use a regular u64 with READ_ONCE() and
> > WRITE_ONCE() and a cast to 'unsigned long'. This fully preserves
> > whatever broken there was (it's not endian-safe for starters, and also
> > looks to be missing ordering).
>
> Thanks for the cleanup.
>
> This code is only intended for use with the vmci device driver, and
> that is X86 only, so during the original upstreaming no effort was
> made to make this work correctly on anything else.
Even x86 needs compiler barriers to ensure the compiler emits the
instructions in the expected order.
> We’ll be updating the vmci device driver to work on other
> architectures soonish, so will be adding barriers to enforce ordering
> as well at that point. If you want to leave your patch as is, we can
> address the type casting then.
You might want to have a look at smp_store_release() and
smp_load_acquire() I suppose.
On Thu, Jun 06, 2019 at 11:34:28AM +0200, Peter Zijlstra wrote:
>
> The VMCI driver is abusing atomic64_t and atomic_t, there is no actual
> atomic RmW operations around.
>
> Rewrite the code to use a regular u64 with READ_ONCE() and
> WRITE_ONCE() and a cast to 'unsigned long'. This fully preserves
> whatever broken there was (it's not endian-safe for starters, and also
> looks to be missing ordering).
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> include/linux/vmw_vmci_defs.h | 30 ++++++++++--------------------
> 1 file changed, 10 insertions(+), 20 deletions(-)
Now applied.
It's pretty horrid code, at least it's a tiny bit better now...
thanks,
greg k-h
On Thu, Jun 06, 2019 at 03:54:24PM +0000, Jorgen Hansen wrote:
>
>
> > On 6 Jun 2019, at 11:34, Peter Zijlstra <[email protected]> wrote:
> >
> >
> > The VMCI driver is abusing atomic64_t and atomic_t, there is no actual
> > atomic RmW operations around.
> >
> > Rewrite the code to use a regular u64 with READ_ONCE() and
> > WRITE_ONCE() and a cast to 'unsigned long'. This fully preserves
> > whatever broken there was (it's not endian-safe for starters, and also
> > looks to be missing ordering).
>
> Thanks for the cleanup.
>
> This code is only intended for use with the vmci device driver, and
> that is X86 only, so during the original upstreaming no effort was
> made to make this work correctly on anything else.
>
> With that in mind, it should be fine to drop the unsigned long * type
> casts, since the introduction of the 32 bit operations were only done
> to avoid an issue with cmpxchg8b on 32-bit, and just doing straight
> writes avoids that too.
>
> We’ll be updating the vmci device driver to work on other
> architectures soonish, so will be adding barriers to enforce ordering
> as well at that point. If you want to leave your patch as is, we can
> address the type casting then.
I've already applied it.
greg k-h