[PATCH] mm/debug: add a cast to u64 for atomic64_read()

[PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Qian Cai]

atomic64_read() on ppc64le returns "long int", so fix the same way as
the commit d549f545e690 ("drm/virtio: use %llu format string form
atomic64_t") by adding a cast to u64, which makes it work on all arches.

In file included from ./include/linux/printk.h:7,
                 from ./include/linux/kernel.h:15,
                 from mm/debug.c:9:
mm/debug.c: In function 'dump_mm':
./include/linux/kern_levels.h:5:18: warning: format '%llx' expects
argument of type 'long long unsigned int', but argument 19 has type
'long int' [-Wformat=]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^~~~~~
./include/linux/kern_levels.h:8:20: note: in expansion of macro
'KERN_SOH'
 #define KERN_EMERG KERN_SOH "0" /* system is unusable */
                    ^~~~~~~~
./include/linux/printk.h:297:9: note: in expansion of macro 'KERN_EMERG'
  printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
         ^~~~~~~~~~
mm/debug.c:133:2: note: in expansion of macro 'pr_emerg'
  pr_emerg("mm %px mmap %px seqnum %llu task_size %lu\n"
  ^~~~~~~~
mm/debug.c:140:17: note: format string is defined here
   "pinned_vm %llx data_vm %lx exec_vm %lx stack_vm %lx\n"
              ~~~^
              %lx

Fixes: 70f8a3ca68d3 ("mm: make mm->pinned_vm an atomic64 counter")
Signed-off-by: Qian Cai <cai@lca.pw>
---
 mm/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/debug.c b/mm/debug.c
index c0b31b6c3877..45d9eb77b84e 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -168,7 +168,7 @@ void dump_mm(const struct mm_struct *mm)
 		mm_pgtables_bytes(mm),
 		mm->map_count,
 		mm->hiwater_rss, mm->hiwater_vm, mm->total_vm, mm->locked_vm,
-		atomic64_read(&mm->pinned_vm),
+		(u64)atomic64_read(&mm->pinned_vm),
 		mm->data_vm, mm->exec_vm, mm->stack_vm,
 		mm->start_code, mm->end_code, mm->start_data, mm->end_data,
 		mm->start_brk, mm->brk, mm->start_stack,
-- 
2.17.2 (Apple Git-113)

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Davidlohr Bueso]

On Sun, 10 Mar 2019, Qian Cai wrote:

>atomic64_read() on ppc64le returns "long int", so fix the same way as
>the commit d549f545e690 ("drm/virtio: use %llu format string form
>atomic64_t") by adding a cast to u64, which makes it work on all arches.
>
>In file included from ./include/linux/printk.h:7,
>                 from ./include/linux/kernel.h:15,
>                 from mm/debug.c:9:
>mm/debug.c: In function 'dump_mm':
>./include/linux/kern_levels.h:5:18: warning: format '%llx' expects
>argument of type 'long long unsigned int', but argument 19 has type
>'long int' [-Wformat=]
> #define KERN_SOH "\001"  /* ASCII Start Of Header */
>                  ^~~~~~
>./include/linux/kern_levels.h:8:20: note: in expansion of macro
>'KERN_SOH'
> #define KERN_EMERG KERN_SOH "0" /* system is unusable */
>                    ^~~~~~~~
>./include/linux/printk.h:297:9: note: in expansion of macro 'KERN_EMERG'
>  printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
>         ^~~~~~~~~~
>mm/debug.c:133:2: note: in expansion of macro 'pr_emerg'
>  pr_emerg("mm %px mmap %px seqnum %llu task_size %lu\n"
>  ^~~~~~~~
>mm/debug.c:140:17: note: format string is defined here
>   "pinned_vm %llx data_vm %lx exec_vm %lx stack_vm %lx\n"
>              ~~~^
>              %lx
>
>Fixes: 70f8a3ca68d3 ("mm: make mm->pinned_vm an atomic64 counter")
>Signed-off-by: Qian Cai <cai@lca.pw>

Acked-by: Davidlohr Bueso <dbueso@suse.de>

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Jason Gunthorpe]

On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> On Sun, 10 Mar 2019, Qian Cai wrote:
> 
> > atomic64_read() on ppc64le returns "long int", so fix the same way as
> > the commit d549f545e690 ("drm/virtio: use %llu format string form
> > atomic64_t") by adding a cast to u64, which makes it work on all arches.
> > 
> > In file included from ./include/linux/printk.h:7,
> >                 from ./include/linux/kernel.h:15,
> >                 from mm/debug.c:9:
> > mm/debug.c: In function 'dump_mm':
> > ./include/linux/kern_levels.h:5:18: warning: format '%llx' expects
> > argument of type 'long long unsigned int', but argument 19 has type
> > 'long int' [-Wformat=]
> > #define KERN_SOH "\001"  /* ASCII Start Of Header */
> >                  ^~~~~~
> > ./include/linux/kern_levels.h:8:20: note: in expansion of macro
> > 'KERN_SOH'
> > #define KERN_EMERG KERN_SOH "0" /* system is unusable */
> >                    ^~~~~~~~
> > ./include/linux/printk.h:297:9: note: in expansion of macro 'KERN_EMERG'
> >  printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> >         ^~~~~~~~~~
> > mm/debug.c:133:2: note: in expansion of macro 'pr_emerg'
> >  pr_emerg("mm %px mmap %px seqnum %llu task_size %lu\n"
> >  ^~~~~~~~
> > mm/debug.c:140:17: note: format string is defined here
> >   "pinned_vm %llx data_vm %lx exec_vm %lx stack_vm %lx\n"
> >              ~~~^
> >              %lx
> > 
> > Fixes: 70f8a3ca68d3 ("mm: make mm->pinned_vm an atomic64 counter")
> > Signed-off-by: Qian Cai <cai@lca.pw>
> 
> Acked-by: Davidlohr Bueso <dbueso@suse.de>

Not saying this patch shouldn't go ahead..

But is there a special reason the atomic64*'s on ppc don't use the u64
type like other archs? Seems like a better thing to fix than adding
casts all over the place.

Jason

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Qian Cai]

On Mon, 2019-03-11 at 12:21 +0000, Jason Gunthorpe wrote:
> On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > On Sun, 10 Mar 2019, Qian Cai wrote:
> > 
> > > atomic64_read() on ppc64le returns "long int", so fix the same way as
> > > the commit d549f545e690 ("drm/virtio: use %llu format string form
> > > atomic64_t") by adding a cast to u64, which makes it work on all arches.
> > > 
> > > In file included from ./include/linux/printk.h:7,
> > >                 from ./include/linux/kernel.h:15,
> > >                 from mm/debug.c:9:
> > > mm/debug.c: In function 'dump_mm':
> > > ./include/linux/kern_levels.h:5:18: warning: format '%llx' expects
> > > argument of type 'long long unsigned int', but argument 19 has type
> > > 'long int' [-Wformat=]
> > > #define KERN_SOH "\001"  /* ASCII Start Of Header */
> > >                  ^~~~~~
> > > ./include/linux/kern_levels.h:8:20: note: in expansion of macro
> > > 'KERN_SOH'
> > > #define KERN_EMERG KERN_SOH "0" /* system is unusable */
> > >                    ^~~~~~~~
> > > ./include/linux/printk.h:297:9: note: in expansion of macro 'KERN_EMERG'
> > >  printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> > >         ^~~~~~~~~~
> > > mm/debug.c:133:2: note: in expansion of macro 'pr_emerg'
> > >  pr_emerg("mm %px mmap %px seqnum %llu task_size %lu\n"
> > >  ^~~~~~~~
> > > mm/debug.c:140:17: note: format string is defined here
> > >   "pinned_vm %llx data_vm %lx exec_vm %lx stack_vm %lx\n"
> > >              ~~~^
> > >              %lx
> > > 
> > > Fixes: 70f8a3ca68d3 ("mm: make mm->pinned_vm an atomic64 counter")
> > > Signed-off-by: Qian Cai <cai@lca.pw>
> > 
> > Acked-by: Davidlohr Bueso <dbueso@suse.de>
> 
> Not saying this patch shouldn't go ahead..
> 
> But is there a special reason the atomic64*'s on ppc don't use the u64
> type like other archs? Seems like a better thing to fix than adding
> casts all over the place.
> 

A bit of history here,

https://patchwork.kernel.org/patch/7344011/#15495901

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Qian Cai]

On Mon, 2019-03-11 at 12:21 +0000, Jason Gunthorpe wrote:
> On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > On Sun, 10 Mar 2019, Qian Cai wrote:
> > 
> > > atomic64_read() on ppc64le returns "long int", so fix the same way as
> > > the commit d549f545e690 ("drm/virtio: use %llu format string form
> > > atomic64_t") by adding a cast to u64, which makes it work on all arches.
> > > 
> > > In file included from ./include/linux/printk.h:7,
> > >                 from ./include/linux/kernel.h:15,
> > >                 from mm/debug.c:9:
> > > mm/debug.c: In function 'dump_mm':
> > > ./include/linux/kern_levels.h:5:18: warning: format '%llx' expects
> > > argument of type 'long long unsigned int', but argument 19 has type
> > > 'long int' [-Wformat=]
> > > #define KERN_SOH "\001"  /* ASCII Start Of Header */
> > >                  ^~~~~~
> > > ./include/linux/kern_levels.h:8:20: note: in expansion of macro
> > > 'KERN_SOH'
> > > #define KERN_EMERG KERN_SOH "0" /* system is unusable */
> > >                    ^~~~~~~~
> > > ./include/linux/printk.h:297:9: note: in expansion of macro 'KERN_EMERG'
> > >  printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> > >         ^~~~~~~~~~
> > > mm/debug.c:133:2: note: in expansion of macro 'pr_emerg'
> > >  pr_emerg("mm %px mmap %px seqnum %llu task_size %lu\n"
> > >  ^~~~~~~~
> > > mm/debug.c:140:17: note: format string is defined here
> > >   "pinned_vm %llx data_vm %lx exec_vm %lx stack_vm %lx\n"
> > >              ~~~^
> > >              %lx
> > > 
> > > Fixes: 70f8a3ca68d3 ("mm: make mm->pinned_vm an atomic64 counter")
> > > Signed-off-by: Qian Cai <cai@lca.pw>
> > 
> > Acked-by: Davidlohr Bueso <dbueso@suse.de>
> 
> Not saying this patch shouldn't go ahead..
> 
> But is there a special reason the atomic64*'s on ppc don't use the u64
> type like other archs? Seems like a better thing to fix than adding
> casts all over the place.
> 

A bit of history here,

https://patchwork.kernel.org/patch/7344011/#15495901

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Mon, Mar 11, 2019 at 1:21 PM Jason Gunthorpe <jgg@mellanox.com> wrote:
> On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > On Sun, 10 Mar 2019, Qian Cai wrote:

> >
> > Acked-by: Davidlohr Bueso <dbueso@suse.de>
>
> Not saying this patch shouldn't go ahead..
>
> But is there a special reason the atomic64*'s on ppc don't use the u64
> type like other archs? Seems like a better thing to fix than adding
> casts all over the place.

Agreed in principle, but I'd note that it's not just ppc64, but almost all
64-bit architectures. x86-64 and arm64 changed over last year
from returning 'long' to 'long long', apparently as an unintended
side effect of commits 8bf705d13039 ("locking/atomic/x86: Switch
atomic.h to use atomic-instrumented.h") and c0df10812835 ("arm64,
locking/atomics: Use instrumented atomics").

It would be nice to just do the instrumented atomics on all 64-bit
architectures for consistency, but that would be a lot of work, and
would not actually give us additional instrumentation on most of them,
since they don't support KASAN (except s390).

       Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Mon, Mar 11, 2019 at 1:21 PM Jason Gunthorpe <jgg@mellanox.com> wrote:
> On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > On Sun, 10 Mar 2019, Qian Cai wrote:

> >
> > Acked-by: Davidlohr Bueso <dbueso@suse.de>
>
> Not saying this patch shouldn't go ahead..
>
> But is there a special reason the atomic64*'s on ppc don't use the u64
> type like other archs? Seems like a better thing to fix than adding
> casts all over the place.

Agreed in principle, but I'd note that it's not just ppc64, but almost all
64-bit architectures. x86-64 and arm64 changed over last year
from returning 'long' to 'long long', apparently as an unintended
side effect of commits 8bf705d13039 ("locking/atomic/x86: Switch
atomic.h to use atomic-instrumented.h") and c0df10812835 ("arm64,
locking/atomics: Use instrumented atomics").

It would be nice to just do the instrumented atomics on all 64-bit
architectures for consistency, but that would be a lot of work, and
would not actually give us additional instrumentation on most of them,
since they don't support KASAN (except s390).

       Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
>
> On Mon, 2019-03-11 at 12:21 +0000, Jason Gunthorpe wrote:
> > On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > > On Sun, 10 Mar 2019, Qian Cai wrote:
> >
> > Not saying this patch shouldn't go ahead..
> >
> > But is there a special reason the atomic64*'s on ppc don't use the u64
> > type like other archs? Seems like a better thing to fix than adding
> > casts all over the place.
> >
>
> A bit of history here,
>
> https://patchwork.kernel.org/patch/7344011/#15495901

Ah, I had already forgotten about that discussion.

At least the atomic_long part we discussed there has been resolved now
as part of commit b5d47ef9ea5c ("locking/atomics: Switch to generated
atomic-long").

Adding Mark Rutland to Cc, maybe he has some ideas of how to use
the infrastructure he added to use consistent types for atomic64()
on the remaining 64-bit architectures.

     Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
>
> On Mon, 2019-03-11 at 12:21 +0000, Jason Gunthorpe wrote:
> > On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > > On Sun, 10 Mar 2019, Qian Cai wrote:
> >
> > Not saying this patch shouldn't go ahead..
> >
> > But is there a special reason the atomic64*'s on ppc don't use the u64
> > type like other archs? Seems like a better thing to fix than adding
> > casts all over the place.
> >
>
> A bit of history here,
>
> https://patchwork.kernel.org/patch/7344011/#15495901

Ah, I had already forgotten about that discussion.

At least the atomic_long part we discussed there has been resolved now
as part of commit b5d47ef9ea5c ("locking/atomics: Switch to generated
atomic-long").

Adding Mark Rutland to Cc, maybe he has some ideas of how to use
the infrastructure he added to use consistent types for atomic64()
on the remaining 64-bit architectures.

     Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Peter Zijlstra]

On Mon, Mar 11, 2019 at 03:20:04PM +0100, Arnd Bergmann wrote:
> On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
> >
> > On Mon, 2019-03-11 at 12:21 +0000, Jason Gunthorpe wrote:
> > > On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > > > On Sun, 10 Mar 2019, Qian Cai wrote:
> > >
> > > Not saying this patch shouldn't go ahead..
> > >
> > > But is there a special reason the atomic64*'s on ppc don't use the u64
> > > type like other archs? Seems like a better thing to fix than adding
> > > casts all over the place.

s64 if anything, atomic stuff is signed (although since we have -fwrapv
it doesn't matter one whit).

> > A bit of history here,
> >
> > https://patchwork.kernel.org/patch/7344011/#15495901
> 
> Ah, I had already forgotten about that discussion.
> 
> At least the atomic_long part we discussed there has been resolved now
> as part of commit b5d47ef9ea5c ("locking/atomics: Switch to generated
> atomic-long").
> 
> Adding Mark Rutland to Cc, maybe he has some ideas of how to use
> the infrastructure he added to use consistent types for atomic64()
> on the remaining 64-bit architectures.

A quick count shows there's only 5 definitions of atomic64_t in the
tree, it would be trivial to align them on type.

$ git grep "} atomic64_t"
arch/arc/include/asm/atomic.h:} atomic64_t;
arch/arm/include/asm/atomic.h:} atomic64_t;
arch/x86/include/asm/atomic64_32.h:} atomic64_t;
include/asm-generic/atomic64.h:} atomic64_t;
include/linux/types.h:} atomic64_t;

Note that the one used in _most_ cases, is the one from linux/types.h,
and that is using 'long'. The others, all typically on ILP32 platforms,
obviously must use long long.

I have no objection to changing the types.h one to long long or all of
them to s64. It really shouldn't matter at all.

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Wed, Mar 13, 2019 at 10:19 AM Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Mar 11, 2019 at 03:20:04PM +0100, Arnd Bergmann wrote:
> > On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
> >
> > At least the atomic_long part we discussed there has been resolved now
> > as part of commit b5d47ef9ea5c ("locking/atomics: Switch to generated
> > atomic-long").
> >
> > Adding Mark Rutland to Cc, maybe he has some ideas of how to use
> > the infrastructure he added to use consistent types for atomic64()
> > on the remaining 64-bit architectures.
>
> A quick count shows there's only 5 definitions of atomic64_t in the
> tree, it would be trivial to align them on type.
>
> $ git grep "} atomic64_t"
> arch/arc/include/asm/atomic.h:} atomic64_t;
> arch/arm/include/asm/atomic.h:} atomic64_t;
> arch/x86/include/asm/atomic64_32.h:} atomic64_t;
> include/asm-generic/atomic64.h:} atomic64_t;
> include/linux/types.h:} atomic64_t;

Right, that would make sense as well.

> Note that the one used in _most_ cases, is the one from linux/types.h,
> and that is using 'long'. The others, all typically on ILP32 platforms,
> obviously must use long long.
>
> I have no objection to changing the types.h one to long long or all of
> them to s64. It really shouldn't matter at all.

I thiunk it needs an '__attribute__((aligned(8)))' annotation at least on
x86-32, but it should be harmless to do that everywhere. The
32-bit architectures of course already use a 'long long' base type
(unsigned long long on x86 and arc), but we'd still need to
change all the 64-bit architectures to consistently use s64
in their implementation. This would be the majority of the work, e.g.

arch/powerpc/include/asm/atomic.h:
static __inline__ void atomic64_##op(long a, atomic64_t *v)             \

arch/riscv/include/asm/atomic.h
static __always_inline                                                  \
c_type atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v)     \

arch/sparc/include/asm/atomic_64.h:
long atomic64_##op##_return(long, atomic64_t *);

arch/s390/include/asm/atomic.h:
static inline void atomic64_##op(long i, atomic64_t *v)                 \

arch/mips/include/asm/atomic.h:
static __inline__ void atomic64_##op(long i, atomic64_t * v)                  \

arch/ia64/include/asm/atomic.h:
static __inline__ long                                                  \
ia64_atomic64_##op (__s64 i, atomic64_t *v)                             \

arch/alpha/include/asm/atomic.h:
static __inline__ void atomic64_##op(long i, atomic64_t * v)            \

arch/parisc/include/asm/atomic.h:
static __inline__ s64 atomic64_##op##_return(s64 i, atomic64_t *v)      \

The problem is not that any of those would be hard to change,
it's more that there are so many functions across 10 architectures,
and everything has some subtle differences somewhere.

It would be tempting to use scripts/atomic/* to generate more of
the code in a consistent way, but that is likely to be even more
work and more error-prone at the start.

      Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Wed, Mar 13, 2019 at 10:19 AM Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Mar 11, 2019 at 03:20:04PM +0100, Arnd Bergmann wrote:
> > On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
> >
> > At least the atomic_long part we discussed there has been resolved now
> > as part of commit b5d47ef9ea5c ("locking/atomics: Switch to generated
> > atomic-long").
> >
> > Adding Mark Rutland to Cc, maybe he has some ideas of how to use
> > the infrastructure he added to use consistent types for atomic64()
> > on the remaining 64-bit architectures.
>
> A quick count shows there's only 5 definitions of atomic64_t in the
> tree, it would be trivial to align them on type.
>
> $ git grep "} atomic64_t"
> arch/arc/include/asm/atomic.h:} atomic64_t;
> arch/arm/include/asm/atomic.h:} atomic64_t;
> arch/x86/include/asm/atomic64_32.h:} atomic64_t;
> include/asm-generic/atomic64.h:} atomic64_t;
> include/linux/types.h:} atomic64_t;

Right, that would make sense as well.

> Note that the one used in _most_ cases, is the one from linux/types.h,
> and that is using 'long'. The others, all typically on ILP32 platforms,
> obviously must use long long.
>
> I have no objection to changing the types.h one to long long or all of
> them to s64. It really shouldn't matter at all.

I thiunk it needs an '__attribute__((aligned(8)))' annotation at least on
x86-32, but it should be harmless to do that everywhere. The
32-bit architectures of course already use a 'long long' base type
(unsigned long long on x86 and arc), but we'd still need to
change all the 64-bit architectures to consistently use s64
in their implementation. This would be the majority of the work, e.g.

arch/powerpc/include/asm/atomic.h:
static __inline__ void atomic64_##op(long a, atomic64_t *v)             \

arch/riscv/include/asm/atomic.h
static __always_inline                                                  \
c_type atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v)     \

arch/sparc/include/asm/atomic_64.h:
long atomic64_##op##_return(long, atomic64_t *);

arch/s390/include/asm/atomic.h:
static inline void atomic64_##op(long i, atomic64_t *v)                 \

arch/mips/include/asm/atomic.h:
static __inline__ void atomic64_##op(long i, atomic64_t * v)                  \

arch/ia64/include/asm/atomic.h:
static __inline__ long                                                  \
ia64_atomic64_##op (__s64 i, atomic64_t *v)                             \

arch/alpha/include/asm/atomic.h:
static __inline__ void atomic64_##op(long i, atomic64_t * v)            \

arch/parisc/include/asm/atomic.h:
static __inline__ s64 atomic64_##op##_return(s64 i, atomic64_t *v)      \

The problem is not that any of those would be hard to change,
it's more that there are so many functions across 10 architectures,
and everything has some subtle differences somewhere.

It would be tempting to use scripts/atomic/* to generate more of
the code in a consistent way, but that is likely to be even more
work and more error-prone at the start.

      Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Peter Zijlstra]

On Wed, Mar 13, 2019 at 02:46:55PM +0100, Arnd Bergmann wrote:
> I thiunk it needs an '__attribute__((aligned(8)))' annotation at least on
> x86-32, 

I _hate_ that s64 isn't already natively aligned.

Anyway, yes, unaligned atomics are a _bad_ idea if they work at all.

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Peter Zijlstra]

On Wed, Mar 13, 2019 at 02:46:55PM +0100, Arnd Bergmann wrote:
> It would be tempting to use scripts/atomic/* to generate more of
> the code in a consistent way, but that is likely to be even more
> work and more error-prone at the start.

Those scripts can't do actual implementations, which is the problem here
I think. The architectures really need to implement a whole bunch of
stuff themselves.

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Mark Rutland]

On Wed, Mar 13, 2019 at 10:18:44AM +0100, Peter Zijlstra wrote:
> On Mon, Mar 11, 2019 at 03:20:04PM +0100, Arnd Bergmann wrote:
> > On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
> > >
> > > On Mon, 2019-03-11 at 12:21 +0000, Jason Gunthorpe wrote:
> > > > On Sun, Mar 10, 2019 at 08:58:15PM -0700, Davidlohr Bueso wrote:
> > > > > On Sun, 10 Mar 2019, Qian Cai wrote:
> > > >
> > > > Not saying this patch shouldn't go ahead..
> > > >
> > > > But is there a special reason the atomic64*'s on ppc don't use the u64
> > > > type like other archs? Seems like a better thing to fix than adding
> > > > casts all over the place.
> 
> s64 if anything, atomic stuff is signed (although since we have -fwrapv
> it doesn't matter one whit).
> 
> > > A bit of history here,
> > >
> > > https://patchwork.kernel.org/patch/7344011/#15495901
> > 
> > Ah, I had already forgotten about that discussion.
> > 
> > At least the atomic_long part we discussed there has been resolved now
> > as part of commit b5d47ef9ea5c ("locking/atomics: Switch to generated
> > atomic-long").
> > 
> > Adding Mark Rutland to Cc, maybe he has some ideas of how to use
> > the infrastructure he added to use consistent types for atomic64()
> > on the remaining 64-bit architectures.
> 
> A quick count shows there's only 5 definitions of atomic64_t in the
> tree, it would be trivial to align them on type.
> 
> $ git grep "} atomic64_t"
> arch/arc/include/asm/atomic.h:} atomic64_t;
> arch/arm/include/asm/atomic.h:} atomic64_t;
> arch/x86/include/asm/atomic64_32.h:} atomic64_t;
> include/asm-generic/atomic64.h:} atomic64_t;
> include/linux/types.h:} atomic64_t;
> 
> Note that the one used in _most_ cases, is the one from linux/types.h,
> and that is using 'long'. The others, all typically on ILP32 platforms,
> obviously must use long long.
> 
> I have no objection to changing the types.h one to long long or all of
> them to s64. It really shouldn't matter at all.

I think that using s64 consistently (with any necessary alignment
annotation) makes the most sense. That's unambigious, and what the
common headers now use.

Now that the scripted atomics are merged, I'd like to move arches over
to arch_atomic_*(), so the argument and return types will become s64
everywhere.

Thanks,
Mark.

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Wed, Mar 13, 2019 at 3:36 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Mar 13, 2019 at 10:18:44AM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 11, 2019 at 03:20:04PM +0100, Arnd Bergmann wrote:
> > > On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
>
> I think that using s64 consistently (with any necessary alignment
> annotation) makes the most sense. That's unambigious, and what the
> common headers now use.
>
> Now that the scripted atomics are merged, I'd like to move arches over
> to arch_atomic_*(), so the argument and return types will become s64
> everywhere.

Yes, that sounds like the easiest way, especially if we don't touch the
internal implementation but simply rename all the symbols provided
by the architectures. Is that what you had in mind, or would you go
beyond the minimum changes here?

        Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Arnd Bergmann]

On Wed, Mar 13, 2019 at 3:36 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Mar 13, 2019 at 10:18:44AM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 11, 2019 at 03:20:04PM +0100, Arnd Bergmann wrote:
> > > On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
>
> I think that using s64 consistently (with any necessary alignment
> annotation) makes the most sense. That's unambigious, and what the
> common headers now use.
>
> Now that the scripted atomics are merged, I'd like to move arches over
> to arch_atomic_*(), so the argument and return types will become s64
> everywhere.

Yes, that sounds like the easiest way, especially if we don't touch the
internal implementation but simply rename all the symbols provided
by the architectures. Is that what you had in mind, or would you go
beyond the minimum changes here?

        Arnd

Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()

[Mark Rutland]

On Wed, Mar 13, 2019 at 03:57:42PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 13, 2019 at 3:36 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Mar 13, 2019 at 10:18:44AM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 11, 2019 at 03:20:04PM +0100, Arnd Bergmann wrote:
> > > > On Mon, Mar 11, 2019 at 3:00 PM Qian Cai <cai@lca.pw> wrote:
> >
> > I think that using s64 consistently (with any necessary alignment
> > annotation) makes the most sense. That's unambigious, and what the
> > common headers now use.
> >
> > Now that the scripted atomics are merged, I'd like to move arches over
> > to arch_atomic_*(), so the argument and return types will become s64
> > everywhere.
> 
> Yes, that sounds like the easiest way, especially if we don't touch the
> internal implementation but simply rename all the symbols provided
> by the architectures. Is that what you had in mind, or would you go
> beyond the minimum changes here?

I'd expected to convert arches one-by-one, updating the types during
conversion. I guess it's not strictly necessary to change the internal
types, but it would seem nicer to do that.

I don't think it's possible to do that rename right now, unless we do it
treewide. There are a few core things that need to be fixed up first,
e.g. making <asm-generic/atomic{,64}.h> play nicely with
<asm-generic/atomic-instrumented.h>.

In the end, what I'd like to get to is:

* Arch code provids arch_atomic*_*().

* Common code fleshes out the entire API as raw_atomic*_*(), build atop
  of arch_atomic*_*(). All the ifdeffery lives here.

* Common code builds the instrumented atomic*_*() API atop of the
  raw_atomic*_*() API. No ifdeffery necessary here.

Thanks,
Mark.