* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 12:18 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022104805.GA1503673@kroah.com>
On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
> On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> > On 22.10.20 11:32, David Laight wrote:
> > > From: David Hildenbrand
> > >> Sent: 22 October 2020 10:25
> > > ...
> > >> ... especially because I recall that clang and gcc behave slightly
> > >> differently:
> > >>
> > >> https://github.com/hjl-tools/x86-psABI/issues/2
> > >>
> > >> "Function args are different: narrow types are sign or zero extended to
> > >> 32 bits, depending on their type. clang depends on this for incoming
> > >> args, but gcc doesn't make that assumption. But both compilers do it
> > >> when calling, so gcc code can call clang code.
> > >
> > > It really is best to use 'int' (or even 'long') for all numeric
> > > arguments (and results) regardless of the domain of the value.
> > >
> > > Related, I've always worried about 'bool'....
> > >
> > >> The upper 32 bits of registers are always undefined garbage for types
> > >> smaller than 64 bits."
> > >
> > > On x86-64 the high bits are zeroed by all 32bit loads.
> >
> > Yeah, but does not help here.
> >
> >
> > My thinking: if the compiler that calls import_iovec() has garbage in
> > the upper 32 bit
> >
> > a) gcc will zero it out and not rely on it being zero.
> > b) clang will not zero it out, assuming it is zero.
> >
> > But
> >
> > a) will zero it out when calling the !inlined variant
> > b) clang will zero it out when calling the !inlined variant
> >
> > When inlining, b) strikes. We access garbage. That would mean that we
> > have calling code that's not generated by clang/gcc IIUC.
> >
> > We can test easily by changing the parameters instead of adding an "inline".
>
> Let me try that as well, as I seem to have a good reproducer, but it
> takes a while to run...
Ok, that didn't work.
And I can't seem to "fix" this by adding noinline to patches further
along in the patch series (because this commit's function is no longer
present due to later patches.)
Will keep digging...
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Introduce POWER10_DD1 feature
From: Michael Ellerman @ 2020-10-22 11:35 UTC (permalink / raw)
To: Ravi Bangoria
Cc: christophe.leroy, ravi.bangoria, mikey, jniethe5, npiggin, maddy,
paulus, naveen.n.rao, linuxppc-dev
In-Reply-To: <20201020054454.194343-1-ravi.bangoria@linux.ibm.com>
Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> POWER10_DD1 feature flag will be needed while adding
> conditional code that applies only for Power10 DD1.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
> arch/powerpc/include/asm/cputable.h | 8 ++++++--
> arch/powerpc/kernel/dt_cpu_ftrs.c | 3 +++
> arch/powerpc/kernel/prom.c | 9 +++++++++
> 3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index 93bc70d4c9a1..d486f56c0d33 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -216,6 +216,7 @@ static inline void cpu_feature_keys_init(void) { }
> #define CPU_FTR_P9_RADIX_PREFETCH_BUG LONG_ASM_CONST(0x0002000000000000)
> #define CPU_FTR_ARCH_31 LONG_ASM_CONST(0x0004000000000000)
> #define CPU_FTR_DAWR1 LONG_ASM_CONST(0x0008000000000000)
> +#define CPU_FTR_POWER10_DD1 LONG_ASM_CONST(0x0010000000000000)
>
> #ifndef __ASSEMBLY__
>
> @@ -479,6 +480,7 @@ static inline void cpu_feature_keys_init(void) { }
> CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
> CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31 | \
> CPU_FTR_DAWR | CPU_FTR_DAWR1)
> +#define CPU_FTRS_POWER10_DD1 (CPU_FTRS_POWER10 | CPU_FTR_POWER10_DD1)
> #define CPU_FTRS_CELL (CPU_FTR_LWSYNC | \
> CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
> CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
> @@ -497,14 +499,16 @@ static inline void cpu_feature_keys_init(void) { }
> #define CPU_FTRS_POSSIBLE \
> (CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | CPU_FTRS_POWER8 | \
> CPU_FTR_ALTIVEC_COMP | CPU_FTR_VSX_COMP | CPU_FTRS_POWER9 | \
> - CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
> + CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
> + CPU_FTRS_POWER10_DD1)
> #else
> #define CPU_FTRS_POSSIBLE \
> (CPU_FTRS_PPC970 | CPU_FTRS_POWER5 | \
> CPU_FTRS_POWER6 | CPU_FTRS_POWER7 | CPU_FTRS_POWER8E | \
> CPU_FTRS_POWER8 | CPU_FTRS_CELL | CPU_FTRS_PA6T | \
> CPU_FTR_VSX_COMP | CPU_FTR_ALTIVEC_COMP | CPU_FTRS_POWER9 | \
> - CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10)
> + CPU_FTRS_POWER9_DD2_1 | CPU_FTRS_POWER9_DD2_2 | CPU_FTRS_POWER10 | \
> + CPU_FTRS_POWER10_DD1)
> #endif /* CONFIG_CPU_LITTLE_ENDIAN */
> #endif
> #else
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 1098863e17ee..b2327f2967ff 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -811,6 +811,9 @@ static __init void cpufeatures_cpu_quirks(void)
> }
>
> update_tlbie_feature_flag(version);
> +
> + if ((version & 0xffffffff) == 0x00800100)
> + cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
> }
>
> static void __init cpufeatures_setup_finished(void)
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index c1545f22c077..c778c81284f7 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -305,6 +305,14 @@ static void __init check_cpu_feature_properties(unsigned long node)
> }
> }
>
> +static void __init fixup_cpu_features(void)
> +{
> + unsigned long version = mfspr(SPRN_PVR);
> +
> + if ((version & 0xffffffff) == 0x00800100)
> + cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
> +}
> +
> static int __init early_init_dt_scan_cpus(unsigned long node,
> const char *uname, int depth,
> void *data)
> @@ -378,6 +386,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
>
> check_cpu_feature_properties(node);
> check_cpu_pa_features(node);
> + fixup_cpu_features();
> }
This is not the way we normally do CPU features.
In the past we have always added a raw entry in cputable.c, see eg. the
Power9 DD 2.0, 2.1 entries.
Doing it here is not really safe, if you're running with an architected
PVR (via cpu-version property), you can't set the DD1 feature, because
you might be migrated to a future CPU that doesn't have the DD1 quirks.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Send SIGBUS from machine_check
From: Joakim Tjernlund @ 2020-10-22 11:19 UTC (permalink / raw)
To: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201001170557.10915-1-joakim.tjernlund@infinera.com>
ping
Also Cc: stable@vger.kernel.org
On Thu, 2020-10-01 at 19:05 +0200, Joakim Tjernlund wrote:
> Embedded PPC CPU should send SIGBUS to user space when applicable.
>
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
> arch/powerpc/kernel/traps.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 0381242920d9..12715d24141c 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -621,6 +621,11 @@ int machine_check_e500mc(struct pt_regs *regs)
> reason & MCSR_MEA ? "Effective" : "Physical", addr);
> }
>
>
> + if ((user_mode(regs))) {
> + _exception(SIGBUS, regs, reason, regs->nip);
> + recoverable = 1;
> + }
> +
> silent_out:
> mtspr(SPRN_MCSR, mcsr);
> return mfspr(SPRN_MCSR) == 0 && recoverable;
> @@ -665,6 +670,10 @@ int machine_check_e500(struct pt_regs *regs)
> if (reason & MCSR_BUS_RPERR)
> printk("Bus - Read Parity Error\n");
>
>
> + if ((user_mode(regs))) {
> + _exception(SIGBUS, regs, reason, regs->nip);
> + return 1;
> + }
> return 0;
> }
>
>
> @@ -695,6 +704,10 @@ int machine_check_e200(struct pt_regs *regs)
> if (reason & MCSR_BUS_WRERR)
> printk("Bus - Write Bus Error on buffered store or cache line push\n");
>
>
> + if ((user_mode(regs))) {
> + _exception(SIGBUS, regs, reason, regs->nip);
> + return 1;
> + }
> return 0;
> }
> #elif defined(CONFIG_PPC32)
> @@ -731,6 +744,10 @@ int machine_check_generic(struct pt_regs *regs)
> default:
> printk("Unknown values in msr\n");
> }
> + if ((user_mode(regs))) {
> + _exception(SIGBUS, regs, reason, regs->nip);
> + return 1;
> + }
> return 0;
> }
> #endif /* everything else */
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 10:48 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <5fd6003b-55a6-2c3c-9a28-8fd3a575ca78@redhat.com>
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> On 22.10.20 11:32, David Laight wrote:
> > From: David Hildenbrand
> >> Sent: 22 October 2020 10:25
> > ...
> >> ... especially because I recall that clang and gcc behave slightly
> >> differently:
> >>
> >> https://github.com/hjl-tools/x86-psABI/issues/2
> >>
> >> "Function args are different: narrow types are sign or zero extended to
> >> 32 bits, depending on their type. clang depends on this for incoming
> >> args, but gcc doesn't make that assumption. But both compilers do it
> >> when calling, so gcc code can call clang code.
> >
> > It really is best to use 'int' (or even 'long') for all numeric
> > arguments (and results) regardless of the domain of the value.
> >
> > Related, I've always worried about 'bool'....
> >
> >> The upper 32 bits of registers are always undefined garbage for types
> >> smaller than 64 bits."
> >
> > On x86-64 the high bits are zeroed by all 32bit loads.
>
> Yeah, but does not help here.
>
>
> My thinking: if the compiler that calls import_iovec() has garbage in
> the upper 32 bit
>
> a) gcc will zero it out and not rely on it being zero.
> b) clang will not zero it out, assuming it is zero.
>
> But
>
> a) will zero it out when calling the !inlined variant
> b) clang will zero it out when calling the !inlined variant
>
> When inlining, b) strikes. We access garbage. That would mean that we
> have calling code that's not generated by clang/gcc IIUC.
>
> We can test easily by changing the parameters instead of adding an "inline".
Let me try that as well, as I seem to have a good reproducer, but it
takes a while to run...
greg k-h
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-22 9:36 UTC (permalink / raw)
To: David Laight, Greg KH
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <bc0a091865f34700b9df332c6e9dcdfd@AcuMS.aculab.com>
On 22.10.20 11:32, David Laight wrote:
> From: David Hildenbrand
>> Sent: 22 October 2020 10:25
> ...
>> ... especially because I recall that clang and gcc behave slightly
>> differently:
>>
>> https://github.com/hjl-tools/x86-psABI/issues/2
>>
>> "Function args are different: narrow types are sign or zero extended to
>> 32 bits, depending on their type. clang depends on this for incoming
>> args, but gcc doesn't make that assumption. But both compilers do it
>> when calling, so gcc code can call clang code.
>
> It really is best to use 'int' (or even 'long') for all numeric
> arguments (and results) regardless of the domain of the value.
>
> Related, I've always worried about 'bool'....
>
>> The upper 32 bits of registers are always undefined garbage for types
>> smaller than 64 bits."
>
> On x86-64 the high bits are zeroed by all 32bit loads.
Yeah, but does not help here.
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
We can test easily by changing the parameters instead of adding an "inline".
--
Thanks,
David / dhildenb
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 9:32 UTC (permalink / raw)
To: 'David Hildenbrand', Greg KH
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <a1533569-948a-1d5b-e231-5531aa988047@redhat.com>
From: David Hildenbrand
> Sent: 22 October 2020 10:25
...
> ... especially because I recall that clang and gcc behave slightly
> differently:
>
> https://github.com/hjl-tools/x86-psABI/issues/2
>
> "Function args are different: narrow types are sign or zero extended to
> 32 bits, depending on their type. clang depends on this for incoming
> args, but gcc doesn't make that assumption. But both compilers do it
> when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
> The upper 32 bits of registers are always undefined garbage for types
> smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Arnd Bergmann @ 2020-10-22 9:16 UTC (permalink / raw)
To: Greg KH
Cc: linux-aio, open list:BROADCOM NVRAM DRIVER, David Howells,
Linux-MM, keyrings, sparclinux, Christoph Hellwig, linux-arch,
linux-s390, linux-scsi, Android Kernel Team, linux-block, Al Viro,
io-uring, Linux ARM, Jens Axboe, Parisc List, Networking,
Nick Desaulniers, linux-kernel@vger.kernel.org, LSM List,
David Laight, Linux FS-devel Mailing List, Andrew Morton,
linuxppc-dev
In-Reply-To: <20201022082654.GA1477657@kroah.com>
On Thu, Oct 22, 2020 at 10:26 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
> > On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
> > > On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
> > >
> > > I can't really figure out what the regression is, only that this commit
> > > triggers a "large Android system binary" from working properly. There's
> > > no kernel log messages anywhere, and I don't have any way to strace the
> > > thing in the testing framework, so any hints that people can provide
> > > would be most appreciated.
> >
> > It's a pure move - modulo changed line breaks in the argument lists
> > the functions involved are identical before and after that (just checked
> > that directly, by checking out the trees before and after, extracting two
> > functions in question from fs/read_write.c and lib/iov_iter.c (before and
> > after, resp.) and checking the diff between those.
> >
> > How certain is your bisection?
>
> The bisection is very reproducable.
>
> But, this looks now to be a compiler bug. I'm using the latest version
> of clang and if I put "noinline" at the front of the function,
> everything works.
>
> Nick, any ideas here as to who I should report this to?
>
> I'll work on a fixup patch for the Android kernel tree to see if I can
> work around it there, but others will hit this in Linus's tree sooner or
> later...
I see that Christoph rewrote the function again in bfdc59701d6d
("iov_iter: refactor rw_copy_check_uvector and import_iovec"),
do you know if the current mainline version is also affected?
Do you know if it happens across multiple architectures or might
be specific to either x86 or arm64?
https://bugs.llvm.org/ is obviously the place for reporting the
issue if it does turn out to be a bug in clang, but that requires
a specific thing going wrong in the output.
One idea I have for debugging it is to sprinkle the inlined
function with lots of barrier()s to prevent a lot of the optimizations.
If that solves the issue, you can bisect through those until you
find one barrier that makes the difference between working and
broken and then look at diff of the assembler output.
Arnd
^ permalink raw reply
* [PATCH v3 3/3] powerpc: Fix update form addressing in inline assembly
From: Christophe Leroy @ 2020-10-22 9:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
mathieu.desnoyers
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <25e6fca46fda3e2a4298448edbf654f64756eee7.1603358942.git.christophe.leroy@csgroup.eu>
In several places, inline assembly uses the "%Un" modifier
to enable the use of instruction with update form addressing,
but the associated "<>" constraint is missing.
As mentioned in previous patch, this fails with gcc 4.9, so
"<>" can't be used directly.
Use UPD_CONSTR macro everywhere %Un modifier is used.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
---
v3: __set_pte_at() not impacted anymore (%U0 removed in previous patch)
v2: Build failure (circular inclusion) fixed by change in patch 1
---
arch/powerpc/include/asm/atomic.h | 9 +++++----
arch/powerpc/include/asm/io.h | 4 ++--
arch/powerpc/kvm/powerpc.c | 4 ++--
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
index 8a55eb8cc97b..61c6e8b200e8 100644
--- a/arch/powerpc/include/asm/atomic.h
+++ b/arch/powerpc/include/asm/atomic.h
@@ -10,6 +10,7 @@
#include <linux/types.h>
#include <asm/cmpxchg.h>
#include <asm/barrier.h>
+#include <asm/asm-const.h>
/*
* Since *_return_relaxed and {cmp}xchg_relaxed are implemented with
@@ -26,14 +27,14 @@ static __inline__ int atomic_read(const atomic_t *v)
{
int t;
- __asm__ __volatile__("lwz%U1%X1 %0,%1" : "=r"(t) : "m"(v->counter));
+ __asm__ __volatile__("lwz%U1%X1 %0,%1" : "=r"(t) : "m"UPD_CONSTR(v->counter));
return t;
}
static __inline__ void atomic_set(atomic_t *v, int i)
{
- __asm__ __volatile__("stw%U0%X0 %1,%0" : "=m"(v->counter) : "r"(i));
+ __asm__ __volatile__("stw%U0%X0 %1,%0" : "=m"UPD_CONSTR(v->counter) : "r"(i));
}
#define ATOMIC_OP(op, asm_op) \
@@ -316,14 +317,14 @@ static __inline__ s64 atomic64_read(const atomic64_t *v)
{
s64 t;
- __asm__ __volatile__("ld%U1%X1 %0,%1" : "=r"(t) : "m"(v->counter));
+ __asm__ __volatile__("ld%U1%X1 %0,%1" : "=r"(t) : "m"UPD_CONSTR(v->counter));
return t;
}
static __inline__ void atomic64_set(atomic64_t *v, s64 i)
{
- __asm__ __volatile__("std%U0%X0 %1,%0" : "=m"(v->counter) : "r"(i));
+ __asm__ __volatile__("std%U0%X0 %1,%0" : "=m"UPD_CONSTR(v->counter) : "r"(i));
}
#define ATOMIC64_OP(op, asm_op) \
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 58635960403c..87964dfb838e 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -122,7 +122,7 @@ static inline u##size name(const volatile u##size __iomem *addr) \
{ \
u##size ret; \
__asm__ __volatile__("sync;"#insn"%U1%X1 %0,%1;twi 0,%0,0;isync"\
- : "=r" (ret) : "m" (*addr) : "memory"); \
+ : "=r" (ret) : "m"UPD_CONSTR (*addr) : "memory"); \
return ret; \
}
@@ -130,7 +130,7 @@ static inline u##size name(const volatile u##size __iomem *addr) \
static inline void name(volatile u##size __iomem *addr, u##size val) \
{ \
__asm__ __volatile__("sync;"#insn"%U0%X0 %1,%0" \
- : "=m" (*addr) : "r" (val) : "memory"); \
+ : "=m"UPD_CONSTR (*addr) : "r" (val) : "memory"); \
mmiowb_set_pending(); \
}
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 13999123b735..cf52d26f49cd 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -1087,7 +1087,7 @@ static inline u64 sp_to_dp(u32 fprs)
preempt_disable();
enable_kernel_fp();
- asm ("lfs%U1%X1 0,%1; stfd%U0%X0 0,%0" : "=m" (fprd) : "m" (fprs)
+ asm ("lfs%U1%X1 0,%1; stfd%U0%X0 0,%0" : "=m"UPD_CONSTR (fprd) : "m"UPD_CONSTR (fprs)
: "fr0");
preempt_enable();
return fprd;
@@ -1099,7 +1099,7 @@ static inline u32 dp_to_sp(u64 fprd)
preempt_disable();
enable_kernel_fp();
- asm ("lfd%U1%X1 0,%1; stfs%U0%X0 0,%0" : "=m" (fprs) : "m" (fprd)
+ asm ("lfd%U1%X1 0,%1; stfs%U0%X0 0,%0" : "=m"UPD_CONSTR (fprs) : "m"UPD_CONSTR (fprd)
: "fr0");
preempt_enable();
return fprs;
--
2.25.0
^ permalink raw reply related
* [PATCH v3 2/3] powerpc: Fix incorrect stw{, ux, u, x} instructions in __set_pte_at
From: Christophe Leroy @ 2020-10-22 9:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
mathieu.desnoyers
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <25e6fca46fda3e2a4298448edbf654f64756eee7.1603358942.git.christophe.leroy@csgroup.eu>
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
The placeholder for instruction selection should use the second
argument's operand, which is %1, not %0. This could generate incorrect
assembly code if the memory addressing of operand %0 is a different
form from that of operand %1.
Also remove the %Un placeholder because having %Un placeholders
for two operands which are based on the same local var (ptep) doesn't
make much sense. By the way, it doesn't change the current behaviour
because "<>" constraint is missing for the associated "=m".
Fixes: 9bf2b5cdc5fe ("powerpc: Fixes for CONFIG_PTE_64BIT for SMP support")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: <stable@vger.kernel.org> # v2.6.28+
Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
[chleroy: revised commit log iaw segher's comments and removed %U0]
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Remove %Un
v2: Changed commit log.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/book3s/32/pgtable.h | 4 ++--
arch/powerpc/include/asm/nohash/pgtable.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index 36443cda8dcf..41d8bc6db303 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -522,9 +522,9 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
if (pte_val(*ptep) & _PAGE_HASHPTE)
flush_hash_entry(mm, ptep, addr);
__asm__ __volatile__("\
- stw%U0%X0 %2,%0\n\
+ stw%X0 %2,%0\n\
eieio\n\
- stw%U0%X0 %L2,%1"
+ stw%X1 %L2,%1"
: "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
: "r" (pte) : "memory");
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 6277e7596ae5..ac75f4ab0dba 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -192,9 +192,9 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
*/
if (IS_ENABLED(CONFIG_PPC32) && IS_ENABLED(CONFIG_PTE_64BIT) && !percpu) {
__asm__ __volatile__("\
- stw%U0%X0 %2,%0\n\
+ stw%X0 %2,%0\n\
eieio\n\
- stw%U0%X0 %L2,%1"
+ stw%X1 %L2,%1"
: "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
: "r" (pte) : "memory");
return;
--
2.25.0
^ permalink raw reply related
* [PATCH v3 1/3] powerpc/uaccess: Don't use "m<>" constraint with GCC 4.9
From: Christophe Leroy @ 2020-10-22 9:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
mathieu.desnoyers
Cc: linuxppc-dev, linux-kernel
GCC 4.9 sometimes fails to build with "m<>" constraint in
inline assembly.
CC lib/iov_iter.o
In file included from ./arch/powerpc/include/asm/cmpxchg.h:6:0,
from ./arch/powerpc/include/asm/atomic.h:11,
from ./include/linux/atomic.h:7,
from ./include/linux/crypto.h:15,
from ./include/crypto/hash.h:11,
from lib/iov_iter.c:2:
lib/iov_iter.c: In function 'iovec_from_user.part.30':
./arch/powerpc/include/asm/uaccess.h:287:2: error: 'asm' operand has impossible constraints
__asm__ __volatile__( \
^
./include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
./arch/powerpc/include/asm/uaccess.h:583:34: note: in expansion of macro 'unsafe_op_wrap'
#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
^
./arch/powerpc/include/asm/uaccess.h:329:10: note: in expansion of macro '__get_user_asm'
case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
^
./arch/powerpc/include/asm/uaccess.h:363:3: note: in expansion of macro '__get_user_size_allowed'
__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
^
./arch/powerpc/include/asm/uaccess.h:100:2: note: in expansion of macro '__get_user_nocheck'
__get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
^
./arch/powerpc/include/asm/uaccess.h:583:49: note: in expansion of macro '__get_user_allowed'
#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
^
lib/iov_iter.c:1663:3: note: in expansion of macro 'unsafe_get_user'
unsafe_get_user(len, &uiov[i].iov_len, uaccess_end);
^
make[1]: *** [scripts/Makefile.build:283: lib/iov_iter.o] Error 1
Define a UPD_CONSTR macro that is "<>" by default and
only "" with GCC prior to GCC 5.
Fixes: fcf1f26895a4 ("powerpc/uaccess: Add pre-update addressing to __put_user_asm_goto()")
Fixes: 2f279eeb68b8 ("powerpc/uaccess: Add pre-update addressing to __get_user_asm() and __put_user_asm()")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
---
v2: Moved UPD_CONSTR to asm-const.h to avoid circular inclusion issues with patch 3.
---
arch/powerpc/include/asm/asm-const.h | 13 +++++++++++++
arch/powerpc/include/asm/uaccess.h | 4 ++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/asm-const.h b/arch/powerpc/include/asm/asm-const.h
index 082c1538c562..0ce2368bd20f 100644
--- a/arch/powerpc/include/asm/asm-const.h
+++ b/arch/powerpc/include/asm/asm-const.h
@@ -11,4 +11,17 @@
# define __ASM_CONST(x) x##UL
# define ASM_CONST(x) __ASM_CONST(x)
#endif
+
+/*
+ * Inline assembly memory constraint
+ *
+ * GCC 4.9 doesn't properly handle pre update memory constraint "m<>"
+ *
+ */
+#if defined(GCC_VERSION) && GCC_VERSION < 50000
+#define UPD_CONSTR ""
+#else
+#define UPD_CONSTR "<>"
+#endif
+
#endif /* _ASM_POWERPC_ASM_CONST_H */
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 604d705f1bb8..8f27ea48fadb 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -223,7 +223,7 @@ do { \
"1: " op "%U1%X1 %0,%1 # put_user\n" \
EX_TABLE(1b, %l2) \
: \
- : "r" (x), "m<>" (*addr) \
+ : "r" (x), "m"UPD_CONSTR (*addr) \
: \
: label)
@@ -294,7 +294,7 @@ extern long __get_user_bad(void);
".previous\n" \
EX_TABLE(1b, 3b) \
: "=r" (err), "=r" (x) \
- : "m<>" (*addr), "i" (-EFAULT), "0" (err))
+ : "m"UPD_CONSTR (*addr), "i" (-EFAULT), "0" (err))
#ifdef __powerpc64__
#define __get_user_asm2(x, addr, err) \
--
2.25.0
^ permalink raw reply related
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 9:28 UTC (permalink / raw)
To: 'David Hildenbrand', Greg KH
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <e04d0c5d-e834-a15b-7844-44dcc82785cc@redhat.com>
From: David Hildenbrand
> Sent: 22 October 2020 10:19
>
> On 22.10.20 11:01, Greg KH wrote:
> > On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
> >> On 22.10.20 10:40, David Laight wrote:
> >>> From: David Hildenbrand
> >>>> Sent: 22 October 2020 09:35
> >>>>
> >>>> On 22.10.20 10:26, Greg KH wrote:
> >>>>> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
> >>>>>> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
> >>>>>>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
> >>>>>>>> From: David Laight <David.Laight@ACULAB.COM>
> >>>>>>>>
> >>>>>>>> This lets the compiler inline it into import_iovec() generating
> >>>>>>>> much better code.
> >>>>>>>>
> >>>>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
> >>>>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >>>>>>>> ---
> >>>>>>>> fs/read_write.c | 179 ------------------------------------------------
> >>>>>>>> lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>> 2 files changed, 176 insertions(+), 179 deletions(-)
> >>>>>>>
> >>>>>>> Strangely, this commit causes a regression in Linus's tree right now.
> >>>>>>>
> >>>>>>> I can't really figure out what the regression is, only that this commit
> >>>>>>> triggers a "large Android system binary" from working properly. There's
> >>>>>>> no kernel log messages anywhere, and I don't have any way to strace the
> >>>>>>> thing in the testing framework, so any hints that people can provide
> >>>>>>> would be most appreciated.
> >>>>>>
> >>>>>> It's a pure move - modulo changed line breaks in the argument lists
> >>>>>> the functions involved are identical before and after that (just checked
> >>>>>> that directly, by checking out the trees before and after, extracting two
> >>>>>> functions in question from fs/read_write.c and lib/iov_iter.c (before and
> >>>>>> after, resp.) and checking the diff between those.
> >>>>>>
> >>>>>> How certain is your bisection?
> >>>>>
> >>>>> The bisection is very reproducable.
> >>>>>
> >>>>> But, this looks now to be a compiler bug. I'm using the latest version
> >>>>> of clang and if I put "noinline" at the front of the function,
> >>>>> everything works.
> >>>>
> >>>> Well, the compiler can do more invasive optimizations when inlining. If
> >>>> you have buggy code that relies on some unspecified behavior, inlining
> >>>> can change the behavior ... but going over that code, there isn't too
> >>>> much action going on. At least nothing screamed at me.
> >>>
> >>> Apart from all the optimisations that get rid off the 'pass be reference'
> >>> parameters and strange conditional tests.
> >>> Plenty of scope for the compiler getting it wrong.
> >>> But nothing even vaguely illegal.
> >>
> >> Not the first time that people blame the compiler to then figure out
> >> that something else is wrong ... but maybe this time is different :)
> >
> > I agree, I hate to blame the compiler, that's almost never the real
> > problem, but this one sure "feels" like it.
> >
> > I'm running some more tests, trying to narrow things down as just adding
> > a "noinline" to the function that got moved here doesn't work on Linus's
> > tree at the moment because the function was split into multiple
> > functions.
> >
> > Give me a few hours...
>
> I might be wrong but
>
> a) import_iovec() uses:
> - unsigned nr_segs -> int
> - unsigned fast_segs -> int
> b) rw_copy_check_uvector() uses:
> - unsigned long nr_segs -> long
> - unsigned long fast_seg -> long
>
> So when calling rw_copy_check_uvector(), we have to zero-extend the
> registers used for passing the arguments. That's definitely done when
> calling the function explicitly. Maybe when inlining something is messed up?
That's also not needed on x86-64 - the high bits get cleared by 32bit writes.
But, IIRC, arm64 leaves them unchanged or undefined.
I guessing that every array access uses a *(Rx + Ry) addressing
mode. So indexing an array even with 'unsigned int' requires
an explicit zero-extend on arm64?
(x86-64 ends up with an explicit sign extend when indexing an
array with 'signed int'.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-22 9:25 UTC (permalink / raw)
To: Greg KH
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <e04d0c5d-e834-a15b-7844-44dcc82785cc@redhat.com>
On 22.10.20 11:19, David Hildenbrand wrote:
> On 22.10.20 11:01, Greg KH wrote:
>> On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
>>> On 22.10.20 10:40, David Laight wrote:
>>>> From: David Hildenbrand
>>>>> Sent: 22 October 2020 09:35
>>>>>
>>>>> On 22.10.20 10:26, Greg KH wrote:
>>>>>> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
>>>>>>> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
>>>>>>>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
>>>>>>>>> From: David Laight <David.Laight@ACULAB.COM>
>>>>>>>>>
>>>>>>>>> This lets the compiler inline it into import_iovec() generating
>>>>>>>>> much better code.
>>>>>>>>>
>>>>>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>>>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>>>>>>> ---
>>>>>>>>> fs/read_write.c | 179 ------------------------------------------------
>>>>>>>>> lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>> 2 files changed, 176 insertions(+), 179 deletions(-)
>>>>>>>>
>>>>>>>> Strangely, this commit causes a regression in Linus's tree right now.
>>>>>>>>
>>>>>>>> I can't really figure out what the regression is, only that this commit
>>>>>>>> triggers a "large Android system binary" from working properly. There's
>>>>>>>> no kernel log messages anywhere, and I don't have any way to strace the
>>>>>>>> thing in the testing framework, so any hints that people can provide
>>>>>>>> would be most appreciated.
>>>>>>>
>>>>>>> It's a pure move - modulo changed line breaks in the argument lists
>>>>>>> the functions involved are identical before and after that (just checked
>>>>>>> that directly, by checking out the trees before and after, extracting two
>>>>>>> functions in question from fs/read_write.c and lib/iov_iter.c (before and
>>>>>>> after, resp.) and checking the diff between those.
>>>>>>>
>>>>>>> How certain is your bisection?
>>>>>>
>>>>>> The bisection is very reproducable.
>>>>>>
>>>>>> But, this looks now to be a compiler bug. I'm using the latest version
>>>>>> of clang and if I put "noinline" at the front of the function,
>>>>>> everything works.
>>>>>
>>>>> Well, the compiler can do more invasive optimizations when inlining. If
>>>>> you have buggy code that relies on some unspecified behavior, inlining
>>>>> can change the behavior ... but going over that code, there isn't too
>>>>> much action going on. At least nothing screamed at me.
>>>>
>>>> Apart from all the optimisations that get rid off the 'pass be reference'
>>>> parameters and strange conditional tests.
>>>> Plenty of scope for the compiler getting it wrong.
>>>> But nothing even vaguely illegal.
>>>
>>> Not the first time that people blame the compiler to then figure out
>>> that something else is wrong ... but maybe this time is different :)
>>
>> I agree, I hate to blame the compiler, that's almost never the real
>> problem, but this one sure "feels" like it.
>>
>> I'm running some more tests, trying to narrow things down as just adding
>> a "noinline" to the function that got moved here doesn't work on Linus's
>> tree at the moment because the function was split into multiple
>> functions.
>>
>> Give me a few hours...
>
> I might be wrong but
>
> a) import_iovec() uses:
> - unsigned nr_segs -> int
> - unsigned fast_segs -> int
> b) rw_copy_check_uvector() uses:
> - unsigned long nr_segs -> long
> - unsigned long fast_seg -> long
>
> So when calling rw_copy_check_uvector(), we have to zero-extend the
> registers used for passing the arguments. That's definitely done when
> calling the function explicitly. Maybe when inlining something is messed up?
>
> Just a thought ...
>
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
Again, just a thought.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-22 9:19 UTC (permalink / raw)
To: Greg KH
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022090155.GA1483166@kroah.com>
On 22.10.20 11:01, Greg KH wrote:
> On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
>> On 22.10.20 10:40, David Laight wrote:
>>> From: David Hildenbrand
>>>> Sent: 22 October 2020 09:35
>>>>
>>>> On 22.10.20 10:26, Greg KH wrote:
>>>>> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
>>>>>> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
>>>>>>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
>>>>>>>> From: David Laight <David.Laight@ACULAB.COM>
>>>>>>>>
>>>>>>>> This lets the compiler inline it into import_iovec() generating
>>>>>>>> much better code.
>>>>>>>>
>>>>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>>>>>> ---
>>>>>>>> fs/read_write.c | 179 ------------------------------------------------
>>>>>>>> lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>> 2 files changed, 176 insertions(+), 179 deletions(-)
>>>>>>>
>>>>>>> Strangely, this commit causes a regression in Linus's tree right now.
>>>>>>>
>>>>>>> I can't really figure out what the regression is, only that this commit
>>>>>>> triggers a "large Android system binary" from working properly. There's
>>>>>>> no kernel log messages anywhere, and I don't have any way to strace the
>>>>>>> thing in the testing framework, so any hints that people can provide
>>>>>>> would be most appreciated.
>>>>>>
>>>>>> It's a pure move - modulo changed line breaks in the argument lists
>>>>>> the functions involved are identical before and after that (just checked
>>>>>> that directly, by checking out the trees before and after, extracting two
>>>>>> functions in question from fs/read_write.c and lib/iov_iter.c (before and
>>>>>> after, resp.) and checking the diff between those.
>>>>>>
>>>>>> How certain is your bisection?
>>>>>
>>>>> The bisection is very reproducable.
>>>>>
>>>>> But, this looks now to be a compiler bug. I'm using the latest version
>>>>> of clang and if I put "noinline" at the front of the function,
>>>>> everything works.
>>>>
>>>> Well, the compiler can do more invasive optimizations when inlining. If
>>>> you have buggy code that relies on some unspecified behavior, inlining
>>>> can change the behavior ... but going over that code, there isn't too
>>>> much action going on. At least nothing screamed at me.
>>>
>>> Apart from all the optimisations that get rid off the 'pass be reference'
>>> parameters and strange conditional tests.
>>> Plenty of scope for the compiler getting it wrong.
>>> But nothing even vaguely illegal.
>>
>> Not the first time that people blame the compiler to then figure out
>> that something else is wrong ... but maybe this time is different :)
>
> I agree, I hate to blame the compiler, that's almost never the real
> problem, but this one sure "feels" like it.
>
> I'm running some more tests, trying to narrow things down as just adding
> a "noinline" to the function that got moved here doesn't work on Linus's
> tree at the moment because the function was split into multiple
> functions.
>
> Give me a few hours...
I might be wrong but
a) import_iovec() uses:
- unsigned nr_segs -> int
- unsigned fast_segs -> int
b) rw_copy_check_uvector() uses:
- unsigned long nr_segs -> long
- unsigned long fast_seg -> long
So when calling rw_copy_check_uvector(), we have to zero-extend the
registers used for passing the arguments. That's definitely done when
calling the function explicitly. Maybe when inlining something is messed up?
Just a thought ...
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Arnd Bergmann @ 2020-10-22 9:14 UTC (permalink / raw)
To: Greg KH
Cc: linux-aio, open list:BROADCOM NVRAM DRIVER, David Howells,
Linux-MM, keyrings, sparclinux, Christoph Hellwig, linux-arch,
linux-s390, linux-scsi, Android Kernel Team, linux-block, Al Viro,
io-uring, Linux ARM, Jens Axboe, Parisc List, Networking,
Nick Desaulniers, linux-kernel@vger.kernel.org, LSM List,
David Laight, Linux FS-devel Mailing List, Andrew Morton,
linuxppc-dev
In-Reply-To: <20201022082654.GA1477657@kroah.com>
On Thu, Oct 22, 2020 at 10:26 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
> > On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
> > > On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
> > >
> > > I can't really figure out what the regression is, only that this commit
> > > triggers a "large Android system binary" from working properly. There's
> > > no kernel log messages anywhere, and I don't have any way to strace the
> > > thing in the testing framework, so any hints that people can provide
> > > would be most appreciated.
> >
> > It's a pure move - modulo changed line breaks in the argument lists
> > the functions involved are identical before and after that (just checked
> > that directly, by checking out the trees before and after, extracting two
> > functions in question from fs/read_write.c and lib/iov_iter.c (before and
> > after, resp.) and checking the diff between those.
> >
> > How certain is your bisection?
>
> The bisection is very reproducable.
>
> But, this looks now to be a compiler bug. I'm using the latest version
> of clang and if I put "noinline" at the front of the function,
> everything works.
>
> Nick, any ideas here as to who I should report this to?
>
> I'll work on a fixup patch for the Android kernel tree to see if I can
> work around it there, but others will hit this in Linus's tree sooner or
> later...
I see that Christoph rewrote the function again in bfdc59701d6d
("iov_iter: refactor rw_copy_check_uvector and import_iovec"),
do you know if the current mainline version is also affected?
Do you know if it happens across multiple architectures or might
be specific to either x86 or arm64?
https://bugs.llvm.org/ is obviously the place for reporting the
issue if it does turn out to be a bug in clang, but that requires
a specific thing going wrong in the output.
One idea I have for debugging it is to sprinkle the inlined
function with lots of barrier()s to prevent a lot of the optimizations.
If that solves the issue, you can bisect through those until you
find one barrier that makes the difference between working and
broken and then look at diff of the assembler output.
Arnd
^ permalink raw reply
* [PATCH] powerpc/mm: move setting pte specific flags to pfn_pmd
From: Aneesh Kumar K.V @ 2020-10-22 9:11 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: linux-mm, Aneesh Kumar K.V
powerpc used to set the pte specific flags in set_pte_at(). This is
different from other architectures. To be consistent with other
architecture powerpc updated pfn_pte to set _PAGE_PTE with
commit 379c926d6334 ("powerpc/mm: move setting pte specific flags to pfn_pte")
The commit didn't do the same w.r.t pfn_pmd because we expect pmd_mkhuge
to do that. But as per Linus that is a bad rule [1].
Hence update pfn_pmd to set _PAGE_PTE.
[1]
" The rule that you must use "pmd_mkhuge()" seems _completely_ wrong.
It's insane. The only valid use to ever make a pmd out of a pfn is to
make a huge-page."
message-id: CAHk-=whG+Z2mBFTT026PZAdjn=gSsLk9bk0wnYJ5peyuVGfwyg@mail.gmail.com
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 17 ++++++++++++++++-
arch/powerpc/mm/book3s64/pgtable.c | 8 +++++++-
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index cd3feeac6e87..a39886681629 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -1231,13 +1231,28 @@ static inline int pmd_same(pmd_t pmd_a, pmd_t pmd_b)
return hash__pmd_same(pmd_a, pmd_b);
}
-static inline pmd_t pmd_mkhuge(pmd_t pmd)
+static inline pmd_t __pmd_mkhuge(pmd_t pmd)
{
if (radix_enabled())
return radix__pmd_mkhuge(pmd);
return hash__pmd_mkhuge(pmd);
}
+/*
+ * pfn_pmd return a pmd_t that can be used as pmd pte entry.
+ */
+static inline pmd_t pmd_mkhuge(pmd_t pmd)
+{
+#ifdef CONFIG_DEBUG_VM
+ if (radix_enabled())
+ WARN_ON((pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE)) == 0);
+ else
+ WARN_ON((pmd_raw(pmd) & cpu_to_be64(_PAGE_PTE | H_PAGE_THP_HUGE)) !=
+ cpu_to_be64(_PAGE_PTE | H_PAGE_THP_HUGE));
+#endif
+ return pmd;
+}
+
#define __HAVE_ARCH_PMDP_SET_ACCESS_FLAGS
extern int pmdp_set_access_flags(struct vm_area_struct *vma,
unsigned long address, pmd_t *pmdp,
diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
index e18ae50a275c..5b3a3bae21aa 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -136,12 +136,18 @@ static pmd_t pmd_set_protbits(pmd_t pmd, pgprot_t pgprot)
return __pmd(pmd_val(pmd) | pgprot_val(pgprot));
}
+/*
+ * At some point we should be able to get rid of
+ * pmd_mkhuge() and mk_huge_pmd() when we update all the
+ * other archs to mark the pmd huge in pfn_pmd()
+ */
pmd_t pfn_pmd(unsigned long pfn, pgprot_t pgprot)
{
unsigned long pmdv;
pmdv = (pfn << PAGE_SHIFT) & PTE_RPN_MASK;
- return pmd_set_protbits(__pmd(pmdv), pgprot);
+
+ return __pmd_mkhuge(pmd_set_protbits(__pmd(pmdv), pgprot));
}
pmd_t mk_pmd(struct page *page, pgprot_t pgprot)
--
2.26.2
^ permalink raw reply related
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 9:06 UTC (permalink / raw)
To: 'Greg KH', David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022090155.GA1483166@kroah.com>
From: Greg KH
> Sent: 22 October 2020 10:02
...
> I'm running some more tests, trying to narrow things down as just adding
> a "noinline" to the function that got moved here doesn't work on Linus's
> tree at the moment because the function was split into multiple
> functions.
I was going to look at that once rc2 is in - and the kernel is
likely to work.
I suspect the split version doesn't get inlined the same way?
Which leaves the horrid argument conversion the inline got
rid of back there again.
Which all rather begs the question of why the compiler doesn't
generate the expected code.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 9:02 UTC (permalink / raw)
To: 'David Hildenbrand', Greg KH, Al Viro, Nick Desaulniers
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <df2e0758-b8ed-5aec-6adc-a18f499c0179@redhat.com>
From: David Hildenbrand
> Sent: 22 October 2020 09:49
...
> >>> But, this looks now to be a compiler bug. I'm using the latest version
> >>> of clang and if I put "noinline" at the front of the function,
> >>> everything works.
> >>
> >> Well, the compiler can do more invasive optimizations when inlining. If
> >> you have buggy code that relies on some unspecified behavior, inlining
> >> can change the behavior ... but going over that code, there isn't too
> >> much action going on. At least nothing screamed at me.
> >
> > Apart from all the optimisations that get rid off the 'pass be reference'
> > parameters and strange conditional tests.
> > Plenty of scope for the compiler getting it wrong.
> > But nothing even vaguely illegal.
>
> Not the first time that people blame the compiler to then figure out
> that something else is wrong ... but maybe this time is different :)
Usually down to missing asm 'memory' constraints...
Need to read the obj file to see what the compiler did.
The code must be 'approximately right' or nothing would run.
So I'd guess it has to do with > 8 fragments.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 9:01 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <df2e0758-b8ed-5aec-6adc-a18f499c0179@redhat.com>
On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
> On 22.10.20 10:40, David Laight wrote:
> > From: David Hildenbrand
> >> Sent: 22 October 2020 09:35
> >>
> >> On 22.10.20 10:26, Greg KH wrote:
> >>> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
> >>>> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
> >>>>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
> >>>>>> From: David Laight <David.Laight@ACULAB.COM>
> >>>>>>
> >>>>>> This lets the compiler inline it into import_iovec() generating
> >>>>>> much better code.
> >>>>>>
> >>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
> >>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >>>>>> ---
> >>>>>> fs/read_write.c | 179 ------------------------------------------------
> >>>>>> lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>> 2 files changed, 176 insertions(+), 179 deletions(-)
> >>>>>
> >>>>> Strangely, this commit causes a regression in Linus's tree right now.
> >>>>>
> >>>>> I can't really figure out what the regression is, only that this commit
> >>>>> triggers a "large Android system binary" from working properly. There's
> >>>>> no kernel log messages anywhere, and I don't have any way to strace the
> >>>>> thing in the testing framework, so any hints that people can provide
> >>>>> would be most appreciated.
> >>>>
> >>>> It's a pure move - modulo changed line breaks in the argument lists
> >>>> the functions involved are identical before and after that (just checked
> >>>> that directly, by checking out the trees before and after, extracting two
> >>>> functions in question from fs/read_write.c and lib/iov_iter.c (before and
> >>>> after, resp.) and checking the diff between those.
> >>>>
> >>>> How certain is your bisection?
> >>>
> >>> The bisection is very reproducable.
> >>>
> >>> But, this looks now to be a compiler bug. I'm using the latest version
> >>> of clang and if I put "noinline" at the front of the function,
> >>> everything works.
> >>
> >> Well, the compiler can do more invasive optimizations when inlining. If
> >> you have buggy code that relies on some unspecified behavior, inlining
> >> can change the behavior ... but going over that code, there isn't too
> >> much action going on. At least nothing screamed at me.
> >
> > Apart from all the optimisations that get rid off the 'pass be reference'
> > parameters and strange conditional tests.
> > Plenty of scope for the compiler getting it wrong.
> > But nothing even vaguely illegal.
>
> Not the first time that people blame the compiler to then figure out
> that something else is wrong ... but maybe this time is different :)
I agree, I hate to blame the compiler, that's almost never the real
problem, but this one sure "feels" like it.
I'm running some more tests, trying to narrow things down as just adding
a "noinline" to the function that got moved here doesn't work on Linus's
tree at the moment because the function was split into multiple
functions.
Give me a few hours...
thanks,
greg k-h
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-22 8:48 UTC (permalink / raw)
To: David Laight, Greg KH, Al Viro, Nick Desaulniers
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <5d2ecb24db1e415b8ff88261435386ec@AcuMS.aculab.com>
On 22.10.20 10:40, David Laight wrote:
> From: David Hildenbrand
>> Sent: 22 October 2020 09:35
>>
>> On 22.10.20 10:26, Greg KH wrote:
>>> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
>>>> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
>>>>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
>>>>>> From: David Laight <David.Laight@ACULAB.COM>
>>>>>>
>>>>>> This lets the compiler inline it into import_iovec() generating
>>>>>> much better code.
>>>>>>
>>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>>>> ---
>>>>>> fs/read_write.c | 179 ------------------------------------------------
>>>>>> lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 176 insertions(+), 179 deletions(-)
>>>>>
>>>>> Strangely, this commit causes a regression in Linus's tree right now.
>>>>>
>>>>> I can't really figure out what the regression is, only that this commit
>>>>> triggers a "large Android system binary" from working properly. There's
>>>>> no kernel log messages anywhere, and I don't have any way to strace the
>>>>> thing in the testing framework, so any hints that people can provide
>>>>> would be most appreciated.
>>>>
>>>> It's a pure move - modulo changed line breaks in the argument lists
>>>> the functions involved are identical before and after that (just checked
>>>> that directly, by checking out the trees before and after, extracting two
>>>> functions in question from fs/read_write.c and lib/iov_iter.c (before and
>>>> after, resp.) and checking the diff between those.
>>>>
>>>> How certain is your bisection?
>>>
>>> The bisection is very reproducable.
>>>
>>> But, this looks now to be a compiler bug. I'm using the latest version
>>> of clang and if I put "noinline" at the front of the function,
>>> everything works.
>>
>> Well, the compiler can do more invasive optimizations when inlining. If
>> you have buggy code that relies on some unspecified behavior, inlining
>> can change the behavior ... but going over that code, there isn't too
>> much action going on. At least nothing screamed at me.
>
> Apart from all the optimisations that get rid off the 'pass be reference'
> parameters and strange conditional tests.
> Plenty of scope for the compiler getting it wrong.
> But nothing even vaguely illegal.
Not the first time that people blame the compiler to then figure out
that something else is wrong ... but maybe this time is different :)
--
Thanks,
David / dhildenb
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 8:40 UTC (permalink / raw)
To: 'David Hildenbrand', Greg KH, Al Viro, Nick Desaulniers
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <80a2e5fa-718a-8433-1ab0-dd5b3e3b5416@redhat.com>
From: David Hildenbrand
> Sent: 22 October 2020 09:35
>
> On 22.10.20 10:26, Greg KH wrote:
> > On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
> >> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
> >>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
> >>>> From: David Laight <David.Laight@ACULAB.COM>
> >>>>
> >>>> This lets the compiler inline it into import_iovec() generating
> >>>> much better code.
> >>>>
> >>>> Signed-off-by: David Laight <david.laight@aculab.com>
> >>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >>>> ---
> >>>> fs/read_write.c | 179 ------------------------------------------------
> >>>> lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 176 insertions(+), 179 deletions(-)
> >>>
> >>> Strangely, this commit causes a regression in Linus's tree right now.
> >>>
> >>> I can't really figure out what the regression is, only that this commit
> >>> triggers a "large Android system binary" from working properly. There's
> >>> no kernel log messages anywhere, and I don't have any way to strace the
> >>> thing in the testing framework, so any hints that people can provide
> >>> would be most appreciated.
> >>
> >> It's a pure move - modulo changed line breaks in the argument lists
> >> the functions involved are identical before and after that (just checked
> >> that directly, by checking out the trees before and after, extracting two
> >> functions in question from fs/read_write.c and lib/iov_iter.c (before and
> >> after, resp.) and checking the diff between those.
> >>
> >> How certain is your bisection?
> >
> > The bisection is very reproducable.
> >
> > But, this looks now to be a compiler bug. I'm using the latest version
> > of clang and if I put "noinline" at the front of the function,
> > everything works.
>
> Well, the compiler can do more invasive optimizations when inlining. If
> you have buggy code that relies on some unspecified behavior, inlining
> can change the behavior ... but going over that code, there isn't too
> much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* [PATCH] selftests/powerpc/eeh: disable kselftest timeout setting for eeh-basic
From: Po-Hsu Lin @ 2020-10-22 8:36 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-kselftest
Cc: joe.lawrence, po-hsu.lin, mathieu.desnoyers, mbenes, shuah
The eeh-basic test got its own 60 seconds timeout (defined in commit
414f50434aa2 "selftests/eeh: Bump EEH wait time to 60s") per breakable
device.
And we have discovered that the number of breakable devices varies
on different hardware. The device recovery time ranges from 0 to 35
seconds. In our test pool it will take about 30 seconds to run on a
Power8 system that with 5 breakable devices, 60 seconds to run on a
Power9 system that with 4 breakable devices.
Thus it's better to disable the default 45 seconds timeout setting in
the kselftest framework to give it a chance to finish. And let the
test to take care of the timeout control.
Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
---
tools/testing/selftests/powerpc/eeh/Makefile | 2 +-
tools/testing/selftests/powerpc/eeh/settings | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/powerpc/eeh/settings
diff --git a/tools/testing/selftests/powerpc/eeh/Makefile b/tools/testing/selftests/powerpc/eeh/Makefile
index b397bab..ae963eb 100644
--- a/tools/testing/selftests/powerpc/eeh/Makefile
+++ b/tools/testing/selftests/powerpc/eeh/Makefile
@@ -3,7 +3,7 @@ noarg:
$(MAKE) -C ../
TEST_PROGS := eeh-basic.sh
-TEST_FILES := eeh-functions.sh
+TEST_FILES := eeh-functions.sh settings
top_srcdir = ../../../../..
include ../../lib.mk
diff --git a/tools/testing/selftests/powerpc/eeh/settings b/tools/testing/selftests/powerpc/eeh/settings
new file mode 100644
index 0000000..e7b9417
--- /dev/null
+++ b/tools/testing/selftests/powerpc/eeh/settings
@@ -0,0 +1 @@
+timeout=0
--
2.7.4
^ permalink raw reply related
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-22 8:35 UTC (permalink / raw)
To: Greg KH, Al Viro, Nick Desaulniers
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, Christoph Hellwig, linux-arch, linux-s390, linux-scsi,
kernel-team, Arnd Bergmann, linux-block, io-uring,
linux-arm-kernel, Jens Axboe, linux-parisc, netdev, linux-kernel,
linux-security-module, David Laight, linux-fsdevel, Andrew Morton,
linuxppc-dev
In-Reply-To: <20201022082654.GA1477657@kroah.com>
On 22.10.20 10:26, Greg KH wrote:
> On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
>> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
>>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
>>>> From: David Laight <David.Laight@ACULAB.COM>
>>>>
>>>> This lets the compiler inline it into import_iovec() generating
>>>> much better code.
>>>>
>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>> ---
>>>> fs/read_write.c | 179 ------------------------------------------------
>>>> lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 176 insertions(+), 179 deletions(-)
>>>
>>> Strangely, this commit causes a regression in Linus's tree right now.
>>>
>>> I can't really figure out what the regression is, only that this commit
>>> triggers a "large Android system binary" from working properly. There's
>>> no kernel log messages anywhere, and I don't have any way to strace the
>>> thing in the testing framework, so any hints that people can provide
>>> would be most appreciated.
>>
>> It's a pure move - modulo changed line breaks in the argument lists
>> the functions involved are identical before and after that (just checked
>> that directly, by checking out the trees before and after, extracting two
>> functions in question from fs/read_write.c and lib/iov_iter.c (before and
>> after, resp.) and checking the diff between those.
>>
>> How certain is your bisection?
>
> The bisection is very reproducable.
>
> But, this looks now to be a compiler bug. I'm using the latest version
> of clang and if I put "noinline" at the front of the function,
> everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 8:26 UTC (permalink / raw)
To: Al Viro, Nick Desaulniers
Cc: linux-aio, linux-mips, David Howells, linux-mm, keyrings,
sparclinux, Christoph Hellwig, linux-arch, linux-s390, linux-scsi,
kernel-team, Arnd Bergmann, linux-block, io-uring,
linux-arm-kernel, Jens Axboe, linux-parisc, netdev, linux-kernel,
linux-security-module, David Laight, linux-fsdevel, Andrew Morton,
linuxppc-dev
In-Reply-To: <20201021233914.GR3576660@ZenIV.linux.org.uk>
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
> On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
> > On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
> > > From: David Laight <David.Laight@ACULAB.COM>
> > >
> > > This lets the compiler inline it into import_iovec() generating
> > > much better code.
> > >
> > > Signed-off-by: David Laight <david.laight@aculab.com>
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > > fs/read_write.c | 179 ------------------------------------------------
> > > lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 176 insertions(+), 179 deletions(-)
> >
> > Strangely, this commit causes a regression in Linus's tree right now.
> >
> > I can't really figure out what the regression is, only that this commit
> > triggers a "large Android system binary" from working properly. There's
> > no kernel log messages anywhere, and I don't have any way to strace the
> > thing in the testing framework, so any hints that people can provide
> > would be most appreciated.
>
> It's a pure move - modulo changed line breaks in the argument lists
> the functions involved are identical before and after that (just checked
> that directly, by checking out the trees before and after, extracting two
> functions in question from fs/read_write.c and lib/iov_iter.c (before and
> after, resp.) and checking the diff between those.
>
> How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Nick, any ideas here as to who I should report this to?
I'll work on a fixup patch for the Android kernel tree to see if I can
work around it there, but others will hit this in Linus's tree sooner or
later...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available
From: Geert Uytterhoeven @ 2020-10-22 7:26 UTC (permalink / raw)
To: Finn Thain
Cc: Greg KH, Laurent Vivier, Linux Kernel Mailing List, linux-m68k,
Paul Mackerras, open list:SERIAL DRIVERS, Brad Boyer,
linuxppc-dev, Joshua Thompson
In-Reply-To: <alpine.LNX.2.23.453.2010221347000.6@nippy.intranet>
Hi Finn,
On Thu, Oct 22, 2020 at 5:23 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> The patch below seems to fix the problem for me. Does it work on your
> system(s)?
Thanks for your patch!
> --- a/arch/m68k/mac/config.c
> +++ b/arch/m68k/mac/config.c
> @@ -776,16 +776,12 @@ static struct resource scc_b_rsrcs[] = {
> struct platform_device scc_a_pdev = {
> .name = "scc",
> .id = 0,
> - .num_resources = ARRAY_SIZE(scc_a_rsrcs),
> - .resource = scc_a_rsrcs,
> };
> EXPORT_SYMBOL(scc_a_pdev);
>
> struct platform_device scc_b_pdev = {
> .name = "scc",
> .id = 1,
> - .num_resources = ARRAY_SIZE(scc_b_rsrcs),
> - .resource = scc_b_rsrcs,
> };
> EXPORT_SYMBOL(scc_b_pdev);
>
> @@ -812,10 +808,15 @@ static void __init mac_identify(void)
>
> /* Set up serial port resources for the console initcall. */
>
> - scc_a_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase + 2;
> - scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
> - scc_b_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase;
> - scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
> + scc_a_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase + 2;
> + scc_a_rsrcs[0].end = scc_a_rsrcs[0].start;
> + scc_a_pdev.num_resources = ARRAY_SIZE(scc_a_rsrcs);
> + scc_a_pdev.resource = scc_a_rsrcs;
> +
> + scc_b_rsrcs[0].start = (resource_size_t)mac_bi_data.sccbase;
> + scc_b_rsrcs[0].end = scc_b_rsrcs[0].start;
> + scc_b_pdev.num_resources = ARRAY_SIZE(scc_b_rsrcs);
> + scc_b_pdev.resource = scc_b_rsrcs;
I can't say I'm a fan of this...
>
> switch (macintosh_config->scc_type) {
> case MAC_SCC_PSC:
> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> index 96e7aa479961..95abdb305d67 100644
> --- a/drivers/tty/serial/pmac_zilog.c
> +++ b/drivers/tty/serial/pmac_zilog.c
> @@ -1697,18 +1697,17 @@ extern struct platform_device scc_a_pdev, scc_b_pdev;
The real issue is this "extern struct platform_device scc_a_pdev, scc_b_pdev",
circumventing the driver framework.
Can we get rid of that?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available
From: Laurent Vivier @ 2020-10-22 7:16 UTC (permalink / raw)
To: Finn Thain
Cc: Greg KH, linux-kernel, linux-m68k, Geert Uytterhoeven,
linux-serial, Brad Boyer, Paul Mackerras, linuxppc-dev,
Joshua Thompson
In-Reply-To: <alpine.LNX.2.23.453.2010221347000.6@nippy.intranet>
Le 22/10/2020 à 05:23, Finn Thain a écrit :
> On Wed, 21 Oct 2020, Laurent Vivier wrote:
>
>> Le 21/10/2020 à 01:43, Finn Thain a écrit :
>>
>>> Laurent, can we avoid the irq == 0 warning splat like this?
>>>
>>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
>>> index 96e7aa479961..7db600cd8cc7 100644
>>> --- a/drivers/tty/serial/pmac_zilog.c
>>> +++ b/drivers/tty/serial/pmac_zilog.c
>>> @@ -1701,8 +1701,10 @@ static int __init pmz_init_port(struct uart_pmac_port *uap)
>>> int irq;
>>>
>>> r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
>>> + if (!r_ports)
>>> + return -ENODEV;
>>> irq = platform_get_irq(uap->pdev, 0);
>>> - if (!r_ports || irq <= 0)
>>> + if (irq <= 0)
>>> return -ENODEV;
>>>
>>> uap->port.mapbase = r_ports->start;
>>>
>>
>> No, this doesn't fix the problem.
>>
>
> Then I had better stop guessing and start up Aranym...
>
> The patch below seems to fix the problem for me. Does it work on your
> system(s)?
It works like a charm.
Thank you,
Laurent
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox