may_use_simd on aarch64, chacha20

may_use_simd on aarch64, chacha20

[Jason A. Donenfeld]

Hi folks,

I noticed that the ARM implementation [1] of chacha20 makes a check to
may_use_simd(), but the ARM64 implementation [2] does not. Question 1:
is this a bug, in which case I'll submit a patch shortly, or is this
intentional? In case of the latter, could somebody explain the
reasoning? On a similar note, the only ARM64 glue code that uses
may_use_simd() is sha256; everything else does not. Shall I submit a
substantial patch series to fix this up everywhere?

Secondly, I noticed that may_use_simd() is essentially aliased to
!in_interrupt(), since it uses the asm-generic variety. Question 2:
Isn't this overkill? Couldn't we make an arm/arm64 variant of this
that only checks in_irq()?

Lastly, APIs like pcrypts and padata execute with bottom halves
disabled, even though their actual execution environment is process
context, via a workqueue. Thus, here, in_interrupt() will always be
true, even though this is likely a place where we want to use simd.
Question 3: is there something better that could be done?

Thanks,
Jason

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/crypto/chacha20-neon-glue.c#n67
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/crypto/chacha20-neon-glue.c#n66

may_use_simd on aarch64, chacha20

[Ard Biesheuvel]

(+ Dave)

> On 21 May 2017, at 19:02, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> Hi folks,
> 
> I noticed that the ARM implementation [1] of chacha20 makes a check to
> may_use_simd(), but the ARM64 implementation [2] does not. Question 1:
> is this a bug, in which case I'll submit a patch shortly, or is this
> intentional? In case of the latter, could somebody explain the
> reasoning?

This is intentional. arm64 supports kernel mode NEON in any context, whereas ARM only supports it in process context. This is due to the way lazy FP restore is implemented on ARM. However, we are about to
change this on arm64 to only allow non-nested kernel mode NEON, similar to x86. This is necessary to support SVE.

> On a similar note, the only ARM64 glue code that uses
> may_use_simd() is sha256; everything else does not. Shall I submit a
> substantial patch series to fix this up everywhere?
> 

Currently, may_use_simd() is only used as a hint on arm64 whether it makes sense to offload crypto to process context. In the sha256
code, whose arm64 neon implementation is only marginally faster than scalar on some micro-architectures, it is used to prefer the scalar code in interrupt context, because the NEON code preserves/restores the NEON state of the interrupted context eagerly, which is costly.

> Secondly, I noticed that may_use_simd() is essentially aliased to
> !in_interrupt(), since it uses the asm-generic variety. Question 2:
> Isn't this overkill? Couldn't we make an arm/arm64 variant of this
> that only checks in_irq()?
> 

No. ARM does not support kernel mode NEON in softirq
context, and arm64 will soon have its own override that only allows non-nested use in softirq context.

> Lastly, APIs like pcrypts and padata execute with bottom halves
> disabled, even though their actual execution environment is process
> context, via a workqueue. Thus, here, in_interrupt() will always be
> true, even though this is likely a place where we want to use simd.
> Question 3: is there something better that could be done?

I guess we should switch to in_serving_softirq() instead.

Thanks,
Ard.

may_use_simd on aarch64, chacha20

[Dave Martin]

On Sun, May 21, 2017 at 10:55:20PM +0200, Ard Biesheuvel wrote:
> (+ Dave)

Apologies for the slow reply -- hopefully this is still useful.

