* Re: next build: 235 warnings 3 failures (next/next-20151117) [not found] <564a9961.878b420a.331b8.fffffd62@mx.google.com> @ 2015-11-17 8:57 ` Arnd Bergmann 2015-11-17 16:44 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2015-11-17 8:57 UTC (permalink / raw) To: Catalin Marinas, will.deacon Cc: kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel On Monday 16 November 2015 19:05:05 Olof's autobuilder wrote: > > Errors: > > arm64.allmodconfig: > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:75:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:79:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:83:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:75:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:79:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:83:3: error: read-only variable '___p1' used as 'asm' output > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' used as 'asm' output The patch below seems to fix it. Please review/apply. 8<---- Subject: ARM64: make smp_load_acquire() work with const arguments smp_load_acquire() uses typeof() to declare a local variable for temporarily storing the output of the memory access. This fails when the argument is constant, because the assembler complains about using a constant register as output: arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' used as 'asm' output This changes the implementation to use an 'unsigned long' for the temporary value and only cast it to the original type in the end. Signed-off-by: Arnd Bergmann <arnd@arndb.de> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h index 624f9679f4b0..05fa329467f6 100644 --- a/arch/arm64/include/asm/barrier.h +++ b/arch/arm64/include/asm/barrier.h @@ -64,7 +64,7 @@ do { \ #define smp_load_acquire(p) \ ({ \ - typeof(*p) ___p1; \ + unsigned long ___p1; \ compiletime_assert_atomic_type(*p); \ switch (sizeof(*p)) { \ case 1: \ @@ -84,7 +84,7 @@ do { \ : "=r" (___p1) : "Q" (*p) : "memory"); \ break; \ } \ - ___p1; \ + (typeof(*p))___p1; \ }) #define read_barrier_depends() do { } while(0) ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-17 8:57 ` next build: 235 warnings 3 failures (next/next-20151117) Arnd Bergmann @ 2015-11-17 16:44 ` Will Deacon 2015-11-17 17:01 ` Eric Dumazet 2015-11-17 17:03 ` Arnd Bergmann 0 siblings, 2 replies; 13+ messages in thread From: Will Deacon @ 2015-11-17 16:44 UTC (permalink / raw) To: Arnd Bergmann Cc: Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel Hi Arnd, On Tue, Nov 17, 2015 at 09:57:30AM +0100, Arnd Bergmann wrote: > On Monday 16 November 2015 19:05:05 Olof's autobuilder wrote: > > > > Errors: > > > > arm64.allmodconfig: > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:75:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:79:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:83:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:75:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:79:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:83:3: error: read-only variable '___p1' used as 'asm' output > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' used as 'asm' output > > The patch below seems to fix it. Please review/apply. > > 8<---- > Subject: ARM64: make smp_load_acquire() work with const arguments > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > storing the output of the memory access. This fails when the argument is > constant, because the assembler complains about using a constant register > as output: > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > used as 'asm' output Do you know the usage in the kernel causing this warning? > This changes the implementation to use an 'unsigned long' for the temporary value > and only cast it to the original type in the end. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h > index 624f9679f4b0..05fa329467f6 100644 > --- a/arch/arm64/include/asm/barrier.h > +++ b/arch/arm64/include/asm/barrier.h > @@ -64,7 +64,7 @@ do { \ > > #define smp_load_acquire(p) \ > ({ \ > - typeof(*p) ___p1; \ > + unsigned long ___p1; \ > compiletime_assert_atomic_type(*p); \ > switch (sizeof(*p)) { \ > case 1: \ > @@ -84,7 +84,7 @@ do { \ > : "=r" (___p1) : "Q" (*p) : "memory"); \ > break; \ > } \ > - ___p1; \ > + (typeof(*p))___p1; \ My worry about having the cast here is if somebody decides to smp_load_acquire from a packed 64-bit structure (e.g. some sort of lock), then we'll get a conversion to non-scalar type error from the compiler. Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-17 16:44 ` Will Deacon @ 2015-11-17 17:01 ` Eric Dumazet 2015-11-17 17:03 ` Arnd Bergmann 1 sibling, 0 replies; 13+ messages in thread From: Eric Dumazet @ 2015-11-17 17:01 UTC (permalink / raw) To: Will Deacon Cc: Arnd Bergmann, Catalin Marinas, kernel-build-reports, olof, David Miller, netdev, linux-next, linux-kernel, linux-arm-kernel On Tue, 2015-11-17 at 16:44 +0000, Will Deacon wrote: > Do you know the usage in the kernel causing this warning? /** * sk_state_load - read sk->sk_state for lockless contexts * @sk: socket pointer * * Paired with sk_state_store(). Used in places we do not hold socket lock : * tcp_diag_get_info(), tcp_get_info(), tcp_poll(), get_tcp4_sock() ... */ static inline int sk_state_load(const struct sock *sk) { return smp_load_acquire(&sk->sk_state); } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-17 16:44 ` Will Deacon 2015-11-17 17:01 ` Eric Dumazet @ 2015-11-17 17:03 ` Arnd Bergmann 2015-11-17 17:12 ` Will Deacon 1 sibling, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2015-11-17 17:03 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > 8<---- > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > storing the output of the memory access. This fails when the argument is > > constant, because the assembler complains about using a constant register > > as output: > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > used as 'asm' output > > Do you know the usage in the kernel causing this warning? A newly introduced function in include/net/sock.h: static inline int sk_state_load(const struct sock *sk) { return smp_load_acquire(&sk->sk_state); } Arnd ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-17 17:03 ` Arnd Bergmann @ 2015-11-17 17:12 ` Will Deacon 2015-11-17 19:17 ` Arnd Bergmann 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2015-11-17 17:12 UTC (permalink / raw) To: Arnd Bergmann Cc: Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel On Tue, Nov 17, 2015 at 06:03:40PM +0100, Arnd Bergmann wrote: > On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > > 8<---- > > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > > storing the output of the memory access. This fails when the argument is > > > constant, because the assembler complains about using a constant register > > > as output: > > > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > > used as 'asm' output > > > > Do you know the usage in the kernel causing this warning? > > A newly introduced function in include/net/sock.h: > > static inline int sk_state_load(const struct sock *sk) > { > return smp_load_acquire(&sk->sk_state); > } Hmm, maybe we could play a similar trick to READ_ONCE by declaring an anonymous union and writing through the non-const member? Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-17 17:12 ` Will Deacon @ 2015-11-17 19:17 ` Arnd Bergmann 2015-11-18 10:14 ` Will Deacon 2015-11-18 10:22 ` David Laight 0 siblings, 2 replies; 13+ messages in thread From: Arnd Bergmann @ 2015-11-17 19:17 UTC (permalink / raw) To: Will Deacon Cc: Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel On Tuesday 17 November 2015 17:12:37 Will Deacon wrote: > On Tue, Nov 17, 2015 at 06:03:40PM +0100, Arnd Bergmann wrote: > > On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > > > 8<---- > > > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > > > storing the output of the memory access. This fails when the argument is > > > > constant, because the assembler complains about using a constant register > > > > as output: > > > > > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > > > used as 'asm' output > > > > > > Do you know the usage in the kernel causing this warning? > > > > A newly introduced function in include/net/sock.h: > > > > static inline int sk_state_load(const struct sock *sk) > > { > > return smp_load_acquire(&sk->sk_state); > > } > > Hmm, maybe we could play a similar trick to READ_ONCE by declaring an > anonymous union and writing through the non-const member? Yes, I think that would work, if you think we need to care about the case where we read into a structure. Can you come up with a patch for that? Arnd ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-17 19:17 ` Arnd Bergmann @ 2015-11-18 10:14 ` Will Deacon 2015-11-18 12:11 ` David Laight 2015-11-18 10:22 ` David Laight 1 sibling, 1 reply; 13+ messages in thread From: Will Deacon @ 2015-11-18 10:14 UTC (permalink / raw) To: Arnd Bergmann Cc: Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel On Tue, Nov 17, 2015 at 08:17:17PM +0100, Arnd Bergmann wrote: > On Tuesday 17 November 2015 17:12:37 Will Deacon wrote: > > On Tue, Nov 17, 2015 at 06:03:40PM +0100, Arnd Bergmann wrote: > > > On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > > > > 8<---- > > > > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > > > > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > > > > storing the output of the memory access. This fails when the argument is > > > > > constant, because the assembler complains about using a constant register > > > > > as output: > > > > > > > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > > > > used as 'asm' output > > > > > > > > Do you know the usage in the kernel causing this warning? > > > > > > A newly introduced function in include/net/sock.h: > > > > > > static inline int sk_state_load(const struct sock *sk) > > > { > > > return smp_load_acquire(&sk->sk_state); > > > } > > > > Hmm, maybe we could play a similar trick to READ_ONCE by declaring an > > anonymous union and writing through the non-const member? > > Yes, I think that would work, if you think we need to care about the > case where we read into a structure. > > Can you come up with a patch for that? Done: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386094.html Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-18 10:14 ` Will Deacon @ 2015-11-18 12:11 ` David Laight 2015-11-18 12:28 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: David Laight @ 2015-11-18 12:11 UTC (permalink / raw) To: 'Will Deacon', Arnd Bergmann Cc: Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel From: Will Deacon > Sent: 18 November 2015 10:14 > On Tue, Nov 17, 2015 at 08:17:17PM +0100, Arnd Bergmann wrote: > > On Tuesday 17 November 2015 17:12:37 Will Deacon wrote: > > > On Tue, Nov 17, 2015 at 06:03:40PM +0100, Arnd Bergmann wrote: > > > > On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > > > > > 8<---- > > > > > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > > > > > > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > > > > > storing the output of the memory access. This fails when the argument is > > > > > > constant, because the assembler complains about using a constant register > > > > > > as output: > > > > > > > > > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > > > > > used as 'asm' output > > > > > > > > > > Do you know the usage in the kernel causing this warning? > > > > > > > > A newly introduced function in include/net/sock.h: > > > > > > > > static inline int sk_state_load(const struct sock *sk) > > > > { > > > > return smp_load_acquire(&sk->sk_state); > > > > } > > > > > > Hmm, maybe we could play a similar trick to READ_ONCE by declaring an > > > anonymous union and writing through the non-const member? > > > > Yes, I think that would work, if you think we need to care about the > > case where we read into a structure. > > > > Can you come up with a patch for that? > > Done: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386094.html That patch forces a memory write-read and returns uninitialised stack for short reads. Who knows what happens on big-endian systems. You need a static inline function with separate temporaries in each branch. David ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-18 12:11 ` David Laight @ 2015-11-18 12:28 ` Will Deacon 2015-11-18 15:21 ` David Laight 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2015-11-18 12:28 UTC (permalink / raw) To: David Laight Cc: Arnd Bergmann, Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel On Wed, Nov 18, 2015 at 12:11:25PM +0000, David Laight wrote: > From: Will Deacon > > Sent: 18 November 2015 10:14 > > On Tue, Nov 17, 2015 at 08:17:17PM +0100, Arnd Bergmann wrote: > > > On Tuesday 17 November 2015 17:12:37 Will Deacon wrote: > > > > On Tue, Nov 17, 2015 at 06:03:40PM +0100, Arnd Bergmann wrote: > > > > > On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > > > > > > 8<---- > > > > > > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > > > > > > > > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > > > > > > storing the output of the memory access. This fails when the argument is > > > > > > > constant, because the assembler complains about using a constant register > > > > > > > as output: > > > > > > > > > > > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > > > > > > used as 'asm' output > > > > > > > > > > > > Do you know the usage in the kernel causing this warning? > > > > > > > > > > A newly introduced function in include/net/sock.h: > > > > > > > > > > static inline int sk_state_load(const struct sock *sk) > > > > > { > > > > > return smp_load_acquire(&sk->sk_state); > > > > > } > > > > > > > > Hmm, maybe we could play a similar trick to READ_ONCE by declaring an > > > > anonymous union and writing through the non-const member? > > > > > > Yes, I think that would work, if you think we need to care about the > > > case where we read into a structure. > > > > > > Can you come up with a patch for that? > > > > Done: > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386094.html > > That patch forces a memory write-read and returns uninitialised stack > for short reads. Really? The disassembly looks fine to me. Do you have a concrete example of where you think it goes wrong, please? > Who knows what happens on big-endian systems. The same thing as READ_ONCE? I'll test it there to make sure, but I don't see a problem. > You need a static inline function with separate temporaries in each > branch. I'm just following the precedent set out by READ_ONCE, since that's tackling exactly the same problem and appears to be working for people. Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-18 12:28 ` Will Deacon @ 2015-11-18 15:21 ` David Laight 2015-11-18 15:36 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: David Laight @ 2015-11-18 15:21 UTC (permalink / raw) To: 'Will Deacon' Cc: Arnd Bergmann, Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel From: Will Deacon > Sent: 18 November 2015 12:28 > On Wed, Nov 18, 2015 at 12:11:25PM +0000, David Laight wrote: > > From: Will Deacon > > > Sent: 18 November 2015 10:14 > > > On Tue, Nov 17, 2015 at 08:17:17PM +0100, Arnd Bergmann wrote: > > > > On Tuesday 17 November 2015 17:12:37 Will Deacon wrote: > > > > > On Tue, Nov 17, 2015 at 06:03:40PM +0100, Arnd Bergmann wrote: > > > > > > On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > > > > > > > 8<---- > > > > > > > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > > > > > > > > > > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > > > > > > > storing the output of the memory access. This fails when the argument is > > > > > > > > constant, because the assembler complains about using a constant register > > > > > > > > as output: > > > > > > > > > > > > > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > > > > > > > used as 'asm' output > > > > > > > > > > > > > > Do you know the usage in the kernel causing this warning? > > > > > > > > > > > > A newly introduced function in include/net/sock.h: > > > > > > > > > > > > static inline int sk_state_load(const struct sock *sk) > > > > > > { > > > > > > return smp_load_acquire(&sk->sk_state); > > > > > > } > > > > > > > > > > Hmm, maybe we could play a similar trick to READ_ONCE by declaring an > > > > > anonymous union and writing through the non-const member? > > > > > > > > Yes, I think that would work, if you think we need to care about the > > > > case where we read into a structure. > > > > > > > > Can you come up with a patch for that? > > > > > > Done: > > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386094.html > > > > That patch forces a memory write-read and returns uninitialised stack > > for short reads. > > Really? The disassembly looks fine to me. Do you have a concrete example > of where you think it goes wrong, please? > > > Who knows what happens on big-endian systems. > > The same thing as READ_ONCE? I'll test it there to make sure, but I > don't see a problem. Ah, god, it is absolutely horrid. But probably right :-( Do all the lda variants zero extend to 64 bits ? If so maybe you could use a single 64 bit variable for the result of the read and then cast it to typeof(*p) to get the required sign extension for small integer types. David ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-18 15:21 ` David Laight @ 2015-11-18 15:36 ` Will Deacon 2015-11-18 15:57 ` David Laight 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2015-11-18 15:36 UTC (permalink / raw) To: David Laight Cc: Arnd Bergmann, Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel On Wed, Nov 18, 2015 at 03:21:19PM +0000, David Laight wrote: > From: Will Deacon > > Sent: 18 November 2015 12:28 > > On Wed, Nov 18, 2015 at 12:11:25PM +0000, David Laight wrote: > > > From: Will Deacon > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386094.html > > > > > > That patch forces a memory write-read and returns uninitialised stack > > > for short reads. > > > > Really? The disassembly looks fine to me. Do you have a concrete example > > of where you think it goes wrong, please? > > > > > Who knows what happens on big-endian systems. > > > > The same thing as READ_ONCE? I'll test it there to make sure, but I > > don't see a problem. > > Ah, god, it is absolutely horrid. But probably right :-( Yeah, I wasn't pretending it was nice :) FWIW, I've given it a reasonable testing in both little-endian and big-endian configurations and it seems to be happy. > Do all the lda variants zero extend to 64 bits ? Yes. > If so maybe you could use a single 64 bit variable for the result of the read > and then cast it to typeof(*p) to get the required sign extension for > small integer types. That was the original proposal from Arnd, but I want this to work with structures smaller than 64-bit (e.g. arch_spinlock_t), so that's why I decided to follow the approach laid down by READ_ONCE. Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-18 15:36 ` Will Deacon @ 2015-11-18 15:57 ` David Laight 0 siblings, 0 replies; 13+ messages in thread From: David Laight @ 2015-11-18 15:57 UTC (permalink / raw) To: 'Will Deacon' Cc: Arnd Bergmann, Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel From: Will Deacon [mailto:will.deacon@arm.com] > Sent: 18 November 2015 15:37 > On Wed, Nov 18, 2015 at 03:21:19PM +0000, David Laight wrote: > > From: Will Deacon > > > Sent: 18 November 2015 12:28 > > > On Wed, Nov 18, 2015 at 12:11:25PM +0000, David Laight wrote: > > > > From: Will Deacon > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386094.html > > > > > > > > That patch forces a memory write-read and returns uninitialised stack > > > > for short reads. > > > > > > Really? The disassembly looks fine to me. Do you have a concrete example > > > of where you think it goes wrong, please? > > > > > > > Who knows what happens on big-endian systems. > > > > > > The same thing as READ_ONCE? I'll test it there to make sure, but I > > > don't see a problem. > > > > Ah, god, it is absolutely horrid. But probably right :-( > > Yeah, I wasn't pretending it was nice :) FWIW, I've given it a reasonable > testing in both little-endian and big-endian configurations and it seems > to be happy. I was missing the fact that the *(int_type *)&union is always reading the full union. The next version of the compiler might decide to barf at the code that appears to be reading beyond the end of the union. > > Do all the lda variants zero extend to 64 bits ? > > Yes. > > > If so maybe you could use a single 64 bit variable for the result of the read > > and then cast it to typeof(*p) to get the required sign extension for > > small integer types. > > That was the original proposal from Arnd, but I want this to work with > structures smaller than 64-bit (e.g. arch_spinlock_t), so that's why > I decided to follow the approach laid down by READ_ONCE. That would still be ok. You'd have something that is effectively: _u64 val = *p; return typeof(*p)val; The compiler might mask unsigned values - but it may be able to determine it isn't needed (which is probably true of your version). For signed types both versions require the compile sign-extend the value. David ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: next build: 235 warnings 3 failures (next/next-20151117) 2015-11-17 19:17 ` Arnd Bergmann 2015-11-18 10:14 ` Will Deacon @ 2015-11-18 10:22 ` David Laight 1 sibling, 0 replies; 13+ messages in thread From: David Laight @ 2015-11-18 10:22 UTC (permalink / raw) To: 'Arnd Bergmann', Will Deacon Cc: Catalin Marinas, kernel-build-reports, olof, David Miller, eric.dumazet, netdev, linux-next, linux-kernel, linux-arm-kernel From: Arnd Bergmann > Sent: 17 November 2015 19:17 > On Tuesday 17 November 2015 17:12:37 Will Deacon wrote: > > On Tue, Nov 17, 2015 at 06:03:40PM +0100, Arnd Bergmann wrote: > > > On Tuesday 17 November 2015 16:44:53 Will Deacon wrote: > > > > > 8<---- > > > > > Subject: ARM64: make smp_load_acquire() work with const arguments > > > > > > > > > > smp_load_acquire() uses typeof() to declare a local variable for temporarily > > > > > storing the output of the memory access. This fails when the argument is > > > > > constant, because the assembler complains about using a constant register > > > > > as output: > > > > > > > > > > arch/arm64/include/asm/barrier.h:71:3: error: read-only variable '___p1' > > > > > used as 'asm' output > > > > > > > > Do you know the usage in the kernel causing this warning? > > > > > > A newly introduced function in include/net/sock.h: > > > > > > static inline int sk_state_load(const struct sock *sk) > > > { > > > return smp_load_acquire(&sk->sk_state); > > > } > > > > Hmm, maybe we could play a similar trick to READ_ONCE by declaring an > > anonymous union and writing through the non-const member? > > Yes, I think that would work, if you think we need to care about the > case where we read into a structure. How are you going to find the right type/size for the non-const member of the union? If the type/size is fixed then why use typeof()? If the variable being read (sk->sk_state above) might only be 8 or 16 bits then you might be able to use sizeof() to select between code paths. David ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-11-18 15:57 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <564a9961.878b420a.331b8.fffffd62@mx.google.com> 2015-11-17 8:57 ` next build: 235 warnings 3 failures (next/next-20151117) Arnd Bergmann 2015-11-17 16:44 ` Will Deacon 2015-11-17 17:01 ` Eric Dumazet 2015-11-17 17:03 ` Arnd Bergmann 2015-11-17 17:12 ` Will Deacon 2015-11-17 19:17 ` Arnd Bergmann 2015-11-18 10:14 ` Will Deacon 2015-11-18 12:11 ` David Laight 2015-11-18 12:28 ` Will Deacon 2015-11-18 15:21 ` David Laight 2015-11-18 15:36 ` Will Deacon 2015-11-18 15:57 ` David Laight 2015-11-18 10:22 ` David Laight
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).