Re: next build: 235 warnings 3 failures (next/next-20151117)

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Arnd Bergmann]

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)

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Will Deacon]

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

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Eric Dumazet]

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);
}

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Arnd Bergmann]

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

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Will Deacon]

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

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Arnd Bergmann]

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

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Will Deacon]

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

RE: next build: 235 warnings 3 failures (next/next-20151117)

[David Laight]

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

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Will Deacon]

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

RE: next build: 235 warnings 3 failures (next/next-20151117)

[David Laight]

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

Re: next build: 235 warnings 3 failures (next/next-20151117)

[Will Deacon]

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

RE: next build: 235 warnings 3 failures (next/next-20151117)

[David Laight]

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

RE: next build: 235 warnings 3 failures (next/next-20151117)

[David Laight]

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