| Message ID | 20190208165513.8435-1-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state | expand |
Hello Julien- On Fri, Feb 08, 2019 at 04:55:13PM +0000, 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 used (i.e kernel_neon_busy is true). > When in used, softirqs usually fallback 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. > > As a softirq should not rely on been able to use simd at a given time, > there are limited reason to keep softirq disabled when touching the > FPSIMD/SVE context. Instead, we can only disable preemption and tell > the NEON unit is currently in use. > > This patch introduces two new helpers kernel_neon_{disable, enable} to > mark the area using FPSIMD/SVE context and use them in replacement of > local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are > also re-implemented to use the new helpers. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > I have been exploring this solution as an alternative approach to the RT > patch "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()". > > So far, the patch has only been lightly tested. > > For RT-linux, it might be possible to use migrate_{enable, disable}. I > am quite new with RT and have some trouble to understand the semantics > of migrate_{enable, disable}. So far, I am still unsure if it is possible > to run another userspace task on the same CPU while getting preempted > when the migration is disabled. Yes, a thread in a migrate_disable()-protected critical section can be preempted, and another thread on the same CPU may enter the critical section. If it's necessary to prevent this from happening, then you need to also make use of a per-CPU mutex. On RT, this would do the right thing w.r.t. priority inheritance. Julia
On 2019-02-08 16:55:13 [+0000], 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. This is equal what x86 is currently doing. The naming is slightly different, there is irq_fpu_usable(). > The current implementation of may_use_simd() allows softirq to use > FPSIMD/SVE unless it is currently in used (i.e kernel_neon_busy is true). > When in used, softirqs usually fallback 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. Is this bad? This means also that your crypto code will not be interrupted by a softirq. Also if you would get rid of it, you could avoid the software fallback in case may_use_simd() says false. > As a softirq should not rely on been able to use simd at a given time, > there are limited reason to keep softirq disabled when touching the > FPSIMD/SVE context. Instead, we can only disable preemption and tell > the NEON unit is currently in use. > > This patch introduces two new helpers kernel_neon_{disable, enable} to > mark the area using FPSIMD/SVE context and use them in replacement of > local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are > also re-implemented to use the new helpers. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > I have been exploring this solution as an alternative approach to the RT > patch "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()". > > So far, the patch has only been lightly tested. > > For RT-linux, it might be possible to use migrate_{enable, disable}. I > am quite new with RT and have some trouble to understand the semantics > of migrate_{enable, disable}. So far, I am still unsure if it is possible > to run another userspace task on the same CPU while getting preempted > when the migration is disabled. In RT: - preemt_disable() is the same as !RT. A thread can not be suspend. An interrupt may interrupt. However on RT we have threaded interrupts so the interrupt is limited to the first-level handler (not the threaded handler). - migrate_disable() means that the thread can not be moved to another CPU. It can be suspended. - local_bh_disable() disables the BH: No softirq can run. In RT local_bh_disable() does not inherit preempt_disable(). Two different softirqs can be executed in parallel. The BH is usually invoked at the end of the threaded interrupt (because the threaded interrupt handler raises the softirq). It can also run in the ksoftirqd. Usually you should not get preempted in a migrate_disable() section. A SCHED_OTHER task should not interrupt another SCHED_OTHER task in a migrate_disable() section. A task with a higher priority (a RT/DL task) will. Since threaded interrupts run with a RT priority of 50, they will interrupt your task in a migrate_disable() section. Sebastian
On Wed, Feb 13, 2019 at 03:30:29PM +0100, Sebastian Andrzej Siewior wrote: > On 2019-02-08 16:55:13 [+0000], 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. > > This is equal what x86 is currently doing. The naming is slightly > different, there is irq_fpu_usable(). Yes, I think it's basically the same idea. It's been evolving a bit on both sides, but is quite similar now. > > The current implementation of may_use_simd() allows softirq to use > > FPSIMD/SVE unless it is currently in used (i.e kernel_neon_busy is true). > > When in used, softirqs usually fallback 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. > > Is this bad? This means also that your crypto code will not be > interrupted by a softirq. Also if you would get rid of it, you could > avoid the software fallback in case may_use_simd() says false. Masking softirqs during kernel_neon_begin()..._end() is unlikely to be a huge problem, but currently we block softirq during all context switch operations that act on the CPU vector registers. The reasons for this are somewhat historical, and IIRC predated the requirement for softirq users of kernel-mode NEON to include the may_use_simd() check and implement a fallback path on arm64. Now that softirq code is required to work around kernel-mode NEON being temporarily unusable, masking softirqs completely during context switch etc. should no longer be necessary. > > As a softirq should not rely on been able to use simd at a given time, > > there are limited reason to keep softirq disabled when touching the > > FPSIMD/SVE context. Instead, we can only disable preemption and tell > > the NEON unit is currently in use. > > > > This patch introduces two new helpers kernel_neon_{disable, enable} to > > mark the area using FPSIMD/SVE context and use them in replacement of > > local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are > > also re-implemented to use the new helpers. > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > --- > > > > I have been exploring this solution as an alternative approach to the RT > > patch "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()". > > > > So far, the patch has only been lightly tested. > > > > For RT-linux, it might be possible to use migrate_{enable, disable}. I > > am quite new with RT and have some trouble to understand the semantics > > of migrate_{enable, disable}. So far, I am still unsure if it is possible > > to run another userspace task on the same CPU while getting preempted > > when the migration is disabled. > > In RT: > - preemt_disable() is the same as !RT. A thread can not be suspend. An > interrupt may interrupt. However on RT we have threaded interrupts so > the interrupt is limited to the first-level handler (not the threaded > handler). > > - migrate_disable() means that the thread can not be moved to another > CPU. It can be suspended. > > - local_bh_disable() disables the BH: No softirq can run. In RT > local_bh_disable() does not inherit preempt_disable(). Two different > softirqs can be executed in parallel. > The BH is usually invoked at the end of the threaded interrupt > (because the threaded interrupt handler raises the softirq). It can > also run in the ksoftirqd. > > Usually you should not get preempted in a migrate_disable() section. A > SCHED_OTHER task should not interrupt another SCHED_OTHER task in a > migrate_disable() section. A task with a higher priority (a RT/DL task) > will. Since threaded interrupts run with a RT priority of 50, they will > interrupt your task in a migrate_disable() section. "Usually" is probably not good enough if another task can run: if the preempting task enters userspace then the vector registers are needed for its use, which is tricky to arrange if the registers are currently in use by context switch logic running in the first task. My current feeling is that we probably have to stick with preempt_disable() here, but hopefully we can get rid of local_bh_disable() (as proposed) with no ill effects... Does that sound sensible? Cheers ---Dave
On Wed, 13 Feb 2019 at 16:36, Dave Martin <Dave.Martin@arm.com> wrote: > > On Wed, Feb 13, 2019 at 03:30:29PM +0100, Sebastian Andrzej Siewior wrote: > > On 2019-02-08 16:55:13 [+0000], 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. > > > > This is equal what x86 is currently doing. The naming is slightly > > different, there is irq_fpu_usable(). > > Yes, I think it's basically the same idea. > > It's been evolving a bit on both sides, but is quite similar now. > may_use_simd() only exists because we have a generic crypto SIMD helper, and so we needed something arch agnostic to wrap around irq_fpu_usable() > > > The current implementation of may_use_simd() allows softirq to use > > > FPSIMD/SVE unless it is currently in used (i.e kernel_neon_busy is true). > > > When in used, softirqs usually fallback 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. > > > > Is this bad? This means also that your crypto code will not be > > interrupted by a softirq. Also if you would get rid of it, you could > > avoid the software fallback in case may_use_simd() says false. > > Masking softirqs during kernel_neon_begin()..._end() is unlikely to be a > huge problem, but currently we block softirq during all context switch > operations that act on the CPU vector registers. > > The reasons for this are somewhat historical, and IIRC predated the > requirement for softirq users of kernel-mode NEON to include the > may_use_simd() check and implement a fallback path on arm64. > > Now that softirq code is required to work around kernel-mode NEON being > temporarily unusable, masking softirqs completely during context switch > etc. should no longer be necessary. > > > > As a softirq should not rely on been able to use simd at a given time, > > > there are limited reason to keep softirq disabled when touching the > > > FPSIMD/SVE context. Instead, we can only disable preemption and tell > > > the NEON unit is currently in use. > > > > > > This patch introduces two new helpers kernel_neon_{disable, enable} to > > > mark the area using FPSIMD/SVE context and use them in replacement of > > > local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are > > > also re-implemented to use the new helpers. > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > > > --- > > > > > > I have been exploring this solution as an alternative approach to the RT > > > patch "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()". > > > > > > So far, the patch has only been lightly tested. > > > > > > For RT-linux, it might be possible to use migrate_{enable, disable}. I > > > am quite new with RT and have some trouble to understand the semantics > > > of migrate_{enable, disable}. So far, I am still unsure if it is possible > > > to run another userspace task on the same CPU while getting preempted > > > when the migration is disabled. > > > > In RT: > > - preemt_disable() is the same as !RT. A thread can not be suspend. An > > interrupt may interrupt. However on RT we have threaded interrupts so > > the interrupt is limited to the first-level handler (not the threaded > > handler). > > > > - migrate_disable() means that the thread can not be moved to another > > CPU. It can be suspended. > > > > - local_bh_disable() disables the BH: No softirq can run. In RT > > local_bh_disable() does not inherit preempt_disable(). Two different > > softirqs can be executed in parallel. > > The BH is usually invoked at the end of the threaded interrupt > > (because the threaded interrupt handler raises the softirq). It can > > also run in the ksoftirqd. > > > > Usually you should not get preempted in a migrate_disable() section. A > > SCHED_OTHER task should not interrupt another SCHED_OTHER task in a > > migrate_disable() section. A task with a higher priority (a RT/DL task) > > will. Since threaded interrupts run with a RT priority of 50, they will > > interrupt your task in a migrate_disable() section. > > "Usually" is probably not good enough if another task can run: if the > preempting task enters userspace then the vector registers are needed > for its use, which is tricky to arrange if the registers are currently > in use by context switch logic running in the first task. > > My current feeling is that we probably have to stick with > preempt_disable() here, but hopefully we can get rid of > local_bh_disable() (as proposed) with no ill effects... > > Does that sound sensible? > > Cheers > ---Dave
On Wed, Feb 13, 2019 at 04:40:00PM +0100, Ard Biesheuvel wrote: > On Wed, 13 Feb 2019 at 16:36, Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Wed, Feb 13, 2019 at 03:30:29PM +0100, Sebastian Andrzej Siewior wrote: > > > On 2019-02-08 16:55:13 [+0000], 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. > > > > > > This is equal what x86 is currently doing. The naming is slightly > > > different, there is irq_fpu_usable(). > > > > Yes, I think it's basically the same idea. > > > > It's been evolving a bit on both sides, but is quite similar now. > > > > may_use_simd() only exists because we have a generic crypto SIMD > helper, and so we needed something arch agnostic to wrap around > irq_fpu_usable() [...] Sounds plausible. Cheers ---Dave
On 2019-02-13 15:36:30 [+0000], Dave Martin wrote: > On Wed, Feb 13, 2019 at 03:30:29PM +0100, Sebastian Andrzej Siewior wrote: > > On 2019-02-08 16:55:13 [+0000], 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. > > > > This is equal what x86 is currently doing. The naming is slightly > > different, there is irq_fpu_usable(). > > Yes, I think it's basically the same idea. > > It's been evolving a bit on both sides, but is quite similar now. I though that this is complicated and wanted to submit a patch to remove irq_fpu_usable() and disable BH as part of kernel_fpu_begin() (but have currently the onother FPU series ongoing which I want to finish first). … > "Usually" is probably not good enough if another task can run: if the > preempting task enters userspace then the vector registers are needed > for its use, which is tricky to arrange if the registers are currently > in use by context switch logic running in the first task. Yes. > My current feeling is that we probably have to stick with > preempt_disable() here, but hopefully we can get rid of > local_bh_disable() (as proposed) with no ill effects... > > Does that sound sensible? If you want to stick with may_use_simd() then yes. > Cheers > ---Dave Sebastian
On 2019-02-13 16:40:00 [+0100], Ard Biesheuvel wrote: > > > This is equal what x86 is currently doing. The naming is slightly > > > different, there is irq_fpu_usable(). > > > > Yes, I think it's basically the same idea. > > > > It's been evolving a bit on both sides, but is quite similar now. > > > > may_use_simd() only exists because we have a generic crypto SIMD > helper, and so we needed something arch agnostic to wrap around > irq_fpu_usable() My question was more if this is helpful and we want to keep or if it would be better to remove it and always disable BH as part of SIMD operations. Sebastian
On Wed, Feb 13, 2019 at 05:52:27PM +0100, Sebastian Andrzej Siewior wrote: > On 2019-02-13 16:40:00 [+0100], Ard Biesheuvel wrote: > > > > This is equal what x86 is currently doing. The naming is slightly > > > > different, there is irq_fpu_usable(). > > > > > > Yes, I think it's basically the same idea. > > > > > > It's been evolving a bit on both sides, but is quite similar now. > > > > > > > may_use_simd() only exists because we have a generic crypto SIMD > > helper, and so we needed something arch agnostic to wrap around > > irq_fpu_usable() > > My question was more if this is helpful and we want to keep or if > it would be better to remove it and always disable BH as part of SIMD > operations. Wouldn't this arbitrarily increase softirq latency? Unconditionally forbidding SIMD in softirq might make more sense. It depends on how important the use cases are... Cheers ---Dave
On 12/02/2019 17:13, Julia Cartwright wrote: > Hello Julien- Hello Julien, > > On Fri, Feb 08, 2019 at 04:55:13PM +0000, 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 used (i.e kernel_neon_busy is true). >> When in used, softirqs usually fallback 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. >> >> As a softirq should not rely on been able to use simd at a given time, >> there are limited reason to keep softirq disabled when touching the >> FPSIMD/SVE context. Instead, we can only disable preemption and tell >> the NEON unit is currently in use. >> >> This patch introduces two new helpers kernel_neon_{disable, enable} to >> mark the area using FPSIMD/SVE context and use them in replacement of >> local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are >> also re-implemented to use the new helpers. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> >> I have been exploring this solution as an alternative approach to the RT >> patch "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()". >> >> So far, the patch has only been lightly tested. >> >> For RT-linux, it might be possible to use migrate_{enable, disable}. I >> am quite new with RT and have some trouble to understand the semantics >> of migrate_{enable, disable}. So far, I am still unsure if it is possible >> to run another userspace task on the same CPU while getting preempted >> when the migration is disabled. > > Yes, a thread in a migrate_disable()-protected critical section can be > preempted, and another thread on the same CPU may enter the critical > section. > > If it's necessary to prevent this from happening, then you need to also > make use of a per-CPU mutex. On RT, this would do the right thing > w.r.t. priority inheritance. Thank you for the explanation, I now understand better the concept of migrate_disable. Best regards,
Hello Sebastian, On 13/02/2019 14:30, Sebastian Andrzej Siewior wrote: > On 2019-02-08 16:55:13 [+0000], 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. > > This is equal what x86 is currently doing. The naming is slightly > different, there is irq_fpu_usable(). The idea behind the patch was taken from x86. On x86, softirq does not seem to be disabled when context switching the FPUs registers. > >> The current implementation of may_use_simd() allows softirq to use >> FPSIMD/SVE unless it is currently in used (i.e kernel_neon_busy is true). >> When in used, softirqs usually fallback 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. > > Is this bad? This means also that your crypto code will not be > interrupted by a softirq. Also if you would get rid of it, you could > avoid the software fallback in case may_use_simd() says false. There seem to have some misunderstanding about the purpose of this patch. Any use of crypto in the kernel is only protected by preempt_disable(). softirqs are only disabled when context switching the FPU register between two userspace tasks. From the commit log cb84d11e1625 "arm64: neon: Remove support for nested or hardirq kernel-mode NEON" this was done to protect against rare softirqs use crypto. It seems to me this is a bit overkill to increase softirq latency if they barely use FPSIMD/SVE. Indeed, the SVE context can be quite large, therefore it can take some times to save/restore it. [...] >> For RT-linux, it might be possible to use migrate_{enable, disable}. I >> am quite new with RT and have some trouble to understand the semantics >> of migrate_{enable, disable}. So far, I am still unsure if it is possible >> to run another userspace task on the same CPU while getting preempted >> when the migration is disabled. > > In RT: > - preemt_disable() is the same as !RT. A thread can not be suspend. An > interrupt may interrupt. However on RT we have threaded interrupts so > the interrupt is limited to the first-level handler (not the threaded > handler). > > - migrate_disable() means that the thread can not be moved to another > CPU. It can be suspended. > > - local_bh_disable() disables the BH: No softirq can run. In RT > local_bh_disable() does not inherit preempt_disable(). Two different > softirqs can be executed in parallel. > The BH is usually invoked at the end of the threaded interrupt > (because the threaded interrupt handler raises the softirq). It can > also run in the ksoftirqd. > > Usually you should not get preempted in a migrate_disable() section. A > SCHED_OTHER task should not interrupt another SCHED_OTHER task in a > migrate_disable() section. A task with a higher priority (a RT/DL task) > will. Since threaded interrupts run with a RT priority of 50, they will > interrupt your task in a migrate_disable() section. Thank you for the explanation! Cheers,
Hi, On 14/02/2019 10:34, Dave Martin wrote: > On Wed, Feb 13, 2019 at 05:52:27PM +0100, Sebastian Andrzej Siewior wrote: >> On 2019-02-13 16:40:00 [+0100], Ard Biesheuvel wrote: >>>>> This is equal what x86 is currently doing. The naming is slightly >>>>> different, there is irq_fpu_usable(). >>>> >>>> Yes, I think it's basically the same idea. >>>> >>>> It's been evolving a bit on both sides, but is quite similar now. >>>> >>> >>> may_use_simd() only exists because we have a generic crypto SIMD >>> helper, and so we needed something arch agnostic to wrap around >>> irq_fpu_usable() >> >> My question was more if this is helpful and we want to keep or if >> it would be better to remove it and always disable BH as part of SIMD >> operations. > > Wouldn't this arbitrarily increase softirq latency? Unconditionally > forbidding SIMD in softirq might make more sense. It depends on how > important the use cases are... Looking at the commit message from cb84d11e1625 "arm64: neon: Remove support for nested or hardirq kernel-mode NEON", one of the use case for crypto in softirq is certain mac80211 drivers. Is there any other use case for use crypto in softirqs? Cheers,
Hi all, On 08/02/2019 16:55, Julien Grall wrote: > @@ -1094,9 +1113,7 @@ void kernel_neon_begin(void) > /* Invalidate any task state remaining in the fpsimd regs: */ > fpsimd_flush_cpu_state(); > > - preempt_disable(); > - > - local_bh_enable(); > + kernel_neon_enable(); I found one error in the patch. We should not re-enable NEON from kernel_neon_begin(). I will resend a patch once the thread settle down. Cheers,
On 2019-02-18 15:07:51 [+0000], Julien Grall wrote: > Hi, Hi, > > Wouldn't this arbitrarily increase softirq latency? Unconditionally > > forbidding SIMD in softirq might make more sense. It depends on how > > important the use cases are... It would increase the softirq latency but the question is how bad would it be. It would continue once the SIMD section is done. If you allow it but made it impossible to use (and use the software fallback) then it would slow down the processing. So… > Looking at the commit message from cb84d11e1625 "arm64: neon: Remove support > for nested or hardirq kernel-mode NEON", one of the use case for crypto in > softirq is certain mac80211 drivers. > > Is there any other use case for use crypto in softirqs? mac80211 does it for some wifi drivers. There used to be IPsec but I *think* this moved to the "parallel processing kthread". During my FPU rework on x86 I didn't find anything that does the processing in softirq (on my machine) so I hacked something so that I could test that I didn't break anything… > Cheers, > Sebastian
Hi Sebastian, On 3/4/19 5:25 PM, Sebastian Andrzej Siewior wrote: > On 2019-02-18 15:07:51 [+0000], Julien Grall wrote: >> Hi, > Hi, > >>> Wouldn't this arbitrarily increase softirq latency? Unconditionally >>> forbidding SIMD in softirq might make more sense. It depends on how >>> important the use cases are... > > It would increase the softirq latency but the question is how bad would > it be. It would continue once the SIMD section is done. On Arm, the kernel may use either FPSIMD or SVE (if supported by the platform). While the FPSIMD context is fairly small (~4K), the SVE context can be up to ~64KB. > If you allow it but made it impossible to use (and use the software > fallback) then it would slow down the processing. So… This is a fair point. However, the use of crypto in softirqs seem to be limited. So I am wondering whether disabling softirq in all the case is worth it. Would it be possible to consider to forbid/warn about using crypto in softirqs? > >> Looking at the commit message from cb84d11e1625 "arm64: neon: Remove support >> for nested or hardirq kernel-mode NEON", one of the use case for crypto in >> softirq is certain mac80211 drivers. >> >> Is there any other use case for use crypto in softirqs? > > mac80211 does it for some wifi drivers. There used to be IPsec but I > *think* this moved to the "parallel processing kthread". I was able to find my way through mac80211 and confirm the use a taslket and therefore softirqs. However, I got lost in the ipsec code. > During my FPU rework on x86 I didn't find anything that does the > processing in softirq (on my machine) so I hacked something so that I > could test that I didn't break anything… This is the same on the platform I have been using for testing. Cheers,
On Thu, Mar 14, 2019 at 06:07:19PM +0000, Julien Grall wrote: > Hi Sebastian, > > On 3/4/19 5:25 PM, Sebastian Andrzej Siewior wrote: [...] > >It would increase the softirq latency but the question is how bad would > >it be. It would continue once the SIMD section is done. > > On Arm, the kernel may use either FPSIMD or SVE (if supported by the > platform). While the FPSIMD context is fairly small (~4K), the SVE context > can be up to ~64KB. It's not quite as bad as that. The FPSIMD state is ~0.5K, with the SVE state size being up to ~8.5K (though for today's implementations ~2K may be considered typical). For comparision, I believe AVX-512 has ~2K of state. Cheers ---Dave
On 15/03/2019 10:06, Dave Martin wrote: > On Thu, Mar 14, 2019 at 06:07:19PM +0000, Julien Grall wrote: >> Hi Sebastian, >> >> On 3/4/19 5:25 PM, Sebastian Andrzej Siewior wrote: > > [...] > >>> It would increase the softirq latency but the question is how bad would >>> it be. It would continue once the SIMD section is done. >> >> On Arm, the kernel may use either FPSIMD or SVE (if supported by the >> platform). While the FPSIMD context is fairly small (~4K), the SVE context >> can be up to ~64KB. > > It's not quite as bad as that. The FPSIMD state is ~0.5K, with the SVE > state size being up to ~8.5K (though for today's implementations ~2K may > be considered typical). Whoops, I forgot to divide by 8. Thank you for spotting it! Cheers,
On Fri, Feb 08, 2019 at 04:55:13PM +0000, 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 used (i.e kernel_neon_busy is true). > When in used, softirqs usually fallback 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. > > As a softirq should not rely on been able to use simd at a given time, > there are limited reason to keep softirq disabled when touching the > FPSIMD/SVE context. Instead, we can only disable preemption and tell > the NEON unit is currently in use. > > This patch introduces two new helpers kernel_neon_{disable, enable} to > mark the area using FPSIMD/SVE context and use them in replacement of > local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are > also re-implemented to use the new helpers. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > I have been exploring this solution as an alternative approach to the RT > patch "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()". > > So far, the patch has only been lightly tested. > > For RT-linux, it might be possible to use migrate_{enable, disable}. I > am quite new with RT and have some trouble to understand the semantics > of migrate_{enable, disable}. So far, I am still unsure if it is possible > to run another userspace task on the same CPU while getting preempted > when the migration is disabled. (Leaving aside the RT aspects for now, but if migrate_{enable,disable} is costly it might not be the best thing to use deep in context switch paths, even if is technically correct...) > --- > arch/arm64/include/asm/simd.h | 4 +-- > arch/arm64/kernel/fpsimd.c | 76 +++++++++++++++++++++++++------------------ > 2 files changed, 46 insertions(+), 34 deletions(-) > > diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h > index 6495cc51246f..94c0dac508aa 100644 > --- a/arch/arm64/include/asm/simd.h > +++ b/arch/arm64/include/asm/simd.h > @@ -15,10 +15,10 @@ > #include <linux/preempt.h> > #include <linux/types.h> > > -#ifdef CONFIG_KERNEL_MODE_NEON > - > DECLARE_PER_CPU(bool, kernel_neon_busy); Why make this unconditional? This declaration is only here for may_use_simd() to use. The stub version of may_use_simd() for the !CONFIG_KERNEL_MODE_NEON case doesn't touch it. > > +#ifdef CONFIG_KERNEL_MODE_NEON > + > /* > * may_use_simd - whether it is allowable at this time to issue SIMD > * instructions or access the SIMD register file > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 5ebe73b69961..b7e5dac26190 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -90,7 +90,8 @@ > * To prevent this from racing with the manipulation of the task's FPSIMD state > * from task context and thereby corrupting the state, it is necessary to > * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE > - * flag with local_bh_disable() unless softirqs are already masked. > + * flag with kernel_neon_{disable, enable}. This will still allow softirqs to > + * run but prevent them to use FPSIMD. > * > * For a certain task, the sequence may look something like this: > * - the task gets scheduled in; if both the task's fpsimd_cpu field > @@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state; > > #endif /* ! CONFIG_ARM64_SVE */ > > +static void kernel_neon_disable(void); > +static void kernel_neon_enable(void); I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE context access for the current context (i.e., makes it safe). Since these also disable/enable preemption, perhaps we can align them with the existing get_cpu()/put_cpu(), something like: void get_cpu_fpsimd_context(); vold put_cpu_fpsimd_context(); If you consider it's worth adding the checking helper I alluded to above, it could then be called something like: bool have_cpu_fpsimd_context(); > + > /* > * Call __sve_free() directly only if you know task can't be scheduled > * or preempted. > @@ -213,11 +217,11 @@ static void sve_free(struct task_struct *task) > * thread_struct is known to be up to date, when preparing to enter > * userspace. > * > - * Softirqs (and preemption) must be disabled. > + * Preemption must be disabled. [*] That's not enough: we need to be in kernel_neon_disable()... _enable() (i.e., kernel_neon_busy needs to be true to prevent softirqs messing with the FPSIMD state). > */ > static void task_fpsimd_load(void) > { > - WARN_ON(!in_softirq() && !irqs_disabled()); > + WARN_ON(!preempt_count() && !irqs_disabled()); Hmmm, looks like we can rewrite this is preemptible(). See include/linux/preempt.h. Since we are checking that nothing can mess with the FPSIMD regs and associated task metadata, we should also be checking kernel_neon_busy here. For readability, we could wrap all that up in a single helper. > if (system_supports_sve() && test_thread_flag(TIF_SVE)) > sve_load_state(sve_pffr(¤t->thread), > @@ -238,7 +242,7 @@ void fpsimd_save(void) > struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st); > /* set by fpsimd_bind_task_to_cpu() or fpsimd_bind_state_to_cpu() */ > > - WARN_ON(!in_softirq() && !irqs_disabled()); > + WARN_ON(!preempt_count() && !irqs_disabled()); > > if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) { > if (system_supports_sve() && test_thread_flag(TIF_SVE)) { > @@ -360,7 +364,7 @@ static int __init sve_sysctl_init(void) { return 0; } > * task->thread.sve_state. > * > * Task can be a non-runnable task, or current. In the latter case, > - * softirqs (and preemption) must be disabled. > + * preemption must be disabled. See [*]. > * task->thread.sve_state must point to at least sve_state_size(task) > * bytes of allocated kernel memory. > * task->thread.uw.fpsimd_state must be up to date before calling this > @@ -387,7 +391,7 @@ static void fpsimd_to_sve(struct task_struct *task) > * task->thread.uw.fpsimd_state. > * > * Task can be a non-runnable task, or current. In the latter case, > - * softirqs (and preemption) must be disabled. > + * preemption must be disabled. See [*]. > * task->thread.sve_state must point to at least sve_state_size(task) > * bytes of allocated kernel memory. > * task->thread.sve_state must be up to date before calling this function. > @@ -547,7 +551,7 @@ int sve_set_vector_length(struct task_struct *task, > * non-SVE thread. > */ > if (task == current) { > - local_bh_disable(); > + kernel_neon_disable(); > > fpsimd_save(); > set_thread_flag(TIF_FOREIGN_FPSTATE); > @@ -558,7 +562,7 @@ int sve_set_vector_length(struct task_struct *task, > sve_to_fpsimd(task); > > if (task == current) > - local_bh_enable(); > + kernel_neon_enable(); > > /* > * Force reallocation of task SVE state to the correct size > @@ -813,7 +817,7 @@ asmlinkage void do_sve_acc(unsigned int esr, struct pt_regs *regs) > > sve_alloc(current); > > - local_bh_disable(); > + kernel_neon_disable(); > > fpsimd_save(); > fpsimd_to_sve(current); > @@ -825,7 +829,7 @@ asmlinkage void do_sve_acc(unsigned int esr, struct pt_regs *regs) > if (test_and_set_thread_flag(TIF_SVE)) > WARN_ON(1); /* SVE access shouldn't have trapped */ > > - local_bh_enable(); > + kernel_neon_enable(); > } > > /* > @@ -892,7 +896,7 @@ void fpsimd_flush_thread(void) > if (!system_supports_fpsimd()) > return; > > - local_bh_disable(); > + kernel_neon_disable(); > > memset(¤t->thread.uw.fpsimd_state, 0, > sizeof(current->thread.uw.fpsimd_state)); > @@ -935,7 +939,7 @@ void fpsimd_flush_thread(void) > > set_thread_flag(TIF_FOREIGN_FPSTATE); > > - local_bh_enable(); > + kernel_neon_enable(); > } > > /* > @@ -947,9 +951,9 @@ void fpsimd_preserve_current_state(void) > if (!system_supports_fpsimd()) > return; > > - local_bh_disable(); > + kernel_neon_disable(); > fpsimd_save(); > - local_bh_enable(); > + kernel_neon_enable(); > } > > /* > @@ -1007,14 +1011,14 @@ void fpsimd_restore_current_state(void) > if (!system_supports_fpsimd()) > return; > > - local_bh_disable(); > + kernel_neon_disable(); > > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > task_fpsimd_load(); > fpsimd_bind_task_to_cpu(); > } > > - local_bh_enable(); > + kernel_neon_enable(); > } > > /* > @@ -1027,7 +1031,7 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state) > if (!system_supports_fpsimd()) > return; > > - local_bh_disable(); > + kernel_neon_disable(); > > current->thread.uw.fpsimd_state = *state; > if (system_supports_sve() && test_thread_flag(TIF_SVE)) > @@ -1038,7 +1042,7 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state) > > clear_thread_flag(TIF_FOREIGN_FPSTATE); > > - local_bh_enable(); > + kernel_neon_enable(); > } > > /* > @@ -1055,11 +1059,28 @@ void fpsimd_flush_cpu_state(void) > set_thread_flag(TIF_FOREIGN_FPSTATE); > } > > -#ifdef CONFIG_KERNEL_MODE_NEON > - > DEFINE_PER_CPU(bool, kernel_neon_busy); > EXPORT_PER_CPU_SYMBOL(kernel_neon_busy); > > +static void kernel_neon_disable(void) > +{ > + preempt_disable(); > + WARN_ON(__this_cpu_read(kernel_neon_busy)); > + __this_cpu_write(kernel_neon_busy, true); Can we do this with one __this_cpu_xchg()? > +} > + > +static void kernel_neon_enable(void) > +{ > + bool busy; > + > + busy = __this_cpu_xchg(kernel_neon_busy, false); > + WARN_ON(!busy); /* No matching kernel_neon_disable()? */ > + > + preempt_enable(); > +} > + > +#ifdef CONFIG_KERNEL_MODE_NEON > + > /* > * Kernel-side NEON support functions > */ > @@ -1084,9 +1105,7 @@ void kernel_neon_begin(void) > > BUG_ON(!may_use_simd()); > > - local_bh_disable(); > - > - __this_cpu_write(kernel_neon_busy, true); > + kernel_neon_disable(); > > /* Save unsaved fpsimd state, if any: */ > fpsimd_save(); > @@ -1094,9 +1113,7 @@ void kernel_neon_begin(void) > /* Invalidate any task state remaining in the fpsimd regs: */ > fpsimd_flush_cpu_state(); > > - preempt_disable(); > - > - local_bh_enable(); > + kernel_neon_enable(); As you commented in your reply elsewhere, we don't want to call kernel_neon_enable() here. We need to keep exclusive ownership of the CPU regs to continue until kernel_neon_end() is called. Otherwise, this looks reasonable overall. One nice feature of this is that is makes it clearer that the code is grabbing exclusive access to a particular thing (the FPSIMD regs and context metadata), which is not very obvious from the bare local_bh_{disable,enable} that was there previously. When reposting, you should probably rebase on kvmarm/next [1], since there is a minor conflict from the SVE KVM series. It looks straightforward to fix up though. [...] For testing, can we have a test irritator module that does something like hooking the timer softirq with a kprobe and trashing the regs inside kernel_neon_begin()/_end()? It would be nice to have such a thing upstream, but it's OK to test with local hacks for now. I'm not sure how this patch will affect context switch overhead, so it would be good to see hackbench numbers (or similar). Cheers ---Dave [1] git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
Hi Dave, Thank you for the review. On 4/4/19 11:52 AM, Dave Martin wrote: > On Fri, Feb 08, 2019 at 04:55:13PM +0000, Julien Grall wrote: >> For RT-linux, it might be possible to use migrate_{enable, disable}. I >> am quite new with RT and have some trouble to understand the semantics >> of migrate_{enable, disable}. So far, I am still unsure if it is possible >> to run another userspace task on the same CPU while getting preempted >> when the migration is disabled. > > (Leaving aside the RT aspects for now, but if migrate_{enable,disable} > is costly it might not be the best thing to use deep in context switch > paths, even if is technically correct...) Based on the discussion in this thread, migrate_enable/migrate_disable is not suitable in this context. The goal of those helpers is to pin the task to the current CPU. On RT, it will not disable the preemption. So the current task can be preempted by a task with higher priority. The next task may require to use the FPSIMD and will potentially result to corrupt the state. RT folks already saw this corruption because local_bh_disable() does not preempt on RT. They are carrying a patch (see "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()") to disable preemption along with local_bh_disable(). Alternatively, Julia suggested to introduce a per-cpu lock to protect the state. I am thinking to defer this for a follow-up patch. The changes in this patch should make it easier because we now have helper to mark the critical section. > >> --- >> arch/arm64/include/asm/simd.h | 4 +-- >> arch/arm64/kernel/fpsimd.c | 76 +++++++++++++++++++++++++------------------ >> 2 files changed, 46 insertions(+), 34 deletions(-) >> >> diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h >> index 6495cc51246f..94c0dac508aa 100644 >> --- a/arch/arm64/include/asm/simd.h >> +++ b/arch/arm64/include/asm/simd.h >> @@ -15,10 +15,10 @@ >> #include <linux/preempt.h> >> #include <linux/types.h> >> >> -#ifdef CONFIG_KERNEL_MODE_NEON >> - >> DECLARE_PER_CPU(bool, kernel_neon_busy); > > Why make this unconditional? This declaration is only here for > may_use_simd() to use. The stub version of may_use_simd() for the > !CONFIG_KERNEL_MODE_NEON case doesn't touch it. kernel_neon_busy will be used in fpsimd.c even when with !CONFIG_KERNEL_MODE_NEON. So it makes sense to also declare it in the header even if not used. Another solution is to avoid expose kernel_neon_busy outside of fpsimd.c and use an helper. > >> >> +#ifdef CONFIG_KERNEL_MODE_NEON >> + >> /* >> * may_use_simd - whether it is allowable at this time to issue SIMD >> * instructions or access the SIMD register file >> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >> index 5ebe73b69961..b7e5dac26190 100644 >> --- a/arch/arm64/kernel/fpsimd.c >> +++ b/arch/arm64/kernel/fpsimd.c >> @@ -90,7 +90,8 @@ >> * To prevent this from racing with the manipulation of the task's FPSIMD state >> * from task context and thereby corrupting the state, it is necessary to >> * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE >> - * flag with local_bh_disable() unless softirqs are already masked. >> + * flag with kernel_neon_{disable, enable}. This will still allow softirqs to >> + * run but prevent them to use FPSIMD. >> * >> * For a certain task, the sequence may look something like this: >> * - the task gets scheduled in; if both the task's fpsimd_cpu field >> @@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state; >> >> #endif /* ! CONFIG_ARM64_SVE */ >> >> +static void kernel_neon_disable(void); >> +static void kernel_neon_enable(void); > > I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE > context access for the current context (i.e., makes it safe). > > Since these also disable/enable preemption, perhaps we can align them > with the existing get_cpu()/put_cpu(), something like: > > void get_cpu_fpsimd_context(); > vold put_cpu_fpsimd_context(); > > If you consider it's worth adding the checking helper I alluded to > above, it could then be called something like: > > bool have_cpu_fpsimd_context(); I am not sure where you suggested a checking helper above. Do you refer to exposing kernel_neon_busy even for !CONFIG_KERNEL_MODE_NEON? > >> + >> /* >> * Call __sve_free() directly only if you know task can't be scheduled >> * or preempted. >> @@ -213,11 +217,11 @@ static void sve_free(struct task_struct *task) >> * thread_struct is known to be up to date, when preparing to enter >> * userspace. >> * >> - * Softirqs (and preemption) must be disabled. >> + * Preemption must be disabled. > > [*] That's not enough: we need to be in kernel_neon_disable()... > _enable() (i.e., kernel_neon_busy needs to be true to prevent softirqs > messing with the FPSIMD state). How about not mentioning preemption at all and just say: "The fpsimd context should be acquired before hand". This would help if we ever decide to protect critical section differently. > >> */ >> static void task_fpsimd_load(void) >> { >> - WARN_ON(!in_softirq() && !irqs_disabled()); >> + WARN_ON(!preempt_count() && !irqs_disabled()); > > Hmmm, looks like we can rewrite this is preemptible(). See > include/linux/preempt.h. > > Since we are checking that nothing can mess with the FPSIMD regs and > associated task metadata, we should also be checking kernel_neon_busy > here. > > For readability, we could wrap all that up in a single helper. With what I said above, we could replace this check WARN_ON(!have_cpu_fpsimd_context()). [...] >> +static void kernel_neon_disable(void) >> +{ >> + preempt_disable(); >> + WARN_ON(__this_cpu_read(kernel_neon_busy)); >> + __this_cpu_write(kernel_neon_busy, true); > > Can we do this with one __this_cpu_xchg()? I think so. > >> +} >> + >> +static void kernel_neon_enable(void) >> +{ >> + bool busy; >> + >> + busy = __this_cpu_xchg(kernel_neon_busy, false); >> + WARN_ON(!busy); /* No matching kernel_neon_disable()? */ >> + >> + preempt_enable(); >> +} >> + >> +#ifdef CONFIG_KERNEL_MODE_NEON >> + >> /* >> * Kernel-side NEON support functions >> */ >> @@ -1084,9 +1105,7 @@ void kernel_neon_begin(void) >> >> BUG_ON(!may_use_simd()); >> >> - local_bh_disable(); >> - >> - __this_cpu_write(kernel_neon_busy, true); >> + kernel_neon_disable(); >> >> /* Save unsaved fpsimd state, if any: */ >> fpsimd_save(); >> @@ -1094,9 +1113,7 @@ void kernel_neon_begin(void) >> /* Invalidate any task state remaining in the fpsimd regs: */ >> fpsimd_flush_cpu_state(); >> >> - preempt_disable(); >> - >> - local_bh_enable(); >> + kernel_neon_enable(); > > As you commented in your reply elsewhere, we don't want to call > kernel_neon_enable() here. We need to keep exclusive ownership of the > CPU regs to continue until kernel_neon_end() is called. I already fixed it in my tree. Thank you for the reminder. > > Otherwise, this looks reasonable overall. > > One nice feature of this is that is makes it clearer that the code is > grabbing exclusive access to a particular thing (the FPSIMD regs and > context metadata), which is not very obvious from the bare > local_bh_{disable,enable} that was there previously. > > When reposting, you should probably rebase on kvmarm/next [1], since > there is a minor conflict from the SVE KVM series. It looks > straightforward to fix up though. I will have a look. > > [...] > > For testing, can we have a test irritator module that does something > like hooking the timer softirq with a kprobe and trashing the regs > inside kernel_neon_begin()/_end()? I will see what I can do. > > It would be nice to have such a thing upstream, but it's OK to test > with local hacks for now. > > > I'm not sure how this patch will affect context switch overhead, so it > would be good to see hackbench numbers (or similar). I will give a try with hackbench/kernbench. Cheers, > [1] git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git > next >
On 2019-04-05 10:02:45 [+0100], Julien Grall wrote: > RT folks already saw this corruption because local_bh_disable() does not > preempt on RT. They are carrying a patch (see "arm64: fpsimd: use > preemp_disable in addition to local_bh_disable()") to disable preemption > along with local_bh_disable(). > > Alternatively, Julia suggested to introduce a per-cpu lock to protect the > state. I am thinking to defer this for a follow-up patch. The changes in > this patch should make it easier because we now have helper to mark the > critical section. A per-CPU lock? It has to be a raw_spinlock_t because a normal spin_lock() / local_lock() would allow scheduling and might be taken as part of the context switch or soon after. Sebastian
On Fri, Apr 05, 2019 at 10:02:45AM +0100, Julien Grall wrote: > Hi Dave, > > Thank you for the review. > > On 4/4/19 11:52 AM, Dave Martin wrote: > >On Fri, Feb 08, 2019 at 04:55:13PM +0000, Julien Grall wrote: > >>For RT-linux, it might be possible to use migrate_{enable, disable}. I > >>am quite new with RT and have some trouble to understand the semantics > >>of migrate_{enable, disable}. So far, I am still unsure if it is possible > >>to run another userspace task on the same CPU while getting preempted > >>when the migration is disabled. > > > >(Leaving aside the RT aspects for now, but if migrate_{enable,disable} > >is costly it might not be the best thing to use deep in context switch > >paths, even if is technically correct...) > > Based on the discussion in this thread, migrate_enable/migrate_disable is > not suitable in this context. > > The goal of those helpers is to pin the task to the current CPU. On RT, it > will not disable the preemption. So the current task can be preempted by a > task with higher priority. > > The next task may require to use the FPSIMD and will potentially result to > corrupt the state. > > RT folks already saw this corruption because local_bh_disable() does not > preempt on RT. They are carrying a patch (see "arm64: fpsimd: use > preemp_disable in addition to local_bh_disable()") to disable preemption > along with local_bh_disable(). > > Alternatively, Julia suggested to introduce a per-cpu lock to protect the > state. I am thinking to defer this for a follow-up patch. The changes in > this patch should make it easier because we now have helper to mark the > critical section. I'll leave it for the RT folks to comment on this. (I see Sebastian already did.) > > > > >>--- > >> arch/arm64/include/asm/simd.h | 4 +-- > >> arch/arm64/kernel/fpsimd.c | 76 +++++++++++++++++++++++++------------------ > >> 2 files changed, 46 insertions(+), 34 deletions(-) > >> > >>diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h > >>index 6495cc51246f..94c0dac508aa 100644 > >>--- a/arch/arm64/include/asm/simd.h > >>+++ b/arch/arm64/include/asm/simd.h > >>@@ -15,10 +15,10 @@ > >> #include <linux/preempt.h> > >> #include <linux/types.h> > >>-#ifdef CONFIG_KERNEL_MODE_NEON > >>- > >> DECLARE_PER_CPU(bool, kernel_neon_busy); > > > >Why make this unconditional? This declaration is only here for > >may_use_simd() to use. The stub version of may_use_simd() for the > >!CONFIG_KERNEL_MODE_NEON case doesn't touch it. > > kernel_neon_busy will be used in fpsimd.c even when with > !CONFIG_KERNEL_MODE_NEON. So it makes sense to also declare it in the header > even if not used. Ah yes, I missed that. We don't need all this logic in the !CONFIG_KERNEL_MODE_NEON case of course, but I'm not sure it's worth optimising that special case. Especially so if we don't see any significant impact in ctxsw-heavy benchmarks. > Another solution is to avoid expose kernel_neon_busy outside of fpsimd.c and > use an helper. Probably not worth it for now. > >>+#ifdef CONFIG_KERNEL_MODE_NEON > >>+ > >> /* > >> * may_use_simd - whether it is allowable at this time to issue SIMD > >> * instructions or access the SIMD register file > >>diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > >>index 5ebe73b69961..b7e5dac26190 100644 > >>--- a/arch/arm64/kernel/fpsimd.c > >>+++ b/arch/arm64/kernel/fpsimd.c > >>@@ -90,7 +90,8 @@ > >> * To prevent this from racing with the manipulation of the task's FPSIMD state > >> * from task context and thereby corrupting the state, it is necessary to > >> * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE > >>- * flag with local_bh_disable() unless softirqs are already masked. > >>+ * flag with kernel_neon_{disable, enable}. This will still allow softirqs to > >>+ * run but prevent them to use FPSIMD. > >> * > >> * For a certain task, the sequence may look something like this: > >> * - the task gets scheduled in; if both the task's fpsimd_cpu field > >>@@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state; > >> #endif /* ! CONFIG_ARM64_SVE */ > >>+static void kernel_neon_disable(void); > >>+static void kernel_neon_enable(void); > > > >I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE > >context access for the current context (i.e., makes it safe). > > > >Since these also disable/enable preemption, perhaps we can align them > >with the existing get_cpu()/put_cpu(), something like: > > > >void get_cpu_fpsimd_context(); > >vold put_cpu_fpsimd_context(); > > > >If you consider it's worth adding the checking helper I alluded to > >above, it could then be called something like: > > > >bool have_cpu_fpsimd_context(); > > I am not sure where you suggested a checking helper above. Do you refer to > exposing kernel_neon_busy even for !CONFIG_KERNEL_MODE_NEON? Hmmm, looks like I got my reply out of order here. I meant the helper (if any) to check !preemptible() && !__this_cpu_read(kernel_neon_busy). Looks like you inferred what I meant later on anyway. > > > > >>+ > >> /* > >> * Call __sve_free() directly only if you know task can't be scheduled > >> * or preempted. > >>@@ -213,11 +217,11 @@ static void sve_free(struct task_struct *task) > >> * thread_struct is known to be up to date, when preparing to enter > >> * userspace. > >> * > >>- * Softirqs (and preemption) must be disabled. > >>+ * Preemption must be disabled. > > > >[*] That's not enough: we need to be in kernel_neon_disable()... > >_enable() (i.e., kernel_neon_busy needs to be true to prevent softirqs > >messing with the FPSIMD state). > > How about not mentioning preemption at all and just say: > > "The fpsimd context should be acquired before hand". > > This would help if we ever decide to protect critical section differently. Yes, or even better, name the function used to do this (i.e., kernel_neon_disable() or get_cpu_fpsimd_context() or whatever it's called). > > > >> */ > >> static void task_fpsimd_load(void) > >> { > >>- WARN_ON(!in_softirq() && !irqs_disabled()); > >>+ WARN_ON(!preempt_count() && !irqs_disabled()); > > > >Hmmm, looks like we can rewrite this is preemptible(). See > >include/linux/preempt.h. > > > >Since we are checking that nothing can mess with the FPSIMD regs and > >associated task metadata, we should also be checking kernel_neon_busy > >here. > > > >For readability, we could wrap all that up in a single helper. > > With what I said above, we could replace this check > WARN_ON(!have_cpu_fpsimd_context()). Agreed. > [...] > > >>+static void kernel_neon_disable(void) > >>+{ > >>+ preempt_disable(); > >>+ WARN_ON(__this_cpu_read(kernel_neon_busy)); > >>+ __this_cpu_write(kernel_neon_busy, true); > > > >Can we do this with one __this_cpu_xchg()? > > I think so. OK > >>+} > >>+ > >>+static void kernel_neon_enable(void) > >>+{ > >>+ bool busy; > >>+ > >>+ busy = __this_cpu_xchg(kernel_neon_busy, false); > >>+ WARN_ON(!busy); /* No matching kernel_neon_disable()? */ > >>+ > >>+ preempt_enable(); > >>+} > >>+ > >>+#ifdef CONFIG_KERNEL_MODE_NEON > >>+ > >> /* > >> * Kernel-side NEON support functions > >> */ > >>@@ -1084,9 +1105,7 @@ void kernel_neon_begin(void) > >> BUG_ON(!may_use_simd()); > >>- local_bh_disable(); > >>- > >>- __this_cpu_write(kernel_neon_busy, true); > >>+ kernel_neon_disable(); > >> /* Save unsaved fpsimd state, if any: */ > >> fpsimd_save(); > >>@@ -1094,9 +1113,7 @@ void kernel_neon_begin(void) > >> /* Invalidate any task state remaining in the fpsimd regs: */ > >> fpsimd_flush_cpu_state(); > >>- preempt_disable(); > >>- > >>- local_bh_enable(); > >>+ kernel_neon_enable(); > > > >As you commented in your reply elsewhere, we don't want to call > >kernel_neon_enable() here. We need to keep exclusive ownership of the > >CPU regs to continue until kernel_neon_end() is called. > > I already fixed it in my tree. Thank you for the reminder. Yes, just confirming my understanding here. > >Otherwise, this looks reasonable overall. > > > >One nice feature of this is that is makes it clearer that the code is > >grabbing exclusive access to a particular thing (the FPSIMD regs and > >context metadata), which is not very obvious from the bare > >local_bh_{disable,enable} that was there previously. > > > >When reposting, you should probably rebase on kvmarm/next [1], since > >there is a minor conflict from the SVE KVM series. It looks > >straightforward to fix up though. > > I will have a look. > > > > >[...] > > > >For testing, can we have a test irritator module that does something > >like hooking the timer softirq with a kprobe and trashing the regs > >inside kernel_neon_begin()/_end()? > > I will see what I can do. > > > > >It would be nice to have such a thing upstream, but it's OK to test > >with local hacks for now. > > > > > >I'm not sure how this patch will affect context switch overhead, so it > >would be good to see hackbench numbers (or similar). > > I will give a try with hackbench/kernbench. Thanks. You can repost the patch before this is done though, to help move the review forward. Cheers ---Dave
Hi, On 05/04/2019 15:39, Sebastian Andrzej Siewior wrote: > On 2019-04-05 10:02:45 [+0100], Julien Grall wrote: >> RT folks already saw this corruption because local_bh_disable() does not >> preempt on RT. They are carrying a patch (see "arm64: fpsimd: use >> preemp_disable in addition to local_bh_disable()") to disable preemption >> along with local_bh_disable(). >> >> Alternatively, Julia suggested to introduce a per-cpu lock to protect the >> state. I am thinking to defer this for a follow-up patch. The changes in >> this patch should make it easier because we now have helper to mark the >> critical section. > > A per-CPU lock? It has to be a raw_spinlock_t because a normal > spin_lock() / local_lock() would allow scheduling and might be taken as > part of the context switch or soon after. raw_spinlock_t would not work here without disabling preemption. Otherwise you may end up to recurse on the lock and therefore deadlock. But then it raise the question of the usefulness of the lock here. However, I don't really understand why allowing the scheduling would be a problem here. Is it a concern because we will waste cycle trying to restore/save a context that will be scratched as soon as we release the lock? Cheers,
On 2019-04-05 16:17:50 [+0100], Julien Grall wrote: > Hi, Hi, > > A per-CPU lock? It has to be a raw_spinlock_t because a normal > > spin_lock() / local_lock() would allow scheduling and might be taken as > > part of the context switch or soon after. > raw_spinlock_t would not work here without disabling preemption. Otherwise > you may end up to recurse on the lock and therefore deadlock. But then it > raise the question of the usefulness of the lock here. > > However, I don't really understand why allowing the scheduling would be a > problem here. Is it a concern because we will waste cycle trying to > restore/save a context that will be scratched as soon as we release the > lock? If you hold the lock within the kernel thread and every kernel thread acquires it before doing any SIMD operations then you are good. It could be a sleeping lock. What happens if you hold the lock, are scheduled out and a user task is about to be scheduled? How do you force the kernel thread out / give up the FPU registers? That preempt_disable() + local_bh_disable() might not be the pretties thing but how bad is it actually? Latency wise you can't schedule(). From RT point of view you need to enable preemption while going from page to page because of the possible kmap() or kmalloc() (on baldy aligned src/dst) with the crypto's page-walk code. If that is not good enough latency wise you could do kernel_fpu_resched() after a few iterations. Currently I'm trying to get kernel_fpu_begin()/end() cheap on x86 so it doesn't always store/restore the FPU context. Then kernel_fpu_resched() shouldn't be that bad. > Cheers, Sebastian
Hi Dave, On 4/4/19 11:52 AM, Dave Martin wrote: > On Fri, Feb 08, 2019 at 04:55:13PM +0000, Julien Grall wrote: > I'm not sure how this patch will affect context switch overhead, so it > would be good to see hackbench numbers (or similar). I finally have some numbers for this patch. The benchmark was ran using Linux 5.1-rc4 and defconfig. On Juno2: * hackbench 100 process 1000 (10 times) * .7% quicker On ThunderX 2: * hackbench 1000 process 1000 (20 times) * 3.4% quicker I am happy to try other benchmark if you think it is useful. Anyway, I will resend the series with the comments addressed. Cheers,
Hi Sebastian, On 4/5/19 4:42 PM, Sebastian Andrzej Siewior wrote: > On 2019-04-05 16:17:50 [+0100], Julien Grall wrote: >> Hi, > Hi, > >>> A per-CPU lock? It has to be a raw_spinlock_t because a normal >>> spin_lock() / local_lock() would allow scheduling and might be taken as >>> part of the context switch or soon after. >> raw_spinlock_t would not work here without disabling preemption. Otherwise >> you may end up to recurse on the lock and therefore deadlock. But then it >> raise the question of the usefulness of the lock here. >> >> However, I don't really understand why allowing the scheduling would be a >> problem here. Is it a concern because we will waste cycle trying to >> restore/save a context that will be scratched as soon as we release the >> lock? > > If you hold the lock within the kernel thread and every kernel thread > acquires it before doing any SIMD operations then you are good. It could > be a sleeping lock. What happens if you hold the lock, are scheduled out > and a user task is about to be scheduled? How do you force the kernel > thread out / give up the FPU registers? You would need the thread out to finish running the critical section before the next thread to run. I was under the impression with sleeping lock, the priority of the thread out would get bumped to finish quickly the critical section. I agree it means that you will waste time restoring registers that will get trashed right after you leave the critical sections. This is probably not really efficient. Anyway, that was only a suggestion I haven't fully thought through. I have no plan to do more work than this patch in the fpsimd context switch. > > That preempt_disable() + local_bh_disable() might not be the pretties > thing but how bad is it actually? From the measurement I did on non-RT, it is beneficial to keep the softirq enabled (see [1]). I don't have platform where FPSIMD is used in softirqs (or at least not by default). So for testing purpose, I wrote a tasklet (based on hrtimer) using the FPSIMD registers every 1ms if it is allowed (i.e may_use_simd() returns true). I let it run for a while and notice that the tasklet will be executed only 0.15% of the time when !may_use_simd(). Furthermore, from what I understood in this thread, there are few limited use cases where FPSIMD will be used in softirqs. So it seems better to me to avoid disabling softirqs at least in non-RT. For the RT, aren't all the softirqs handled in a thread? So what would be the benefits to disable softirqs if we already disable preemption? In any case, this patch introduces new helpers (get_cpu_fpsimd_context and put_cpu_fpsimd_context) to delimit regions using the HW FPSIMD. So it would be easy to modify the behavior if we wanted too. > Latency wise you can't schedule(). From RT point of view you need to > enable preemption while going from page to page because of the possible > kmap() or kmalloc() (on baldy aligned src/dst) with the crypto's > page-walk code. Make sense. However I don't think we can keep enable the preemption with the current implementation of FPSIMD context switch. I noticed you disabled crypto for Arm64 because of allocations, I have a todo to look at what we can do. Cheers, [1] https://marc.info/?l=linux-rt-users&m=155499183812211&w=2 > > Sebastian >
Hi Dave, On 4/5/19 4:07 PM, Dave Martin wrote: > On Fri, Apr 05, 2019 at 10:02:45AM +0100, Julien Grall wrote: >>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>>> + >>>> /* >>>> * may_use_simd - whether it is allowable at this time to issue SIMD >>>> * instructions or access the SIMD register file >>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >>>> index 5ebe73b69961..b7e5dac26190 100644 >>>> --- a/arch/arm64/kernel/fpsimd.c >>>> +++ b/arch/arm64/kernel/fpsimd.c >>>> @@ -90,7 +90,8 @@ >>>> * To prevent this from racing with the manipulation of the task's FPSIMD state >>>> * from task context and thereby corrupting the state, it is necessary to >>>> * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE >>>> - * flag with local_bh_disable() unless softirqs are already masked. >>>> + * flag with kernel_neon_{disable, enable}. This will still allow softirqs to >>>> + * run but prevent them to use FPSIMD. >>>> * >>>> * For a certain task, the sequence may look something like this: >>>> * - the task gets scheduled in; if both the task's fpsimd_cpu field >>>> @@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state; >>>> #endif /* ! CONFIG_ARM64_SVE */ >>>> +static void kernel_neon_disable(void); >>>> +static void kernel_neon_enable(void); >>> >>> I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE >>> context access for the current context (i.e., makes it safe). >>> >>> Since these also disable/enable preemption, perhaps we can align them >>> with the existing get_cpu()/put_cpu(), something like: >>> >>> void get_cpu_fpsimd_context(); >>> vold put_cpu_fpsimd_context(); >>> >>> If you consider it's worth adding the checking helper I alluded to >>> above, it could then be called something like: >>> >>> bool have_cpu_fpsimd_context(); >> >> I am not sure where you suggested a checking helper above. Do you refer to >> exposing kernel_neon_busy even for !CONFIG_KERNEL_MODE_NEON? > > Hmmm, looks like I got my reply out of order here. > > I meant the helper (if any) to check > !preemptible() && !__this_cpu_read(kernel_neon_busy). I guess you are using && instead of || because some of the caller may not call get_cpu_fpsimd_context() before but still disable preemption, right? Wouldn't it be better to have all the user calling get_cpu_fpsimd_context() and put_cpu_fpsimd_context()? This has the advantage to uniformize how the way FPSIMD is protected and also... > > Looks like you inferred what I meant later on anyway. > >> >>> >>>> + >>>> /* >>>> * Call __sve_free() directly only if you know task can't be scheduled >>>> * or preempted. >>>> @@ -213,11 +217,11 @@ static void sve_free(struct task_struct *task) >>>> * thread_struct is known to be up to date, when preparing to enter >>>> * userspace. >>>> * >>>> - * Softirqs (and preemption) must be disabled. >>>> + * Preemption must be disabled. >>> >>> [*] That's not enough: we need to be in kernel_neon_disable()... >>> _enable() (i.e., kernel_neon_busy needs to be true to prevent softirqs >>> messing with the FPSIMD state). >> >> How about not mentioning preemption at all and just say: >> >> "The fpsimd context should be acquired before hand". >> >> This would help if we ever decide to protect critical section differently. > > Yes, or even better, name the function used to do this (i.e., > kernel_neon_disable() or get_cpu_fpsimd_context() or whatever it's > called). ... would make the comments simpler because we would have only one possible case to care. Cheers,
On Thu, Apr 11, 2019 at 04:58:41PM +0100, Julien Grall wrote: > Hi Dave, > > On 4/5/19 4:07 PM, Dave Martin wrote: > >On Fri, Apr 05, 2019 at 10:02:45AM +0100, Julien Grall wrote: > >>>>+#ifdef CONFIG_KERNEL_MODE_NEON > >>>>+ > >>>> /* > >>>> * may_use_simd - whether it is allowable at this time to issue SIMD > >>>> * instructions or access the SIMD register file > >>>>diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > >>>>index 5ebe73b69961..b7e5dac26190 100644 > >>>>--- a/arch/arm64/kernel/fpsimd.c > >>>>+++ b/arch/arm64/kernel/fpsimd.c > >>>>@@ -90,7 +90,8 @@ > >>>> * To prevent this from racing with the manipulation of the task's FPSIMD state > >>>> * from task context and thereby corrupting the state, it is necessary to > >>>> * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE > >>>>- * flag with local_bh_disable() unless softirqs are already masked. > >>>>+ * flag with kernel_neon_{disable, enable}. This will still allow softirqs to > >>>>+ * run but prevent them to use FPSIMD. > >>>> * > >>>> * For a certain task, the sequence may look something like this: > >>>> * - the task gets scheduled in; if both the task's fpsimd_cpu field > >>>>@@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state; > >>>> #endif /* ! CONFIG_ARM64_SVE */ > >>>>+static void kernel_neon_disable(void); > >>>>+static void kernel_neon_enable(void); > >>> > >>>I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE > >>>context access for the current context (i.e., makes it safe). > >>> > >>>Since these also disable/enable preemption, perhaps we can align them > >>>with the existing get_cpu()/put_cpu(), something like: > >>> > >>>void get_cpu_fpsimd_context(); > >>>vold put_cpu_fpsimd_context(); > >>> > >>>If you consider it's worth adding the checking helper I alluded to > >>>above, it could then be called something like: > >>> > >>>bool have_cpu_fpsimd_context(); > >> > >>I am not sure where you suggested a checking helper above. Do you refer to > >>exposing kernel_neon_busy even for !CONFIG_KERNEL_MODE_NEON? > > > >Hmmm, looks like I got my reply out of order here. > > > >I meant the helper (if any) to check > >!preemptible() && !__this_cpu_read(kernel_neon_busy). > > I guess you are using && instead of || because some of the caller may not > call get_cpu_fpsimd_context() before but still disable preemption, right? > > Wouldn't it be better to have all the user calling get_cpu_fpsimd_context() > and put_cpu_fpsimd_context()? > > This has the advantage to uniformize how the way FPSIMD is protected and > also... My expectation is that all users would have called get_cpu_fpsimd_context(). The reason for writing the check that way is that we can't meaningfully inspect percpu variables unless we are non-preemptible already. The && means we don't do the percpu read at all is the case where preemptible() is true. Or do you think my logic is wrong somewhere? (It's possible...) > >Looks like you inferred what I meant later on anyway. > > > >> > >>> > >>>>+ > >>>> /* > >>>> * Call __sve_free() directly only if you know task can't be scheduled > >>>> * or preempted. > >>>>@@ -213,11 +217,11 @@ static void sve_free(struct task_struct *task) > >>>> * thread_struct is known to be up to date, when preparing to enter > >>>> * userspace. > >>>> * > >>>>- * Softirqs (and preemption) must be disabled. > >>>>+ * Preemption must be disabled. > >>> > >>>[*] That's not enough: we need to be in kernel_neon_disable()... > >>>_enable() (i.e., kernel_neon_busy needs to be true to prevent softirqs > >>>messing with the FPSIMD state). > >> > >>How about not mentioning preemption at all and just say: > >> > >>"The fpsimd context should be acquired before hand". > >> > >>This would help if we ever decide to protect critical section differently. > > > >Yes, or even better, name the function used to do this (i.e., > >kernel_neon_disable() or get_cpu_fpsimd_context() or whatever it's > >called). > > ... would make the comments simpler because we would have only one possible > case to care. Agreed ---Dave
Hi Dave, On 4/11/19 5:34 PM, Dave Martin wrote: > On Thu, Apr 11, 2019 at 04:58:41PM +0100, Julien Grall wrote: >> Hi Dave, >> >> On 4/5/19 4:07 PM, Dave Martin wrote: >>> On Fri, Apr 05, 2019 at 10:02:45AM +0100, Julien Grall wrote: >>>>>> +#ifdef CONFIG_KERNEL_MODE_NEON >>>>>> + >>>>>> /* >>>>>> * may_use_simd - whether it is allowable at this time to issue SIMD >>>>>> * instructions or access the SIMD register file >>>>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >>>>>> index 5ebe73b69961..b7e5dac26190 100644 >>>>>> --- a/arch/arm64/kernel/fpsimd.c >>>>>> +++ b/arch/arm64/kernel/fpsimd.c >>>>>> @@ -90,7 +90,8 @@ >>>>>> * To prevent this from racing with the manipulation of the task's FPSIMD state >>>>>> * from task context and thereby corrupting the state, it is necessary to >>>>>> * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE >>>>>> - * flag with local_bh_disable() unless softirqs are already masked. >>>>>> + * flag with kernel_neon_{disable, enable}. This will still allow softirqs to >>>>>> + * run but prevent them to use FPSIMD. >>>>>> * >>>>>> * For a certain task, the sequence may look something like this: >>>>>> * - the task gets scheduled in; if both the task's fpsimd_cpu field >>>>>> @@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state; >>>>>> #endif /* ! CONFIG_ARM64_SVE */ >>>>>> +static void kernel_neon_disable(void); >>>>>> +static void kernel_neon_enable(void); >>>>> >>>>> I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE >>>>> context access for the current context (i.e., makes it safe). >>>>> >>>>> Since these also disable/enable preemption, perhaps we can align them >>>>> with the existing get_cpu()/put_cpu(), something like: >>>>> >>>>> void get_cpu_fpsimd_context(); >>>>> vold put_cpu_fpsimd_context(); >>>>> >>>>> If you consider it's worth adding the checking helper I alluded to >>>>> above, it could then be called something like: >>>>> >>>>> bool have_cpu_fpsimd_context(); >>>> >>>> I am not sure where you suggested a checking helper above. Do you refer to >>>> exposing kernel_neon_busy even for !CONFIG_KERNEL_MODE_NEON? >>> >>> Hmmm, looks like I got my reply out of order here. >>> >>> I meant the helper (if any) to check >>> !preemptible() && !__this_cpu_read(kernel_neon_busy). >> >> I guess you are using && instead of || because some of the caller may not >> call get_cpu_fpsimd_context() before but still disable preemption, right? >> >> Wouldn't it be better to have all the user calling get_cpu_fpsimd_context() >> and put_cpu_fpsimd_context()? >> >> This has the advantage to uniformize how the way FPSIMD is protected and >> also... > > My expectation is that all users would have called > get_cpu_fpsimd_context(). This is not the case today (see kvm_arch_vcpu_put_fp), I will look at protecting it with a call to get_cpu_fpsimd_context(). > > The reason for writing the check that way is that we can't meaningfully > inspect percpu variables unless we are non-preemptible already. The && > means we don't do the percpu read at all is the case where preemptible() > is true. I am not sure to understand why it would be necessary. this_cpu_read(kernel_neon_busy) should be sufficient here. If it is set then, preemption is disabled. Or are you worried about user directly setting kernel_neon_busy instead of calling get_cpu_fpsimd_context? > > Or do you think my logic is wrong somewhere? (It's possible...) I think your logic would not return the correct value. We want have_cpu_fpsimd_context() to return true if it is not preemptible and kernel_neon_busy is true. So we would want: !preemptible() && __this_cpu_read(kernel_neon_busy) If we speak about the implementation of have_cpu_fpsimd_context(), then we want: !preemptible() && __this_cpu_read(kernel_neon_busy) Cheers,
diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h index 6495cc51246f..94c0dac508aa 100644 --- a/arch/arm64/include/asm/simd.h +++ b/arch/arm64/include/asm/simd.h @@ -15,10 +15,10 @@ #include <linux/preempt.h> #include <linux/types.h> -#ifdef CONFIG_KERNEL_MODE_NEON - DECLARE_PER_CPU(bool, kernel_neon_busy); +#ifdef CONFIG_KERNEL_MODE_NEON + /* * may_use_simd - whether it is allowable at this time to issue SIMD * instructions or access the SIMD register file diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 5ebe73b69961..b7e5dac26190 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -90,7 +90,8 @@ * To prevent this from racing with the manipulation of the task's FPSIMD state * from task context and thereby corrupting the state, it is necessary to * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE - * flag with local_bh_disable() unless softirqs are already masked. + * flag with kernel_neon_{disable, enable}. This will still allow softirqs to + * run but prevent them to use FPSIMD. * * For a certain task, the sequence may look something like this: * - the task gets scheduled in; if both the task's fpsimd_cpu field @@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state; #endif /* ! CONFIG_ARM64_SVE */ +static void kernel_neon_disable(void); +static void kernel_neon_enable(void); + /* * Call __sve_free() directly only if you know task can't be scheduled * or preempted. @@ -213,11 +217,11 @@ static void sve_free(struct task_struct *task) * thread_struct is known to be up to date, when preparing to enter * userspace. * - * Softirqs (and preemption) must be disabled. + * Preemption must be disabled. */ static void task_fpsimd_load(void) { - WARN_ON(!in_softirq() && !irqs_disabled()); + WARN_ON(!preempt_count() && !irqs_disabled()); if (system_supports_sve() && test_thread_flag(TIF_SVE)) sve_load_state(sve_pffr(¤t->thread), @@ -238,7 +242,7 @@ void fpsimd_save(void) struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st); /* set by fpsimd_bind_task_to_cpu() or fpsimd_bind_state_to_cpu() */ - WARN_ON(!in_softirq() && !irqs_disabled()); + WARN_ON(!preempt_count() && !irqs_disabled()); if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) { if (system_supports_sve() && test_thread_flag(TIF_SVE)) { @@ -360,7 +364,7 @@ static int __init sve_sysctl_init(void) { return 0; } * task->thread.sve_state. * * Task can be a non-runnable task, or current. In the latter case, - * softirqs (and preemption) must be disabled. + * preemption must be disabled. * task->thread.sve_state must point to at least sve_state_size(task) * bytes of allocated kernel memory. * task->thread.uw.fpsimd_state must be up to date before calling this @@ -387,7 +391,7 @@ static void fpsimd_to_sve(struct task_struct *task) * task->thread.uw.fpsimd_state. * * Task can be a non-runnable task, or current. In the latter case, - * softirqs (and preemption) must be disabled. + * preemption must be disabled. * task->thread.sve_state must point to at least sve_state_size(task) * bytes of allocated kernel memory. * task->thread.sve_state must be up to date before calling this function. @@ -547,7 +551,7 @@ int sve_set_vector_length(struct task_struct *task, * non-SVE thread. */ if (task == current) { - local_bh_disable(); + kernel_neon_disable(); fpsimd_save(); set_thread_flag(TIF_FOREIGN_FPSTATE); @@ -558,7 +562,7 @@ int sve_set_vector_length(struct task_struct *task, sve_to_fpsimd(task); if (task == current) - local_bh_enable(); + kernel_neon_enable(); /* * Force reallocation of task SVE state to the correct size @@ -813,7 +817,7 @@ asmlinkage void do_sve_acc(unsigned int esr, struct pt_regs *regs) sve_alloc(current); - local_bh_disable(); + kernel_neon_disable(); fpsimd_save(); fpsimd_to_sve(current); @@ -825,7 +829,7 @@ asmlinkage void do_sve_acc(unsigned int esr, struct pt_regs *regs) if (test_and_set_thread_flag(TIF_SVE)) WARN_ON(1); /* SVE access shouldn't have trapped */ - local_bh_enable(); + kernel_neon_enable(); } /* @@ -892,7 +896,7 @@ void fpsimd_flush_thread(void) if (!system_supports_fpsimd()) return; - local_bh_disable(); + kernel_neon_disable(); memset(¤t->thread.uw.fpsimd_state, 0, sizeof(current->thread.uw.fpsimd_state)); @@ -935,7 +939,7 @@ void fpsimd_flush_thread(void) set_thread_flag(TIF_FOREIGN_FPSTATE); - local_bh_enable(); + kernel_neon_enable(); } /* @@ -947,9 +951,9 @@ void fpsimd_preserve_current_state(void) if (!system_supports_fpsimd()) return; - local_bh_disable(); + kernel_neon_disable(); fpsimd_save(); - local_bh_enable(); + kernel_neon_enable(); } /* @@ -1007,14 +1011,14 @@ void fpsimd_restore_current_state(void) if (!system_supports_fpsimd()) return; - local_bh_disable(); + kernel_neon_disable(); if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { task_fpsimd_load(); fpsimd_bind_task_to_cpu(); } - local_bh_enable(); + kernel_neon_enable(); } /* @@ -1027,7 +1031,7 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state) if (!system_supports_fpsimd()) return; - local_bh_disable(); + kernel_neon_disable(); current->thread.uw.fpsimd_state = *state; if (system_supports_sve() && test_thread_flag(TIF_SVE)) @@ -1038,7 +1042,7 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state) clear_thread_flag(TIF_FOREIGN_FPSTATE); - local_bh_enable(); + kernel_neon_enable(); } /* @@ -1055,11 +1059,28 @@ void fpsimd_flush_cpu_state(void) set_thread_flag(TIF_FOREIGN_FPSTATE); } -#ifdef CONFIG_KERNEL_MODE_NEON - DEFINE_PER_CPU(bool, kernel_neon_busy); EXPORT_PER_CPU_SYMBOL(kernel_neon_busy); +static void kernel_neon_disable(void) +{ + preempt_disable(); + WARN_ON(__this_cpu_read(kernel_neon_busy)); + __this_cpu_write(kernel_neon_busy, true); +} + +static void kernel_neon_enable(void) +{ + bool busy; + + busy = __this_cpu_xchg(kernel_neon_busy, false); + WARN_ON(!busy); /* No matching kernel_neon_disable()? */ + + preempt_enable(); +} + +#ifdef CONFIG_KERNEL_MODE_NEON + /* * Kernel-side NEON support functions */ @@ -1084,9 +1105,7 @@ void kernel_neon_begin(void) BUG_ON(!may_use_simd()); - local_bh_disable(); - - __this_cpu_write(kernel_neon_busy, true); + kernel_neon_disable(); /* Save unsaved fpsimd state, if any: */ fpsimd_save(); @@ -1094,9 +1113,7 @@ void kernel_neon_begin(void) /* Invalidate any task state remaining in the fpsimd regs: */ fpsimd_flush_cpu_state(); - preempt_disable(); - - local_bh_enable(); + kernel_neon_enable(); } EXPORT_SYMBOL(kernel_neon_begin); @@ -1111,15 +1128,10 @@ EXPORT_SYMBOL(kernel_neon_begin); */ void kernel_neon_end(void) { - bool busy; - if (!system_supports_fpsimd()) return; - busy = __this_cpu_xchg(kernel_neon_busy, false); - WARN_ON(!busy); /* No matching kernel_neon_begin()? */ - - preempt_enable(); + kernel_neon_enable(); } EXPORT_SYMBOL(kernel_neon_end);
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 used (i.e kernel_neon_busy is true). When in used, softirqs usually fallback 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. As a softirq should not rely on been able to use simd at a given time, there are limited reason to keep softirq disabled when touching the FPSIMD/SVE context. Instead, we can only disable preemption and tell the NEON unit is currently in use. This patch introduces two new helpers kernel_neon_{disable, enable} to mark the area using FPSIMD/SVE context and use them in replacement of local_bh_{disable, enable}. The functions kernel_neon_{begin, end} are also re-implemented to use the new helpers. Signed-off-by: Julien Grall <julien.grall@arm.com> --- I have been exploring this solution as an alternative approach to the RT patch "arm64: fpsimd: use preemp_disable in addition to local_bh_disable()". So far, the patch has only been lightly tested. For RT-linux, it might be possible to use migrate_{enable, disable}. I am quite new with RT and have some trouble to understand the semantics of migrate_{enable, disable}. So far, I am still unsure if it is possible to run another userspace task on the same CPU while getting preempted when the migration is disabled. --- arch/arm64/include/asm/simd.h | 4 +-- arch/arm64/kernel/fpsimd.c | 76 +++++++++++++++++++++++++------------------ 2 files changed, 46 insertions(+), 34 deletions(-)