kernel mode NEON support

arch/arm/vfp/vfpmodule.c +3 -1

+
+#endif /* CONFIG_KERNEL_MODE_NEON */
+
 /*
  * VFP support code initialisation.
  */
--

1.8.1.2

Replying to self:

arch/arm/vfp/vfpmodule.c void vfp_kmode_exception(void) · line 712

Quoted 8 lines · Ard Biesheuvel

arch/arm/vfp/vfpmodule.c +8 -0

+void kernel_neon_end(void)
+{
+       /* Disable the NEON/VFP unit. */
+       fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+       barrier();
+       dec_preempt_count();
+}
+EXPORT_SYMBOL(kernel_neon_end);

Meh. This is not going to please the RT crowd, as preempt_schedule() will not be called on PREEMPT builds in this case.

Propose to replace it with

    preempt_enable();
#ifndef CONFIG_PREEMPT_COUNT
    /* in this case, the preempt_enable() right above is just a barrier() */
    dec_preempt_count();

#endif

(and the converse in kernel_neon_begin())

Quoted 23 lines · Ard Biesheuvel

Replying to self:

arch/arm/vfp/vfpmodule.c void vfp_kmode_exception(void) · line 712

Quoted 8 lines · Ard Biesheuvel

arch/arm/vfp/vfpmodule.c +8 -0

+void kernel_neon_end(void)
+{
+       /* Disable the NEON/VFP unit. */
+       fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+       barrier();
+       dec_preempt_count();
+}
+EXPORT_SYMBOL(kernel_neon_end);

Meh. This is not going to please the RT crowd, as preempt_schedule() will not be called on PREEMPT builds in this case.

Propose to replace it with

    preempt_enable();
#ifndef CONFIG_PREEMPT_COUNT
    /* in this case, the preempt_enable() right above is just a barrier() */
    dec_preempt_count();

#endif

Quoted 1 line

(and the converse in kernel_neon_begin())

Yuck, that's ugly as sin! How does x86 deal with this? Looking at kernel_fpu_{begin,end}, they just disable preemption so I guess that they assume the caller is non-blocking? There's an aside about the use of preempt-notifiers for KVM, so it does sound like the onus is on the caller not to shoot themselves in the face.

Quoted 9 lines · Will Deacon

Quoted 8 lines · Ard Biesheuvel

Propose to replace it with

    preempt_enable();
#ifndef CONFIG_PREEMPT_COUNT
    /* in this case, the preempt_enable() right above is just a barrier() */
    dec_preempt_count();

#endif

Quoted 8 lines

Quoted 1 line

(and the converse in kernel_neon_begin())

Yuck, that's ugly as sin! How does x86 deal with this? Looking at kernel_fpu_{begin,end}, they just disable preemption so I guess that they assume the caller is non-blocking? There's an aside about the use of preempt-notifiers for KVM, so it does sound like the onus is on the caller not to shoot themselves in the face.

