| Message ID | 20190310183051.87303-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/debug: add a cast to u64 for atomic64_read() | expand |
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>
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
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
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
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
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.
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
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.
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.
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.
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
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.
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,
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(-)