* [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 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
* 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
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).