Messages in this thread | | | Date | Fri, 26 Apr 2019 16:31:14 +0100 | From | Dave Martin <> | Subject | Re: [PATCH v4 3/3] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state |
| |
On Fri, Apr 26, 2019 at 04:06:02PM +0100, Julien Grall wrote: > Hi, > > On 26/04/2019 15:52, Dave Martin wrote: > >On Fri, Apr 26, 2019 at 03:37:40PM +0100, Julien Grall wrote: > >>When the kernel is compiled with CONFIG_KERNEL_MODE_NEON, some part of > >>the kernel may be able to use FPSIMD/SVE. This is for instance the case > >>for crypto code. > >> > >>Any use of FPSIMD/SVE in the kernel are clearly marked by using the > >>function kernel_neon_{begin, end}. Furthermore, this can only be used > >>when may_use_simd() returns true. > >> > >>The current implementation of may_use_simd() allows softirq to use > >>FPSIMD/SVE unless it is currently in use (i.e kernel_neon_busy is true). > >>When in use, softirqs usually fall back to a software method. > >> > >>At the moment, as a softirq may use FPSIMD/SVE, softirqs are disabled > >>when touching the FPSIMD/SVE context. This has the drawback to disable > >>all softirqs even if they are not using FPSIMD/SVE. > >> > >>Since a softirq is supposed to check may_use_simd() anyway before > >>attempting to use FPSIMD/SVE, there is limited reason to keep softirq > >>disabled when touching the FPSIMD/SVE context. Instead, we can simply > >>disable preemption and mark the FPSIMD/SVE context as in use by setting > >>CPU's kernel_neon_busy flag. > > > >fpsimd_context_busy? > > Yes. > > > > >>Two new helpers {get, put}_cpu_fpsimd_context is introduced to mark the > >>area using FPSIMD/SVE context and uses them in replacement of > > > >Paragraph mangled during edit? > > Possibly, I will update it. > > > > >-> "are introduced ... and they are used to replace ..." > > > >>local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are > >>also re-implemented to use the new helpers. > >> > >>Additionally, double-underscored versions of the helpers are provided to > >>be used in function called with interrupt masked. They are used for > >>sanity and also help to mark place where the FPSIMD context can be > >>manipulate freely. > > > >For the benefit of other readers, this should be more explicit. Also, > >the distinction between the normal and __ helpers is that the latter > >can be caller with preemption disabled. > > > >To clarify the impact, we can say something like > > > >"These are only relevant on paths where irqs are disabled anyway, so > >they are not needed for correctness in the current code. Let's use them > >anyway though: this marks the critical sections clearly and will help > >to avoid mistakes during future maintenance." > > How about the following commit message? > > arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state > > When the kernel is compiled with CONFIG_KERNEL_MODE_NEON, some part of > the kernel may be able to use FPSIMD/SVE. This is for instance the case > for crypto code. > > Any use of FPSIMD/SVE in the kernel are clearly marked by using the > function kernel_neon_{begin, end}. Furthermore, this can only be used > when may_use_simd() returns true. > > The current implementation of may_use_simd() allows softirq to use > FPSIMD/SVE unless it is currently in use (i.e kernel_neon_busy is true). > When in use, softirqs usually fall back to a software method. > > At the moment, as a softirq may use FPSIMD/SVE, softirqs are disabled > when touching the FPSIMD/SVE context. This has the drawback to disable > all softirqs even if they are not using FPSIMD/SVE. > > Since a softirq is supposed to check may_use_simd() anyway before > attempting to use FPSIMD/SVE, there is limited reason to keep softirq > disabled when touching the FPSIMD/SVE context. Instead, we can simply > disable preemption and mark the FPSIMD/SVE context as in use by setting > CPU's fpsimd_context_busy flag. > > Two new helpers {get, put}_cpu_fpsimd_context are introduced to mark > the area using FPSIMD/SVE context and they are used to replace > local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are > also re-implemented to use the new helpers. > > Additionally, double-underscored versions of the helpers are provided to > called when preemption is already disabled. These are only relevant on > paths where irqs are disabled anyway, so they are not needed for > correctness in the current code. Let's use them anyway though: this > marks critical sections clearly and will help to avoid mistakes during > future maintenance.
Looks good to me.
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
(For the diff as well as the commit message, obviously.)
Cheers ---Dave
| |