From: Dave Martin <Dave.Martin@arm.com> Date: 2020-06-10 17:03:19
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
From: Dave Martin <Dave.Martin@arm.com> Date: 2020-06-10 17:04:02
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(-)
@@ -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
From: Dave Martin <Dave.Martin@arm.com> Date: 2020-06-10 17:04:08
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(-)
@@ -119,7 +120,17 @@ struct fpsimd_last_state_struct {staticDEFINE_PER_CPU(structfpsimd_last_state_struct,fpsimd_last_state);/* Default VL for tasks that don't set it explicitly: */-staticintsve_default_vl=-1;+staticint__sve_default_vl=-1;++staticintget_sve_default_vl(void)+{+returnREAD_ONCE(__sve_default_vl);+}++staticvoidset_sve_default_vl(intval)+{+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){intret;-intvl=sve_default_vl;+intvl=get_sve_default_vl();structctl_tabletmp_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));return0;}
@@ -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)
From: Will Deacon <will@kernel.org> Date: 2020-06-15 16:34:32
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
[...]
From: Dave Martin <Dave.Martin@arm.com> Date: 2020-06-16 09:57:03
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:
quoted
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
[...]
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
quoted hunk
---
Build-tested only, but it seems pretty straightforward.
arch/arm64/kernel/fpsimd.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
@@ -119,7 +120,17 @@ struct fpsimd_last_state_struct {staticDEFINE_PER_CPU(structfpsimd_last_state_struct,fpsimd_last_state);/* Default VL for tasks that don't set it explicitly: */-staticintsve_default_vl=-1;+staticint__sve_default_vl=-1;++staticintget_sve_default_vl(void)+{+returnREAD_ONCE(__sve_default_vl);+}++staticvoidset_sve_default_vl(intval)+{+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){intret;-intvl=sve_default_vl;+intvl=get_sve_default_vl();structctl_tabletmp_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));return0;}
@@ -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)
From: Will Deacon <will@kernel.org> Date: 2020-06-16 15:05:02
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:
quoted
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
From: Dave Martin <Dave.Martin@arm.com> Date: 2020-06-16 16:17:13
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:
quoted
On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote:
quoted
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
From: Will Deacon <will@kernel.org> Date: 2020-06-16 17:19:37
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:
quoted
On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote:
quoted
On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote:
quoted
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
From: Dave Martin <Dave.Martin@arm.com> Date: 2020-06-17 09:41:02
On Tue, Jun 16, 2020 at 06:19:27PM +0100, Will Deacon wrote:
quoted hunk
On Tue, Jun 16, 2020 at 05:17:04PM +0100, Dave Martin wrote:
quoted
On Tue, Jun 16, 2020 at 04:04:51PM +0100, Will Deacon wrote:
quoted
On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote:
quoted
On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote:
quoted
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
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
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
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?
quoted
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