* [PATCH 0/2] arm64/sve: Misc fixes
@ 2020-06-10 17:03 Dave Martin
2020-06-10 17:03 ` [PATCH 1/2] docs/arm64: Fix typo'd #define in sve.rst Dave Martin
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Dave Martin @ 2020-06-10 17:03 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Catalin Marinas, Will Deacon
A couple of unrelated minor fixes: one documentation typo fix, and one
fix for a (mostly) theoretical data race.
Dave Martin (2):
docs/arm64: Fix typo'd #define in sve.rst
arm64/sve: Eliminate data races on sve_default_vl
Documentation/arm64/sve.rst | 6 +++---
arch/arm64/kernel/fpsimd.c | 25 ++++++++++++++++++-------
2 files changed, 21 insertions(+), 10 deletions(-)
--
2.1.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] docs/arm64: Fix typo'd #define in sve.rst 2020-06-10 17:03 [PATCH 0/2] arm64/sve: Misc fixes Dave Martin @ 2020-06-10 17:03 ` Dave Martin 2020-06-10 17:03 ` [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl Dave Martin 2020-06-15 16:34 ` [PATCH 0/2] arm64/sve: Misc fixes Will Deacon 2 siblings, 0 replies; 12+ messages in thread From: Dave Martin @ 2020-06-10 17:03 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Catalin Marinas, Will Deacon sve.rst describes a flag PR_SVE_SET_VL_INHERIT for the PR_SVE_SET_VL prctl, but there is no flag of this name. The flag is shared between the _GET and _SET calls, so the _SET prefix was dropped, giving the name PR_SVE_VL_INHERIT in the headers. Fix it. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- Documentation/arm64/sve.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/arm64/sve.rst b/Documentation/arm64/sve.rst index 5689c74..bfd55f4 100644 --- a/Documentation/arm64/sve.rst +++ b/Documentation/arm64/sve.rst @@ -186,7 +186,7 @@ prctl(PR_SVE_SET_VL, unsigned long arg) flags: - PR_SVE_SET_VL_INHERIT + PR_SVE_VL_INHERIT Inherit the current vector length across execve(). Otherwise, the vector length is reset to the system default at execve(). (See @@ -247,7 +247,7 @@ prctl(PR_SVE_GET_VL) The following flag may be OR-ed into the result: - PR_SVE_SET_VL_INHERIT + PR_SVE_VL_INHERIT Vector length will be inherited across execve(). @@ -393,7 +393,7 @@ The regset data starts with struct user_sve_header, containing: * At every execve() call, the new vector length of the new process is set to the system default vector length, unless - * PR_SVE_SET_VL_INHERIT (or equivalently SVE_PT_VL_INHERIT) is set for the + * PR_SVE_VL_INHERIT (or equivalently SVE_PT_VL_INHERIT) is set for the calling thread, or * a deferred vector length change is pending, established via the -- 2.1.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-10 17:03 [PATCH 0/2] arm64/sve: Misc fixes Dave Martin 2020-06-10 17:03 ` [PATCH 1/2] docs/arm64: Fix typo'd #define in sve.rst Dave Martin @ 2020-06-10 17:03 ` Dave Martin 2020-06-16 13:18 ` Qian Cai 2020-06-15 16:34 ` [PATCH 0/2] arm64/sve: Misc fixes Will Deacon 2 siblings, 1 reply; 12+ messages in thread From: Dave Martin @ 2020-06-10 17:03 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Catalin Marinas, Will Deacon sve_default_vl can be modified via the /proc/sys/abi/sve_default_vl sysctl concurrently with use, and modified concurrently by multiple threads. Adding a lock for this seems overkill, and I don't want to think any more than necessary, so just define wrappers using READ_ONCE()/ WRITE_ONCE(). This will avoid the possibility of torn accesses and repeated loads and stores. There's no evidence yet that this is going wrong in practice: this is just hygiene. For generic sysctl users, it would be better to build this kind of thing into the sysctl common code somehow. Reported-by: Will Deacon <will@kernel.org> Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- Build-tested only, but it seems pretty straightforward. arch/arm64/kernel/fpsimd.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 94289d1..19865c9 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -12,6 +12,7 @@ #include <linux/bug.h> #include <linux/cache.h> #include <linux/compat.h> +#include <linux/compiler.h> #include <linux/cpu.h> #include <linux/cpu_pm.h> #include <linux/kernel.h> @@ -119,7 +120,17 @@ struct fpsimd_last_state_struct { static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state); /* Default VL for tasks that don't set it explicitly: */ -static int sve_default_vl = -1; +static int __sve_default_vl = -1; + +static int get_sve_default_vl(void) +{ + return READ_ONCE(__sve_default_vl); +} + +static void set_sve_default_vl(int val) +{ + WRITE_ONCE(__sve_default_vl, val); +} #ifdef CONFIG_ARM64_SVE @@ -345,7 +356,7 @@ static int sve_proc_do_default_vl(struct ctl_table *table, int write, loff_t *ppos) { int ret; - int vl = sve_default_vl; + int vl = get_sve_default_vl(); struct ctl_table tmp_table = { .data = &vl, .maxlen = sizeof(vl), @@ -362,7 +373,7 @@ static int sve_proc_do_default_vl(struct ctl_table *table, int write, if (!sve_vl_valid(vl)) return -EINVAL; - sve_default_vl = find_supported_vector_length(vl); + set_sve_default_vl(find_supported_vector_length(vl)); return 0; } @@ -869,7 +880,7 @@ void __init sve_setup(void) * For the default VL, pick the maximum supported value <= 64. * VL == 64 is guaranteed not to grow the signal frame. */ - sve_default_vl = find_supported_vector_length(64); + set_sve_default_vl(find_supported_vector_length(64)); bitmap_andnot(tmp_map, sve_vq_partial_map, sve_vq_map, SVE_VQ_MAX); @@ -890,7 +901,7 @@ void __init sve_setup(void) pr_info("SVE: maximum available vector length %u bytes per vector\n", sve_max_vl); pr_info("SVE: default vector length %u bytes per vector\n", - sve_default_vl); + get_sve_default_vl()); /* KVM decides whether to support mismatched systems. Just warn here: */ if (sve_max_virtualisable_vl < sve_max_vl) @@ -1030,13 +1041,13 @@ void fpsimd_flush_thread(void) * vector length configured: no kernel task can become a user * task without an exec and hence a call to this function. * By the time the first call to this function is made, all - * early hardware probing is complete, so sve_default_vl + * early hardware probing is complete, so __sve_default_vl * should be valid. * If a bug causes this to go wrong, we make some noise and * try to fudge thread.sve_vl to a safe value here. */ vl = current->thread.sve_vl_onexec ? - current->thread.sve_vl_onexec : sve_default_vl; + current->thread.sve_vl_onexec : get_sve_default_vl(); if (WARN_ON(!sve_vl_valid(vl))) vl = SVE_VL_MIN; -- 2.1.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-10 17:03 ` [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl Dave Martin @ 2020-06-16 13:18 ` Qian Cai 2020-06-16 15:04 ` Will Deacon 0 siblings, 1 reply; 12+ messages in thread From: Qian Cai @ 2020-06-16 13:18 UTC (permalink / raw) To: Dave Martin; +Cc: Catalin Marinas, Will Deacon, linux-arm-kernel On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote: > sve_default_vl can be modified via the /proc/sys/abi/sve_default_vl > sysctl concurrently with use, and modified concurrently by multiple > threads. > > Adding a lock for this seems overkill, and I don't want to think any > more than necessary, so just define wrappers using READ_ONCE()/ > WRITE_ONCE(). > > This will avoid the possibility of torn accesses and repeated loads > and stores. > > There's no evidence yet that this is going wrong in practice: this > is just hygiene. For generic sysctl users, it would be better to > build this kind of thing into the sysctl common code somehow. > > Reported-by: Will Deacon <will@kernel.org> > Signed-off-by: Dave Martin <Dave.Martin@arm.com> While this original patch looks correct, linux-next has this, [will: move set_sve_default_vl() inside #ifdef to squash allnoconfig warning] 1e570f512cbd ("arm64/sve: Eliminate data races on sve_default_vl") which causes an error with CONFIG_ARM64_SVE=n, This .config, https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config arch/arm64/kernel/fpsimd.c: In function ‘sve_proc_do_default_vl’: arch/arm64/kernel/fpsimd.c:375:2: error: implicit declaration of function ‘set_sve_default_vl’; did you mean ‘get_sve_default_vl’? [-Werror=implicit-function-declaration] set_sve_default_vl(find_supported_vector_length(vl)); ^~~~~~~~~~~~~~~~~~ get_sve_default_vl > --- > > Build-tested only, but it seems pretty straightforward. > > arch/arm64/kernel/fpsimd.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 94289d1..19865c9 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -12,6 +12,7 @@ > #include <linux/bug.h> > #include <linux/cache.h> > #include <linux/compat.h> > +#include <linux/compiler.h> > #include <linux/cpu.h> > #include <linux/cpu_pm.h> > #include <linux/kernel.h> > @@ -119,7 +120,17 @@ struct fpsimd_last_state_struct { > static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state); > > /* Default VL for tasks that don't set it explicitly: */ > -static int sve_default_vl = -1; > +static int __sve_default_vl = -1; > + > +static int get_sve_default_vl(void) > +{ > + return READ_ONCE(__sve_default_vl); > +} > + > +static void set_sve_default_vl(int val) > +{ > + WRITE_ONCE(__sve_default_vl, val); > +} > > #ifdef CONFIG_ARM64_SVE > > @@ -345,7 +356,7 @@ static int sve_proc_do_default_vl(struct ctl_table *table, int write, > loff_t *ppos) > { > int ret; > - int vl = sve_default_vl; > + int vl = get_sve_default_vl(); > struct ctl_table tmp_table = { > .data = &vl, > .maxlen = sizeof(vl), > @@ -362,7 +373,7 @@ static int sve_proc_do_default_vl(struct ctl_table *table, int write, > if (!sve_vl_valid(vl)) > return -EINVAL; > > - sve_default_vl = find_supported_vector_length(vl); > + set_sve_default_vl(find_supported_vector_length(vl)); > return 0; > } > > @@ -869,7 +880,7 @@ void __init sve_setup(void) > * For the default VL, pick the maximum supported value <= 64. > * VL == 64 is guaranteed not to grow the signal frame. > */ > - sve_default_vl = find_supported_vector_length(64); > + set_sve_default_vl(find_supported_vector_length(64)); > > bitmap_andnot(tmp_map, sve_vq_partial_map, sve_vq_map, > SVE_VQ_MAX); > @@ -890,7 +901,7 @@ void __init sve_setup(void) > pr_info("SVE: maximum available vector length %u bytes per vector\n", > sve_max_vl); > pr_info("SVE: default vector length %u bytes per vector\n", > - sve_default_vl); > + get_sve_default_vl()); > > /* KVM decides whether to support mismatched systems. Just warn here: */ > if (sve_max_virtualisable_vl < sve_max_vl) > @@ -1030,13 +1041,13 @@ void fpsimd_flush_thread(void) > * vector length configured: no kernel task can become a user > * task without an exec and hence a call to this function. > * By the time the first call to this function is made, all > - * early hardware probing is complete, so sve_default_vl > + * early hardware probing is complete, so __sve_default_vl > * should be valid. > * If a bug causes this to go wrong, we make some noise and > * try to fudge thread.sve_vl to a safe value here. > */ > vl = current->thread.sve_vl_onexec ? > - current->thread.sve_vl_onexec : sve_default_vl; > + current->thread.sve_vl_onexec : get_sve_default_vl(); > > if (WARN_ON(!sve_vl_valid(vl))) > vl = SVE_VL_MIN; > -- > 2.1.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-16 13:18 ` Qian Cai @ 2020-06-16 15:04 ` Will Deacon 2020-06-16 16:17 ` Dave Martin 0 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2020-06-16 15:04 UTC (permalink / raw) To: Qian Cai; +Cc: Catalin Marinas, Dave Martin, linux-arm-kernel On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote: > On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote: > > sve_default_vl can be modified via the /proc/sys/abi/sve_default_vl > > sysctl concurrently with use, and modified concurrently by multiple > > threads. > > > > Adding a lock for this seems overkill, and I don't want to think any > > more than necessary, so just define wrappers using READ_ONCE()/ > > WRITE_ONCE(). > > > > This will avoid the possibility of torn accesses and repeated loads > > and stores. > > > > There's no evidence yet that this is going wrong in practice: this > > is just hygiene. For generic sysctl users, it would be better to > > build this kind of thing into the sysctl common code somehow. > > > > Reported-by: Will Deacon <will@kernel.org> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > While this original patch looks correct, linux-next has this, > > [will: move set_sve_default_vl() inside #ifdef to squash allnoconfig warning] > > 1e570f512cbd ("arm64/sve: Eliminate data races on sve_default_vl") > > which causes an error with CONFIG_ARM64_SVE=n, > > This .config, > https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config > > arch/arm64/kernel/fpsimd.c: In function ‘sve_proc_do_default_vl’: > arch/arm64/kernel/fpsimd.c:375:2: error: implicit declaration of > function ‘set_sve_default_vl’; did you mean ‘get_sve_default_vl’? > [-Werror=implicit-function-declaration] > set_sve_default_vl(find_supported_vector_length(vl)); > ^~~~~~~~~~~~~~~~~~ > get_sve_default_vl Thanks, I'll take a look. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-16 15:04 ` Will Deacon @ 2020-06-16 16:17 ` Dave Martin 2020-06-16 17:19 ` Will Deacon 0 siblings, 1 reply; 12+ messages in thread From: Dave Martin @ 2020-06-16 16:17 UTC (permalink / raw) To: Will Deacon; +Cc: Catalin Marinas, Qian Cai, linux-arm-kernel On Tue, Jun 16, 2020 at 04:04:51PM +0100, Will Deacon wrote: > On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote: > > On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote: > > > sve_default_vl can be modified via the /proc/sys/abi/sve_default_vl > > > sysctl concurrently with use, and modified concurrently by multiple > > > threads. > > > > > > Adding a lock for this seems overkill, and I don't want to think any > > > more than necessary, so just define wrappers using READ_ONCE()/ > > > WRITE_ONCE(). > > > > > > This will avoid the possibility of torn accesses and repeated loads > > > and stores. > > > > > > There's no evidence yet that this is going wrong in practice: this > > > is just hygiene. For generic sysctl users, it would be better to > > > build this kind of thing into the sysctl common code somehow. > > > > > > Reported-by: Will Deacon <will@kernel.org> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > While this original patch looks correct, linux-next has this, > > > > [will: move set_sve_default_vl() inside #ifdef to squash allnoconfig warning] > > > > 1e570f512cbd ("arm64/sve: Eliminate data races on sve_default_vl") > > > > which causes an error with CONFIG_ARM64_SVE=n, > > > > This .config, > > https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config > > > > arch/arm64/kernel/fpsimd.c: In function ‘sve_proc_do_default_vl’: > > arch/arm64/kernel/fpsimd.c:375:2: error: implicit declaration of > > function ‘set_sve_default_vl’; did you mean ‘get_sve_default_vl’? > > [-Werror=implicit-function-declaration] > > set_sve_default_vl(find_supported_vector_length(vl)); > > ^~~~~~~~~~~~~~~~~~ > > get_sve_default_vl > > Thanks, I'll take a look. I haven't looked in detail at this; I guess the new helpers just need to be manually placed in the right #ifdef block. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-16 16:17 ` Dave Martin @ 2020-06-16 17:19 ` Will Deacon 2020-06-17 9:40 ` Dave Martin 0 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2020-06-16 17:19 UTC (permalink / raw) To: Dave Martin; +Cc: Catalin Marinas, Qian Cai, linux-arm-kernel On Tue, Jun 16, 2020 at 05:17:04PM +0100, Dave Martin wrote: > On Tue, Jun 16, 2020 at 04:04:51PM +0100, Will Deacon wrote: > > On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote: > > > On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote: > > > > sve_default_vl can be modified via the /proc/sys/abi/sve_default_vl > > > > sysctl concurrently with use, and modified concurrently by multiple > > > > threads. > > > > > > > > Adding a lock for this seems overkill, and I don't want to think any > > > > more than necessary, so just define wrappers using READ_ONCE()/ > > > > WRITE_ONCE(). > > > > > > > > This will avoid the possibility of torn accesses and repeated loads > > > > and stores. > > > > > > > > There's no evidence yet that this is going wrong in practice: this > > > > is just hygiene. For generic sysctl users, it would be better to > > > > build this kind of thing into the sysctl common code somehow. > > > > > > > > Reported-by: Will Deacon <will@kernel.org> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > > > While this original patch looks correct, linux-next has this, > > > > > > [will: move set_sve_default_vl() inside #ifdef to squash allnoconfig warning] > > > > > > 1e570f512cbd ("arm64/sve: Eliminate data races on sve_default_vl") > > > > > > which causes an error with CONFIG_ARM64_SVE=n, > > > > > > This .config, > > > https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config > > > > > > arch/arm64/kernel/fpsimd.c: In function ‘sve_proc_do_default_vl’: > > > arch/arm64/kernel/fpsimd.c:375:2: error: implicit declaration of > > > function ‘set_sve_default_vl’; did you mean ‘get_sve_default_vl’? > > > [-Werror=implicit-function-declaration] > > > set_sve_default_vl(find_supported_vector_length(vl)); > > > ^~~~~~~~~~~~~~~~~~ > > > get_sve_default_vl > > > > Thanks, I'll take a look. > > I haven't looked in detail at this; I guess the new helpers just > need to be manually placed in the right #ifdef block. That was what I did when I merged the patch, but that broke configurations where SYSCTL is enabled but SVE is disabled. I've ended up with this diff on top of for-next/fixes. Will --->8 diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index d9eee9194511..55c8f3ec6705 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -349,7 +349,7 @@ static unsigned int find_supported_vector_length(unsigned int vl) return sve_vl_from_vq(__bit_to_vq(bit)); } -#ifdef CONFIG_SYSCTL +#if defined(CONFIG_ARM64_SVE) && defined(CONFIG_SYSCTL) static int sve_proc_do_default_vl(struct ctl_table *table, int write, void *buffer, size_t *lenp, loff_t *ppos) @@ -394,9 +394,9 @@ static int __init sve_sysctl_init(void) return 0; } -#else /* ! CONFIG_SYSCTL */ +#else /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ static int __init sve_sysctl_init(void) { return 0; } -#endif /* ! CONFIG_SYSCTL */ +#endif /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ #define ZREG(sve_state, vq, n) ((char *)(sve_state) + \ (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET)) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-16 17:19 ` Will Deacon @ 2020-06-17 9:40 ` Dave Martin 2020-06-17 10:08 ` Will Deacon 0 siblings, 1 reply; 12+ messages in thread From: Dave Martin @ 2020-06-17 9:40 UTC (permalink / raw) To: Will Deacon; +Cc: Catalin Marinas, Qian Cai, linux-arm-kernel On Tue, Jun 16, 2020 at 06:19:27PM +0100, Will Deacon wrote: > On Tue, Jun 16, 2020 at 05:17:04PM +0100, Dave Martin wrote: > > On Tue, Jun 16, 2020 at 04:04:51PM +0100, Will Deacon wrote: > > > On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote: > > > > On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote: > > > > > sve_default_vl can be modified via the /proc/sys/abi/sve_default_vl > > > > > sysctl concurrently with use, and modified concurrently by multiple > > > > > threads. > > > > > > > > > > Adding a lock for this seems overkill, and I don't want to think any > > > > > more than necessary, so just define wrappers using READ_ONCE()/ > > > > > WRITE_ONCE(). > > > > > > > > > > This will avoid the possibility of torn accesses and repeated loads > > > > > and stores. > > > > > > > > > > There's no evidence yet that this is going wrong in practice: this > > > > > is just hygiene. For generic sysctl users, it would be better to > > > > > build this kind of thing into the sysctl common code somehow. > > > > > > > > > > Reported-by: Will Deacon <will@kernel.org> > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > > > > > While this original patch looks correct, linux-next has this, > > > > > > > > [will: move set_sve_default_vl() inside #ifdef to squash allnoconfig warning] > > > > > > > > 1e570f512cbd ("arm64/sve: Eliminate data races on sve_default_vl") > > > > > > > > which causes an error with CONFIG_ARM64_SVE=n, > > > > > > > > This .config, > > > > https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config > > > > > > > > arch/arm64/kernel/fpsimd.c: In function ‘sve_proc_do_default_vl’: > > > > arch/arm64/kernel/fpsimd.c:375:2: error: implicit declaration of > > > > function ‘set_sve_default_vl’; did you mean ‘get_sve_default_vl’? > > > > [-Werror=implicit-function-declaration] > > > > set_sve_default_vl(find_supported_vector_length(vl)); > > > > ^~~~~~~~~~~~~~~~~~ > > > > get_sve_default_vl > > > > > > Thanks, I'll take a look. > > > > I haven't looked in detail at this; I guess the new helpers just > > need to be manually placed in the right #ifdef block. > > That was what I did when I merged the patch, but that broke configurations > where SYSCTL is enabled but SVE is disabled. I've ended up with this > diff on top of for-next/fixes. > > Will > > --->8 > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index d9eee9194511..55c8f3ec6705 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -349,7 +349,7 @@ static unsigned int find_supported_vector_length(unsigned int vl) > return sve_vl_from_vq(__bit_to_vq(bit)); > } > > -#ifdef CONFIG_SYSCTL > +#if defined(CONFIG_ARM64_SVE) && defined(CONFIG_SYSCTL) > > static int sve_proc_do_default_vl(struct ctl_table *table, int write, > void *buffer, size_t *lenp, loff_t *ppos) > @@ -394,9 +394,9 @@ static int __init sve_sysctl_init(void) > return 0; > } > > -#else /* ! CONFIG_SYSCTL */ > +#else /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ > static int __init sve_sysctl_init(void) { return 0; } > -#endif /* ! CONFIG_SYSCTL */ > +#endif /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ Hmm, I guess that works, but it still seems cumbersome. #ifdefs do tend to breed as the code gets extended, so I'd worked hard to eliminate them as much as possible. Can't we simply leave the helpers outside the #ifdef, and do this? /* Default VL for tasks that don't set it explicitly: */ static int __sve_default_vl = -1; -static int get_sve_default_vl(void) +static inline int get_sve_default_vl(void) { return READ_ONCE(__sve_default_vl); } -static void set_sve_default_vl(int val) +static inline void set_sve_default_vl(int val) { WRITE_ONCE(__sve_default_vl, val); } Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-17 9:40 ` Dave Martin @ 2020-06-17 10:08 ` Will Deacon 2020-06-17 11:06 ` Dave Martin 0 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2020-06-17 10:08 UTC (permalink / raw) To: Dave Martin; +Cc: Catalin Marinas, Qian Cai, linux-arm-kernel On Wed, Jun 17, 2020 at 10:40:54AM +0100, Dave Martin wrote: > On Tue, Jun 16, 2020 at 06:19:27PM +0100, Will Deacon wrote: > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > index d9eee9194511..55c8f3ec6705 100644 > > --- a/arch/arm64/kernel/fpsimd.c > > +++ b/arch/arm64/kernel/fpsimd.c > > @@ -349,7 +349,7 @@ static unsigned int find_supported_vector_length(unsigned int vl) > > return sve_vl_from_vq(__bit_to_vq(bit)); > > } > > > > -#ifdef CONFIG_SYSCTL > > +#if defined(CONFIG_ARM64_SVE) && defined(CONFIG_SYSCTL) > > > > static int sve_proc_do_default_vl(struct ctl_table *table, int write, > > void *buffer, size_t *lenp, loff_t *ppos) > > @@ -394,9 +394,9 @@ static int __init sve_sysctl_init(void) > > return 0; > > } > > > > -#else /* ! CONFIG_SYSCTL */ > > +#else /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ > > static int __init sve_sysctl_init(void) { return 0; } > > -#endif /* ! CONFIG_SYSCTL */ > > +#endif /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ > > Hmm, I guess that works, but it still seems cumbersome. #ifdefs do > tend to breed as the code gets extended, so I'd worked hard to > eliminate them as much as possible. This is just extending an existing #ifdef though, and I don't think it makes any sense to compile in the SVE sysctl logic if SVE is not enabled. If CONFIG_SYSCTL didn't exist, this code would almost certainly be inside a CONFIG_SVE block anyway. > Can't we simply leave the helpers outside the #ifdef, and do this? > > /* Default VL for tasks that don't set it explicitly: */ > static int __sve_default_vl = -1; > > -static int get_sve_default_vl(void) > +static inline int get_sve_default_vl(void) > { > return READ_ONCE(__sve_default_vl); > } > > -static void set_sve_default_vl(int val) > +static inline void set_sve_default_vl(int val) > { > WRITE_ONCE(__sve_default_vl, val); > } That would work too, although I'd be wary of somebody removing the inline later on because "the compiler knows best about inlining decisions". I'd also say that calling set_sve_default_vl() is an error if CONFIG_SVE is not defined as we really want get_sve_default_vl() to return -1 unconditionally in that case. Having set_sve_default_vl() inside the #ifdef ensures that. I don't care too strongly either way, but I already queued my diff last night [1] in order to fix linux-next, so I'd prefer not to drop it unless there's a functional reason to do so. Will [1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?h=for-next/fixes&id=e575fb9e76c8e33440fb859572a8b7d430f053d6 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl 2020-06-17 10:08 ` Will Deacon @ 2020-06-17 11:06 ` Dave Martin 0 siblings, 0 replies; 12+ messages in thread From: Dave Martin @ 2020-06-17 11:06 UTC (permalink / raw) To: Will Deacon; +Cc: Catalin Marinas, Qian Cai, linux-arm-kernel On Wed, Jun 17, 2020 at 11:08:32AM +0100, Will Deacon wrote: > On Wed, Jun 17, 2020 at 10:40:54AM +0100, Dave Martin wrote: > > On Tue, Jun 16, 2020 at 06:19:27PM +0100, Will Deacon wrote: > > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > > index d9eee9194511..55c8f3ec6705 100644 > > > --- a/arch/arm64/kernel/fpsimd.c > > > +++ b/arch/arm64/kernel/fpsimd.c > > > @@ -349,7 +349,7 @@ static unsigned int find_supported_vector_length(unsigned int vl) > > > return sve_vl_from_vq(__bit_to_vq(bit)); > > > } > > > > > > -#ifdef CONFIG_SYSCTL > > > +#if defined(CONFIG_ARM64_SVE) && defined(CONFIG_SYSCTL) > > > > > > static int sve_proc_do_default_vl(struct ctl_table *table, int write, > > > void *buffer, size_t *lenp, loff_t *ppos) > > > @@ -394,9 +394,9 @@ static int __init sve_sysctl_init(void) > > > return 0; > > > } > > > > > > -#else /* ! CONFIG_SYSCTL */ > > > +#else /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ > > > static int __init sve_sysctl_init(void) { return 0; } > > > -#endif /* ! CONFIG_SYSCTL */ > > > +#endif /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */ > > > > Hmm, I guess that works, but it still seems cumbersome. #ifdefs do > > tend to breed as the code gets extended, so I'd worked hard to > > eliminate them as much as possible. > > This is just extending an existing #ifdef though, and I don't think it > makes any sense to compile in the SVE sysctl logic if SVE is not enabled. > If CONFIG_SYSCTL didn't exist, this code would almost certainly be inside > a CONFIG_SVE block anyway. Only code that's unreachable from inside the translation unit needs to be #ifdeffed. For the rest, the compiler knows how to determine what's used (indeed, it's better at it than humans). Originally I relied on #ifdefs more, but I needed a lot of them, and it was hell to rebase every time anything needed to be moved around. Currently I don't see anything that gets compiled in if CONFIG_SYSCTL=n. Other than what was already compiled before this patch. We still need to track the default vl, because it depends on the hardware; however it's effectively ro-after-init if CONFIG_SYSCTL=n. I think that complicating the #ifdef conditions in this file is a slippery slope, but I guess it's the it's up to the maintainer whether to care about that. Am I missing something? > > Can't we simply leave the helpers outside the #ifdef, and do this? > > > > /* Default VL for tasks that don't set it explicitly: */ > > static int __sve_default_vl = -1; > > > > -static int get_sve_default_vl(void) > > +static inline int get_sve_default_vl(void) > > { > > return READ_ONCE(__sve_default_vl); > > } > > > > -static void set_sve_default_vl(int val) > > +static inline void set_sve_default_vl(int val) > > { > > WRITE_ONCE(__sve_default_vl, val); > > } > > That would work too, although I'd be wary of somebody removing the inline > later on because "the compiler knows best about inlining decisions". I'd AFAIK inline is widely used for static functions in headers for precisely this reason. I have tried to use __maybe_unused (or even #ifdefs) in the past to be more explicit, but got shouted at. We could optionally use __maybe_unused here if you think that's more self-explanatory. > also say that calling set_sve_default_vl() is an error if CONFIG_SVE is > not defined as we really want get_sve_default_vl() to return -1 > unconditionally in that case. Having set_sve_default_vl() inside the > #ifdef ensures that. Fair point, I'm not sure how valuable it is. We manage without it thus far: prior to these changes, the sve_default_vl variable was not #ifdeffed. > I don't care too strongly either way, but I already queued my diff last > night [1] in order to fix linux-next, so I'd prefer not to drop it unless > there's a functional reason to do so. Ack, since this change would be purely to ease future maintanence (or not, if you judge it's not useful), it's not urgent. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] arm64/sve: Misc fixes 2020-06-10 17:03 [PATCH 0/2] arm64/sve: Misc fixes Dave Martin 2020-06-10 17:03 ` [PATCH 1/2] docs/arm64: Fix typo'd #define in sve.rst Dave Martin 2020-06-10 17:03 ` [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl Dave Martin @ 2020-06-15 16:34 ` Will Deacon 2020-06-16 9:56 ` Dave Martin 2 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2020-06-15 16:34 UTC (permalink / raw) To: linux-arm-kernel, Dave Martin; +Cc: catalin.marinas, Will Deacon On Wed, 10 Jun 2020 18:03:08 +0100, Dave Martin wrote: > A couple of unrelated minor fixes: one documentation typo fix, and one > fix for a (mostly) theoretical data race. > > Dave Martin (2): > docs/arm64: Fix typo'd #define in sve.rst > arm64/sve: Eliminate data races on sve_default_vl > > [...] Applied to arm64 (for-next/fixes), thanks! [1/2] docs/arm64: Fix typo'd #define in sve.rst https://git.kernel.org/arm64/c/9ba6a9efa4a4 [2/2] arm64/sve: Eliminate data races on sve_default_vl https://git.kernel.org/arm64/c/1e570f512cbd Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] arm64/sve: Misc fixes 2020-06-15 16:34 ` [PATCH 0/2] arm64/sve: Misc fixes Will Deacon @ 2020-06-16 9:56 ` Dave Martin 0 siblings, 0 replies; 12+ messages in thread From: Dave Martin @ 2020-06-16 9:56 UTC (permalink / raw) To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel On Mon, Jun 15, 2020 at 05:34:01PM +0100, Will Deacon wrote: > On Wed, 10 Jun 2020 18:03:08 +0100, Dave Martin wrote: > > A couple of unrelated minor fixes: one documentation typo fix, and one > > fix for a (mostly) theoretical data race. > > > > Dave Martin (2): > > docs/arm64: Fix typo'd #define in sve.rst > > arm64/sve: Eliminate data races on sve_default_vl > > > > [...] > > Applied to arm64 (for-next/fixes), thanks! > > [1/2] docs/arm64: Fix typo'd #define in sve.rst > https://git.kernel.org/arm64/c/9ba6a9efa4a4 > [2/2] arm64/sve: Eliminate data races on sve_default_vl > https://git.kernel.org/arm64/c/1e570f512cbd Thanks ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-17 11:06 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-10 17:03 [PATCH 0/2] arm64/sve: Misc fixes Dave Martin 2020-06-10 17:03 ` [PATCH 1/2] docs/arm64: Fix typo'd #define in sve.rst Dave Martin 2020-06-10 17:03 ` [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl Dave Martin 2020-06-16 13:18 ` Qian Cai 2020-06-16 15:04 ` Will Deacon 2020-06-16 16:17 ` Dave Martin 2020-06-16 17:19 ` Will Deacon 2020-06-17 9:40 ` Dave Martin 2020-06-17 10:08 ` Will Deacon 2020-06-17 11:06 ` Dave Martin 2020-06-15 16:34 ` [PATCH 0/2] arm64/sve: Misc fixes Will Deacon 2020-06-16 9:56 ` Dave Martin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).