From: Julien Grall <julien.grall@arm.com>
To: Dave Martin <Dave.Martin@arm.com>
Cc: linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, ard.biesheuvel@linaro.org,
julien.thierry@arm.com, marc.zyngier@arm.com,
catalin.marinas@arm.com, suzuki.poulose@arm.com,
will.deacon@arm.com, christoffer.dall@arm.com,
james.morse@arm.com
Subject: Re: [PATCH v4 3/3] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state
Date: Fri, 26 Apr 2019 16:06:02 +0100 [thread overview]
Message-ID: <322340c7-0c97-76f8-8ab8-875040b4459c@arm.com> (raw)
In-Reply-To: <20190426145232.GK3567@e103592.cambridge.arm.com>
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.
The change has been benchmarked on Linux 5.1-rc4 with defconfig.
On Juno2:
* hackbench 100 process 1000 (10 times)
* .7% quicker
On ThunderX 2:
* hackbench 1000 process 1000 (20 times)
* 3.4% quicker
>
> [...]
>
> (Sorry to nitpick)
That's fine, I should have be more careful when rework the commit message.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2019-04-26 15:06 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-26 14:37 [PATCH v4 0/3] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state Julien Grall
2019-04-26 14:37 ` [PATCH v4 1/3] arm64/fpsimd: Remove the prototype for sve_flush_cpu_state() Julien Grall
2019-04-26 14:37 ` [PATCH v4 2/3] arch/arm64: fpsimd: Introduce fpsimd_save_and_flush_cpu_state() and use it Julien Grall
2019-04-26 14:37 ` [PATCH v4 3/3] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state Julien Grall
2019-04-26 14:52 ` Dave Martin
2019-04-26 15:06 ` Julien Grall [this message]
2019-04-26 15:31 ` Dave Martin
2019-05-07 10:52 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=322340c7-0c97-76f8-8ab8-875040b4459c@arm.com \
--to=julien.grall@arm.com \
--cc=Dave.Martin@arm.com \
--cc=ard.biesheuvel@linaro.org \
--cc=catalin.marinas@arm.com \
--cc=christoffer.dall@arm.com \
--cc=james.morse@arm.com \
--cc=julien.thierry@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=suzuki.poulose@arm.com \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).