Even if x86 doesn't care about this, do you really think we should take the risk of silently clobbering the NEON registers if the caller does something that may end up sleeping? Anyway, I don't remember exactly who suggested using inc_preempt_count() directly, but doing so brings about the responsibility of calling preempt_schedule() when leaving the critical section, and just using both (without the #ifdef) is also not an option.

So can you suggest a better way of making sure schedule_debug() shoots us down if calling schedule() between kernel_neon_begin and kernel_neon_end, even on non-preempt builds?

Quoted 3 lines · Ard Biesheuvel

So can you suggest a better way of making sure schedule_debug() shoots us down if calling schedule() between kernel_neon_begin and kernel_neon_end, even on non-preempt builds?

With what we currently have in the kernel, no, I can't think of a better way. However, I also don't think that smuggling in a back-end hack is a good idea either. How about we follow x86's lead on this and rely on the caller not to sleep for the timebeing? Then, separately to this patch series, you could look at augmenting the scheduler so that schedule_debug can complain if it encounters a task that is not expected to sleep? That seems like the right place to fix this problem, and will benefit other architectures too.

Quoted 5 lines · Will Deacon

Quoted 3 lines · Ard Biesheuvel

So can you suggest a better way of making sure schedule_debug() shoots us down if calling schedule() between kernel_neon_begin and kernel_neon_end, even on non-preempt builds?

Quoted 8 lines

With what we currently have in the kernel, no, I can't think of a better way. However, I also don't think that smuggling in a back-end hack is a good idea either. How about we follow x86's lead on this and rely on the caller not to sleep for the timebeing? Then, separately to this patch series, you could look at augmenting the scheduler so that schedule_debug can complain if it encounters a task that is not expected to sleep? That seems like the right place to fix this problem, and will benefit other architectures too.

Good point. As preempt_enable/disable already have this side effect on PREEMPT builds, perhaps it wouldn't be such a bad idea to modify them in the non-PREEMPT case to at least complain if schedule() is invoked during such a section.

@Russell: you mentioned spinlocks at some point to prevent sleeping. Are you ok with Will's suggestion instead, i.e., to rely on preempt_disable() to do the right thing, and fix it later because currently, it doesn't on non-PREEMPT?

--
Ard.

Quoted 10 lines · Ard Biesheuvel

Quoted 8 lines · Will Deacon

With what we currently have in the kernel, no, I can't think of a better way. However, I also don't think that smuggling in a back-end hack is a good idea either. How about we follow x86's lead on this and rely on the caller not to sleep for the timebeing? Then, separately to this patch series, you could look at augmenting the scheduler so that schedule_debug can complain if it encounters a task that is not expected to sleep? That seems like the right place to fix this problem, and will benefit other architectures too.

Quoted 5 lines

Good point. As preempt_enable/disable already have this side effect on PREEMPT builds, perhaps it wouldn't be such a bad idea to modify them in the non-PREEMPT case to at least complain if schedule() is invoked during such a section.

It appears we have this already, but in the non-PREEMPT case, it needs CONFIG_DEBUG_ATOMIC_SLEEP to be defined.
Let's just rely on preempt_disable() to do the right thing...

Quoted 17 lines · Ard Biesheuvel

Quoted 10 lines · Ard Biesheuvel

Quoted 8 lines · Will Deacon

With what we currently have in the kernel, no, I can't think of a better way. However, I also don't think that smuggling in a back-end hack is a good idea either. How about we follow x86's lead on this and rely on the caller not to sleep for the timebeing? Then, separately to this patch series, you could look at augmenting the scheduler so that schedule_debug can complain if it encounters a task that is not expected to sleep? That seems like the right place to fix this problem, and will benefit other architectures too.

Quoted 5 lines

Good point. As preempt_enable/disable already have this side effect on PREEMPT builds, perhaps it wouldn't be such a bad idea to modify them in the non-PREEMPT case to at least complain if schedule() is invoked during such a section.

Quoted 4 lines

It appears we have this already, but in the non-PREEMPT case, it needs CONFIG_DEBUG_ATOMIC_SLEEP to be defined.
Let's just rely on preempt_disable() to do the right thing...

OK, just one more question before I respin the next (hopefully final) version:
if a caller does sleep after calling kernel_neon_begin() (and thus receives no warning if he runs a non-PREEMPT build with CONFIG_DEBUG_ATOMIC_SLEEP disabled), he will most likely find the NEON/VFP unit disabled after waking up (as we disable it on a context switch), so any subsequent NEON instructions will trigger the undef handler.

Should I perhaps expand the vfp_kmode_exception() function which gets invoked in this case to be more helpful in identifying this condition? Currently it just BUG()s on conditions that indicate dependence on support code, and reports an undefined instruction otherwise.

Quoted 17 lines · Ard Biesheuvel

Quoted 10 lines · Ard Biesheuvel

Quoted 8 lines · Will Deacon

With what we currently have in the kernel, no, I can't think of a better way. However, I also don't think that smuggling in a back-end hack is a good idea either. How about we follow x86's lead on this and rely on the caller not to sleep for the timebeing? Then, separately to this patch series, you could look at augmenting the scheduler so that schedule_debug can complain if it encounters a task that is not expected to sleep? That seems like the right place to fix this problem, and will benefit other architectures too.

Quoted 5 lines

Good point. As preempt_enable/disable already have this side effect on PREEMPT builds, perhaps it wouldn't be such a bad idea to modify them in the non-PREEMPT case to at least complain if schedule() is invoked during such a section.

Quoted 3 lines

It appears we have this already, but in the non-PREEMPT case, it needs CONFIG_DEBUG_ATOMIC_SLEEP to be defined.
Let's just rely on preempt_disable() to do the right thing...

OK, just one more question before I respin the next (hopefully final) version:
if a caller does sleep after calling kernel_neon_begin() (and thus receives no warning if he runs a non-PREEMPT build with CONFIG_DEBUG_ATOMIC_SLEEP disabled), he will most likely find the NEON/VFP unit disabled after waking up (as we disable it on a context switch), so any subsequent NEON instructions will trigger the undef handler.

Quoted 7 lines · Ard Biesheuvel

OK, just one more question before I respin the next (hopefully final) version:
if a caller does sleep after calling kernel_neon_begin() (and thus receives no warning if he runs a non-PREEMPT build with CONFIG_DEBUG_ATOMIC_SLEEP disabled), he will most likely find the NEON/VFP unit disabled after waking up (as we disable it on a context switch), so any subsequent NEON instructions will trigger the undef handler.

Can you check on the VFP context switch path whether kernel_neon_begin() has been called and we are moving away from the task? You could even store the LR in kernel_neon_begin() to give better error information.

Quoted 7 lines · Ard Biesheuvel

OK, just one more question before I respin the next (hopefully final) version:
if a caller does sleep after calling kernel_neon_begin() (and thus receives no warning if he runs a non-PREEMPT build with CONFIG_DEBUG_ATOMIC_SLEEP disabled), he will most likely find the NEON/VFP unit disabled after waking up (as we disable it on a context switch), so any subsequent NEON instructions will trigger the undef handler.

Can you check on the VFP context switch path whether kernel_neon_begin() has been called and we are moving away from the task? You could even store the LR in kernel_neon_begin() to give better error information.

Replying to self:

arch/arm/vfp/vfpmodule.c void vfp_kmode_exception(void) · line 712

Quoted 8 lines · Ard Biesheuvel

arch/arm/vfp/vfpmodule.c +8 -0

+void kernel_neon_end(void)
+{
+       /* Disable the NEON/VFP unit. */
+       fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+       barrier();
+       dec_preempt_count();
+}
+EXPORT_SYMBOL(kernel_neon_end);

Meh. This is not going to please the RT crowd, as preempt_schedule() will not be called on PREEMPT builds in this case.

Propose to replace it with

    preempt_enable();

#ifndef CONFIG_PREEMPT_COUNT

    /* in this case, the preempt_enable() right above is just a barrier() */
    dec_preempt_count();

#endif

Quoted 5 lines · Ard Biesheuvel

Meh. This is not going to please the RT crowd, as preempt_schedule() will not be called on PREEMPT builds in this case.

Propose to replace it with

Quoted 2 lines

    preempt_enable();

#ifndef CONFIG_PREEMPT_COUNT

if (IS_ENABLED(CONFIG_xxx)) at least

Quoted 3 lines

    /* in this case, the preempt_enable() right above is just a barrier() */
    dec_preempt_count();

#endif

but why do you need to call inc_preempt and dec directly?

Quoted 13 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 7 lines · Ard Biesheuvel

Meh. This is not going to please the RT crowd, as preempt_schedule() will not be called on PREEMPT builds in this case.

Propose to replace it with

    preempt_enable();

#ifndef CONFIG_PREEMPT_COUNT

if (IS_ENABLED(CONFIG_xxx)) at least

Quoted 3 lines

    /* in this case, the preempt_enable() right above is just a barrier() */
    dec_preempt_count();

#endif

Quoted 1 line

but why do you need to call inc_preempt and dec directly?

There is a concern that violations of the rule that a task should not sleep between kernel_neon_begin and kernel_neon_end calls may not be spotted on non-PREEMPT builds that don't have CONFIG_DEBUG_ATOMIC_SLEEP set.

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 7 lines · Ard Biesheuvel

Meh. This is not going to please the RT crowd, as preempt_schedule() will not be called on PREEMPT builds in this case.

Propose to replace it with

    preempt_enable();

#ifndef CONFIG_PREEMPT_COUNT

if (IS_ENABLED(CONFIG_xxx)) at least

Quoted 3 lines

    /* in this case, the preempt_enable() right above is just a barrier() */
    dec_preempt_count();

#endif

Quoted 6 lines

Quoted 1 line

but why do you need to call inc_preempt and dec directly?

There is a concern that violations of the rule that a task should not sleep between kernel_neon_begin and kernel_neon_end calls may not be spotted on non-PREEMPT builds that don't have CONFIG_DEBUG_ATOMIC_SLEEP set.

Would an explicit call to schedule() trigger with CONFIG_DEBUG_ATOMIC_SLEEP? It looks that this config option only triggers for explicit might_sleep() calls but we don't have one for explicit schedule() calls (cond_resched() call has might_sleep()).