> > On 21 May 2017, at 19:02, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > 
> > Hi folks,
> > 
> > I noticed that the ARM implementation [1] of chacha20 makes a check to
> > may_use_simd(), but the ARM64 implementation [2] does not. Question 1:
> > is this a bug, in which case I'll submit a patch shortly, or is this
> > intentional? In case of the latter, could somebody explain the
> > reasoning?
> 
> This is intentional. arm64 supports kernel mode NEON in any context,
> whereas ARM only supports it in process context. This is due to the way
> lazy FP restore is implemented on ARM. However, we are about to change
> this on arm64 to only allow non-nested kernel mode NEON, similar to
> x86. This is necessary to support SVE.
> 
> > On a similar note, the only ARM64 glue code that uses
> > may_use_simd() is sha256; everything else does not. Shall I submit a
> > substantial patch series to fix this up everywhere?
> > 
> 
> Currently, may_use_simd() is only used as a hint on arm64 whether it
> makes sense to offload crypto to process context. In the sha256 code,
> whose arm64 neon implementation is only marginally faster than scalar
> on some micro-architectures, it is used to prefer the scalar code in
> interrupt context, because the NEON code preserves/restores the NEON
> state of the interrupted context eagerly, which is costly.
> 
> > Secondly, I noticed that may_use_simd() is essentially aliased to
> > !in_interrupt(), since it uses the asm-generic variety. Question 2:
> > Isn't this overkill? Couldn't we make an arm/arm64 variant of this
> > that only checks in_irq()?
> > 
> 
> No. ARM does not support kernel mode NEON in softirq context, and
> arm64 will soon have its own override that only allows non-nested use
> in softirq context.
> 
> > Lastly, APIs like pcrypts and padata execute with bottom halves
> > disabled, even though their actual execution environment is process
> > context, via a workqueue. Thus, here, in_interrupt() will always be
> > true, even though this is likely a place where we want to use simd.
> > Question 3: is there something better that could be done?
> 
> I guess we should switch to in_serving_softirq() instead.

For context, the arm64 kernel-mode NEON refactoring I've been working
on [1] defines may_use_simd() to indicate whether calling
kernel_neon_begin() is safe or not, rather then being a hint about
whether it is desirable to do so.

This changes the definition of may_use_simd() to something that is
probably appropriate for your scenario.  From process context without
any outer kernel_neon_begin()...kernel_neon_end(), it should return
true.

In general, callers in any context should do something like:

	if (may_use_simd()) {
		kernel_neon_begin();
		/* SIMD implementation */
		kernel_neon_end();
	} else {
		/* fallback implementation, or defer to another context */
	}

Even from task context where may_use_simd() should always return true
and thus where the fallback path can be omitted, it may be a good idea
to do something like

	if (!may_use_simd()) {
		WARN_ON(1);
		return -EBUSY;
	}

... or similar.


If the semantics or name don't feel right, I'm happy to change them.

Cheers
---Dave


[1] lists.infradead.org/pipermail/linux-arm-kernel/2017-May/508901.html
(arm64: neon: Remove support for nested or hardirq kernel-mode NEON)

may_use_simd on aarch64, chacha20

[Ard Biesheuvel]

On 26 May 2017 at 15:28, Dave Martin <Dave.Martin@arm.com> wrote:
> On Sun, May 21, 2017 at 10:55:20PM +0200, Ard Biesheuvel wrote:
>> (+ Dave)
>
> Apologies for the slow reply -- hopefully this is still useful.
>
>> > On 21 May 2017, at 19:02, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> >
>> > Hi folks,
>> >
>> > I noticed that the ARM implementation [1] of chacha20 makes a check to
>> > may_use_simd(), but the ARM64 implementation [2] does not. Question 1:
>> > is this a bug, in which case I'll submit a patch shortly, or is this
>> > intentional? In case of the latter, could somebody explain the
>> > reasoning?
>>
>> This is intentional. arm64 supports kernel mode NEON in any context,
>> whereas ARM only supports it in process context. This is due to the way
>> lazy FP restore is implemented on ARM. However, we are about to change
>> this on arm64 to only allow non-nested kernel mode NEON, similar to
>> x86. This is necessary to support SVE.
>>
>> > On a similar note, the only ARM64 glue code that uses
>> > may_use_simd() is sha256; everything else does not. Shall I submit a
>> > substantial patch series to fix this up everywhere?
>> >
>>
>> Currently, may_use_simd() is only used as a hint on arm64 whether it
>> makes sense to offload crypto to process context. In the sha256 code,
>> whose arm64 neon implementation is only marginally faster than scalar
>> on some micro-architectures, it is used to prefer the scalar code in
>> interrupt context, because the NEON code preserves/restores the NEON
>> state of the interrupted context eagerly, which is costly.
>>
>> > Secondly, I noticed that may_use_simd() is essentially aliased to
>> > !in_interrupt(), since it uses the asm-generic variety. Question 2:
>> > Isn't this overkill? Couldn't we make an arm/arm64 variant of this
>> > that only checks in_irq()?
>> >
>>
>> No. ARM does not support kernel mode NEON in softirq context, and
>> arm64 will soon have its own override that only allows non-nested use
>> in softirq context.
>>
>> > Lastly, APIs like pcrypts and padata execute with bottom halves
>> > disabled, even though their actual execution environment is process
>> > context, via a workqueue. Thus, here, in_interrupt() will always be
>> > true, even though this is likely a place where we want to use simd.
>> > Question 3: is there something better that could be done?
>>
>> I guess we should switch to in_serving_softirq() instead.
>
> For context, the arm64 kernel-mode NEON refactoring I've been working
> on [1] defines may_use_simd() to indicate whether calling
> kernel_neon_begin() is safe or not, rather then being a hint about
> whether it is desirable to do so.
>

