All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/debug: add a cast to u64 for atomic64_read()
@ 2019-03-10 18:30 Qian Cai
  2019-03-11  3:58 ` Davidlohr Bueso
  0 siblings, 1 reply; 18+ messages in thread
From: Qian Cai @ 2019-03-10 18:30 UTC (permalink / raw)
  To: akpm; +Cc: dave, jgg, arnd, linux-mm, linux-kernel, 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)


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-10 18:30 [PATCH] mm/debug: add a cast to u64 for atomic64_read() Qian Cai
@ 2019-03-11  3:58 ` Davidlohr Bueso
  2019-03-11 12:21   ` Jason Gunthorpe
  0 siblings, 1 reply; 18+ messages in thread
From: Davidlohr Bueso @ 2019-03-11  3:58 UTC (permalink / raw)
  To: Qian Cai; +Cc: akpm, jgg, arnd, linux-mm, linux-kernel

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>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-11  3:58 ` Davidlohr Bueso
@ 2019-03-11 12:21   ` Jason Gunthorpe
  2019-03-11 14:00       ` Qian Cai
  2019-03-11 14:00       ` Arnd Bergmann
  0 siblings, 2 replies; 18+ messages in thread
From: Jason Gunthorpe @ 2019-03-11 12:21 UTC (permalink / raw)
  To: Qian Cai, akpm, arnd, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-11 12:21   ` Jason Gunthorpe
@ 2019-03-11 14:00       ` Qian Cai
  2019-03-11 14:00       ` Arnd Bergmann
  1 sibling, 0 replies; 18+ messages in thread
From: Qian Cai @ 2019-03-11 14:00 UTC (permalink / raw)
  To: Jason Gunthorpe, akpm, arnd, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
@ 2019-03-11 14:00       ` Qian Cai
  0 siblings, 0 replies; 18+ messages in thread
From: Qian Cai @ 2019-03-11 14:00 UTC (permalink / raw)
  To: Jason Gunthorpe, akpm, arnd, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-11 12:21   ` Jason Gunthorpe
@ 2019-03-11 14:00       ` Arnd Bergmann
  2019-03-11 14:00       ` Arnd Bergmann
  1 sibling, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-11 14:00 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Qian Cai, akpm, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
@ 2019-03-11 14:00       ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-11 14:00 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Qian Cai, akpm, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-11 14:00       ` Qian Cai
@ 2019-03-11 14:20         ` Arnd Bergmann
  -1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-11 14:20 UTC (permalink / raw)
  To: Qian Cai; +Cc: Jason Gunthorpe, akpm, linux-mm, linux-kernel, Mark Rutland

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
@ 2019-03-11 14:20         ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-11 14:20 UTC (permalink / raw)
  To: Qian Cai; +Cc: Jason Gunthorpe, akpm, linux-mm, linux-kernel, Mark Rutland

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-11 14:20         ` Arnd Bergmann
  (?)
@ 2019-03-13  9:18         ` Peter Zijlstra
  2019-03-13 13:46             ` Arnd Bergmann
  2019-03-13 14:35           ` Mark Rutland
  -1 siblings, 2 replies; 18+ messages in thread
From: Peter Zijlstra @ 2019-03-13  9:18 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel, Mark Rutland

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-13  9:18         ` Peter Zijlstra
@ 2019-03-13 13:46             ` Arnd Bergmann
  2019-03-13 14:35           ` Mark Rutland
  1 sibling, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-13 13:46 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel, Mark Rutland

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
@ 2019-03-13 13:46             ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-13 13:46 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel, Mark Rutland

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-13 13:46             ` Arnd Bergmann
  (?)
@ 2019-03-13 14:21             ` Peter Zijlstra
  -1 siblings, 0 replies; 18+ messages in thread
From: Peter Zijlstra @ 2019-03-13 14:21 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel, Mark Rutland

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-13 13:46             ` Arnd Bergmann
  (?)
  (?)
@ 2019-03-13 14:24             ` Peter Zijlstra
  -1 siblings, 0 replies; 18+ messages in thread
From: Peter Zijlstra @ 2019-03-13 14:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel, Mark Rutland

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-13  9:18         ` Peter Zijlstra
  2019-03-13 13:46             ` Arnd Bergmann
@ 2019-03-13 14:35           ` Mark Rutland
  2019-03-13 14:57               ` Arnd Bergmann
  1 sibling, 1 reply; 18+ messages in thread
From: Mark Rutland @ 2019-03-13 14:35 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Arnd Bergmann, Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-13 14:35           ` Mark Rutland
@ 2019-03-13 14:57               ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-13 14:57 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Peter Zijlstra, Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
@ 2019-03-13 14:57               ` Arnd Bergmann
  0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2019-03-13 14:57 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Peter Zijlstra, Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] mm/debug: add a cast to u64 for atomic64_read()
  2019-03-13 14:57               ` Arnd Bergmann
  (?)
@ 2019-03-13 16:29               ` Mark Rutland
  -1 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2019-03-13 16:29 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Peter Zijlstra, Qian Cai, Jason Gunthorpe, akpm, linux-mm, linux-kernel

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-03-13 16:29 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-10 18:30 [PATCH] mm/debug: add a cast to u64 for atomic64_read() Qian Cai
2019-03-11  3:58 ` Davidlohr Bueso
2019-03-11 12:21   ` Jason Gunthorpe
2019-03-11 14:00     ` Qian Cai
2019-03-11 14:00       ` Qian Cai
2019-03-11 14:20       ` Arnd Bergmann
2019-03-11 14:20         ` Arnd Bergmann
2019-03-13  9:18         ` Peter Zijlstra
2019-03-13 13:46           ` Arnd Bergmann
2019-03-13 13:46             ` Arnd Bergmann
2019-03-13 14:21             ` Peter Zijlstra
2019-03-13 14:24             ` Peter Zijlstra
2019-03-13 14:35           ` Mark Rutland
2019-03-13 14:57             ` Arnd Bergmann
2019-03-13 14:57               ` Arnd Bergmann
2019-03-13 16:29               ` Mark Rutland
2019-03-11 14:00     ` Arnd Bergmann
2019-03-11 14:00       ` Arnd Bergmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.