Re: [PATCH v4 3/3] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state

[Dave Martin <Dave.Martin@arm.com>]

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