Hi all,
This all started when we realized that
atomic64_t v;
...
typeof(v.counter) my_val = atomic64_set(&v, VAL);
is a valid assignment on some architectures, but not on others [1]:
in particular, the assignment is valid on all architectures #define
-ing their atomic64_set() macro to WRITE_ONCE() (which is currently
evaluated).
Mark suggested to clean up all non-portable users of atomic*_set(),
and to prevent WRITE_ONCE() from being evaluated [2]; this resulted
in this first attempt/patchset based on Mark's atomics/type-cleanup
branch [3].
Cheers Andrea
[1] https://lkml.kernel.org/r/20190523083013.GA4616@andrea
[2] https://lkml.kernel.org/r/[email protected]
[3] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git atomics/type-cleanup
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jorgen Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Andrea Parri (2):
vmw_vmci: Clean up uses of atomic*_set()
compiler: Prevent evaluation of WRITE_ONCE()
include/linux/compiler.h | 5 ++---
include/linux/vmw_vmci_defs.h | 4 ++--
2 files changed, 4 insertions(+), 5 deletions(-)
--
2.7.4
The primitive vmci_q_set_pointer() relies on atomic*_set() being of
type 'void', but this is a non-portable implementation detail.
Reported-by: Mark Rutland <[email protected]>
Signed-off-by: Andrea Parri <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jorgen Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
---
include/linux/vmw_vmci_defs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
index 0c06178e4985b..eb593868e2e9e 100644
--- a/include/linux/vmw_vmci_defs.h
+++ b/include/linux/vmw_vmci_defs.h
@@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
u64 new_val)
{
#if defined(CONFIG_X86_32)
- return atomic_set((atomic_t *)var, (u32)new_val);
+ atomic_set((atomic_t *)var, (u32)new_val);
#else
- return atomic64_set(var, new_val);
+ atomic64_set(var, new_val);
#endif
}
--
2.7.4
Now that there's no single use of the value of WRITE_ONCE(), change
the implementation to eliminate it.
Suggested-by: Mark Rutland <[email protected]>
Signed-off-by: Andrea Parri <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jorgen Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
---
include/linux/compiler.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8aaf7cd026b06..4024c809a6c63 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -277,12 +277,11 @@ unsigned long read_word_at_a_time(const void *addr)
}
#define WRITE_ONCE(x, val) \
-({ \
+do { \
union { typeof(x) __val; char __c[1]; } __u = \
{ .__val = (__force typeof(x)) (val) }; \
__write_once_size(&(x), __u.__c, sizeof(x)); \
- __u.__val; \
-})
+} while (0)
#endif /* __KERNEL__ */
--
2.7.4
On Fri, May 24, 2019 at 12:35:35PM +0200, Andrea Parri wrote:
> The primitive vmci_q_set_pointer() relies on atomic*_set() being of
> type 'void', but this is a non-portable implementation detail.
>
> Reported-by: Mark Rutland <[email protected]>
> Signed-off-by: Andrea Parri <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Jorgen Hansen <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> ---
> include/linux/vmw_vmci_defs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
> index 0c06178e4985b..eb593868e2e9e 100644
> --- a/include/linux/vmw_vmci_defs.h
> +++ b/include/linux/vmw_vmci_defs.h
> @@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
> u64 new_val)
> {
> #if defined(CONFIG_X86_32)
> - return atomic_set((atomic_t *)var, (u32)new_val);
> + atomic_set((atomic_t *)var, (u32)new_val);
> #else
> - return atomic64_set(var, new_val);
> + atomic64_set(var, new_val);
> #endif
> }
All that should just die a horrible death. That code is crap.
See:
lkml.kernel.org/r/[email protected]
This would be better titled as:
compiler: don't return a value from WRITE_ONCE()
... since we do want the WRITE_ONCE() itself to be evaluated.
On Fri, May 24, 2019 at 12:35:36PM +0200, Andrea Parri wrote:
> Now that there's no single use of the value of WRITE_ONCE(), change
> the implementation to eliminate it.
I hope that's the case, but it's possible that some macros might be
relying on this, so it's probably worth waiting to see if the kbuild
test robot screams.
Otherwise, I agree that WRITE_ONCE() returning a value is surprising,
and unnecessary. IIRC you said that trying to suport that in other
implementations was painful, so aligning on a non-returning version
sounds reasonable to me.
>
> Suggested-by: Mark Rutland <[email protected]>
> Signed-off-by: Andrea Parri <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Jorgen Hansen <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> ---
> include/linux/compiler.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 8aaf7cd026b06..4024c809a6c63 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -277,12 +277,11 @@ unsigned long read_word_at_a_time(const void *addr)
> }
>
> #define WRITE_ONCE(x, val) \
> -({ \
> +do { \
> union { typeof(x) __val; char __c[1]; } __u = \
> { .__val = (__force typeof(x)) (val) }; \
> __write_once_size(&(x), __u.__c, sizeof(x)); \
> - __u.__val; \
> -})
> +} while (0)
With the title fixed, and assuming that the kbuild test robot doesn't
find uses we've missed:
Acked-by: Mark Rutland <[email protected]>
Thanks,
Mark.
>
> #endif /* __KERNEL__ */
>
> --
> 2.7.4
>
On Fri, May 24, 2019 at 11:53:40AM +0100, Mark Rutland wrote:
>
> This would be better titled as:
>
> compiler: don't return a value from WRITE_ONCE()
No strong opinion here: I'll adopt your suggestion in v2 if there are
no objections. And similarly for the rcu_assign_pointer() patch.
>
> ... since we do want the WRITE_ONCE() itself to be evaluated.
>
> On Fri, May 24, 2019 at 12:35:36PM +0200, Andrea Parri wrote:
> > Now that there's no single use of the value of WRITE_ONCE(), change
> > the implementation to eliminate it.
>
> I hope that's the case, but it's possible that some macros might be
> relying on this, so it's probably worth waiting to see if the kbuild
> test robot screams.
Absolutely! Does kbuild process your tree? I might be worth to apply
the patch to just see what kbuild 'think' about it...
>
> Otherwise, I agree that WRITE_ONCE() returning a value is surprising,
> and unnecessary. IIRC you said that trying to suport that in other
> implementations was painful, so aligning on a non-returning version
> sounds reasonable to me.
And I should probably also modify the few #define-s under tools/ (that
I missed in this iteration...)
Thanks,
Andrea
On Fri, May 24, 2019 at 12:39:34PM +0200, Peter Zijlstra wrote:
> On Fri, May 24, 2019 at 12:35:35PM +0200, Andrea Parri wrote:
> > The primitive vmci_q_set_pointer() relies on atomic*_set() being of
> > type 'void', but this is a non-portable implementation detail.
> >
> > Reported-by: Mark Rutland <[email protected]>
> > Signed-off-by: Andrea Parri <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Jorgen Hansen <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > ---
> > include/linux/vmw_vmci_defs.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
> > index 0c06178e4985b..eb593868e2e9e 100644
> > --- a/include/linux/vmw_vmci_defs.h
> > +++ b/include/linux/vmw_vmci_defs.h
> > @@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
> > u64 new_val)
> > {
> > #if defined(CONFIG_X86_32)
> > - return atomic_set((atomic_t *)var, (u32)new_val);
> > + atomic_set((atomic_t *)var, (u32)new_val);
> > #else
> > - return atomic64_set(var, new_val);
> > + atomic64_set(var, new_val);
> > #endif
> > }
>
> All that should just die a horrible death. That code is crap.
>
> See:
>
> lkml.kernel.org/r/[email protected]
I agree, Peter's patch should be the thing that is applied, not this
one.
thanks,
greg k-h
On Fri, May 24, 2019 at 12:39:34PM +0200, Peter Zijlstra wrote:
> On Fri, May 24, 2019 at 12:35:35PM +0200, Andrea Parri wrote:
> > The primitive vmci_q_set_pointer() relies on atomic*_set() being of
> > type 'void', but this is a non-portable implementation detail.
> >
> > Reported-by: Mark Rutland <[email protected]>
> > Signed-off-by: Andrea Parri <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Jorgen Hansen <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > ---
> > include/linux/vmw_vmci_defs.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
> > index 0c06178e4985b..eb593868e2e9e 100644
> > --- a/include/linux/vmw_vmci_defs.h
> > +++ b/include/linux/vmw_vmci_defs.h
> > @@ -759,9 +759,9 @@ static inline void vmci_q_set_pointer(atomic64_t *var,
> > u64 new_val)
> > {
> > #if defined(CONFIG_X86_32)
> > - return atomic_set((atomic_t *)var, (u32)new_val);
> > + atomic_set((atomic_t *)var, (u32)new_val);
> > #else
> > - return atomic64_set(var, new_val);
> > + atomic64_set(var, new_val);
> > #endif
> > }
>
> All that should just die a horrible death. That code is crap.
>
> See:
>
> lkml.kernel.org/r/[email protected]
I see, that was indeed 'racy' with my patch! ;-) Thank you!
Andrea
> > All that should just die a horrible death. That code is crap.
> >
> > See:
> >
> > lkml.kernel.org/r/[email protected]
>
> I agree, Peter's patch should be the thing that is applied, not this
> one.
Thanks for the confirmation, Greg. I'll drop mine.
At this time, it's not clear to me where Peter's patch will be applied,
and where/whether I should rebase 2/2 (if there is interested in that):
Please let me know.
Thanks,
Andrea