Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1270973 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: next build: 235 warnings 3 failures (next/next-20151117) Arnd Bergmann <arnd@arndb.de> - 2015-11-17 10:00 +0100
    Re: next build: 235 warnings 3 failures (next/next-20151117) Will Deacon <will.deacon@arm.com> - 2015-11-17 17:50 +0100
      Re: next build: 235 warnings 3 failures (next/next-20151117) Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-17 18:10 +0100
      Re: next build: 235 warnings 3 failures (next/next-20151117) Arnd Bergmann <arnd@arndb.de> - 2015-11-17 18:10 +0100
        Re: next build: 235 warnings 3 failures (next/next-20151117) Will Deacon <will.deacon@arm.com> - 2015-11-17 18:20 +0100
          Re: next build: 235 warnings 3 failures (next/next-20151117) Arnd Bergmann <arnd@arndb.de> - 2015-11-17 20:20 +0100
            Re: next build: 235 warnings 3 failures (next/next-20151117) Will Deacon <will.deacon@arm.com> - 2015-11-18 11:20 +0100
              RE: next build: 235 warnings 3 failures (next/next-20151117) David Laight <David.Laight@ACULAB.COM> - 2015-11-18 13:20 +0100
                Re: next build: 235 warnings 3 failures (next/next-20151117) Will Deacon <will.deacon@arm.com> - 2015-11-18 13:30 +0100
                  RE: next build: 235 warnings 3 failures (next/next-20151117) David Laight <David.Laight@ACULAB.COM> - 2015-11-18 16:30 +0100
                    Re: next build: 235 warnings 3 failures (next/next-20151117) Will Deacon <will.deacon@arm.com> - 2015-11-18 16:40 +0100
                      RE: next build: 235 warnings 3 failures (next/next-20151117) David Laight <David.Laight@ACULAB.COM> - 2015-11-18 17:00 +0100
            RE: next build: 235 warnings 3 failures (next/next-20151117) David Laight <David.Laight@ACULAB.COM> - 2015-11-18 11:30 +0100

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

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)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1271408

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271456

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271463

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271468

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271584

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272075

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272113

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272123

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272294

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272305

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272319

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272084

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web