I should mention again, perhaps redundantly, that may_use_simd() was
not intended as a hint. It only became this way after we removed the
limitation on arm64 that SIMD may only be used in process context.

> This changes the definition of may_use_simd() to something that is
> probably appropriate for your scenario.  From process context without
> any outer kernel_neon_begin()...kernel_neon_end(), it should return
> true.
>
> In general, callers in any context should do something like:
>
>         if (may_use_simd()) {
>                 kernel_neon_begin();
>                 /* SIMD implementation */
>                 kernel_neon_end();
>         } else {
>                 /* fallback implementation, or defer to another context */
>         }
>
> Even from task context where may_use_simd() should always return true
> and thus where the fallback path can be omitted, it may be a good idea
> to do something like
>
>         if (!may_use_simd()) {
>                 WARN_ON(1);
>                 return -EBUSY;
>         }
>
> ... or similar.
>

I don't think it is generally a good idea to make inferences about
whether may_use_simd() is going to return true or false in code that
is not tightly coupled to the FP/SIMD arch code itself, although I do
understand that there may be cases where it is cumbersome to provide a
fallback, especially if you know it is never going to be used.

may_use_simd on aarch64, chacha20

[Dave Martin]

On Fri, May 26, 2017 at 07:44:46PM +0200, Ard Biesheuvel wrote:
> On 26 May 2017 at 15:28, Dave Martin <Dave.Martin@arm.com> wrote:
> > On Sun, May 21, 2017 at 10:55:20PM +0200, Ard Biesheuvel wrote:
> >> (+ Dave)

[...]

> >> > Lastly, APIs like pcrypts and padata execute with bottom halves
> >> > disabled, even though their actual execution environment is process
> >> > context, via a workqueue. Thus, here, in_interrupt() will always be
> >> > true, even though this is likely a place where we want to use simd.
> >> > Question 3: is there something better that could be done?
> >>
> >> I guess we should switch to in_serving_softirq() instead.
> >
> > For context, the arm64 kernel-mode NEON refactoring I've been working
> > on [1] defines may_use_simd() to indicate whether calling
> > kernel_neon_begin() is safe or not, rather then being a hint about
> > whether it is desirable to do so.
> >
> 
> I should mention again, perhaps redundantly, that may_use_simd() was
> not intended as a hint. It only became this way after we removed the
> limitation on arm64 that SIMD may only be used in process context.

My current may_use_simd() is intended to have those non-hint semantics
-- however, it feels a bit odd to have to include an extra header just
for that, when clients of kernel-mode NEON are necessarily not portable.

I don't have a very strong feeling either way, though.

> > This changes the definition of may_use_simd() to something that is
> > probably appropriate for your scenario.  From process context without
> > any outer kernel_neon_begin()...kernel_neon_end(), it should return
> > true.
> >
> > In general, callers in any context should do something like:
> >
> >         if (may_use_simd()) {
> >                 kernel_neon_begin();
> >                 /* SIMD implementation */
> >                 kernel_neon_end();
> >         } else {
> >                 /* fallback implementation, or defer to another context */
> >         }
> >
> > Even from task context where may_use_simd() should always return true
> > and thus where the fallback path can be omitted, it may be a good idea
> > to do something like
> >
> >         if (!may_use_simd()) {
> >                 WARN_ON(1);
> >                 return -EBUSY;
> >         }
> >
> > ... or similar.
> >
> 
> I don't think it is generally a good idea to make inferences about
> whether may_use_simd() is going to return true or false in code that
> is not tightly coupled to the FP/SIMD arch code itself, although I do
> understand that there may be cases where it is cumbersome to provide a
> fallback, especially if you know it is never going to be used.

Agreed.  I don't think we should encourage this, but I thought it worth
illustrating.  Hopefully there are few if any real situations where this
would be justified.

Cheers
---Dave