Messages in this thread | | | Subject | Re: [PATCH v4 3/3] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state | From | Julien Grall <> | Date | Tue, 7 May 2019 11:52:18 +0100 |
| |
Hi Dave,
On 4/26/19 4:31 PM, Dave Martin wrote: > 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.)
Thank you! I will resend the series once rc1 has been cut.
Cheers,
-- Julien Grall
| |