Re: [Xenomai] [PATCH 1/2] hal/x86: fix kernel-space FPU with 4.1 kernel

[Henning Schild <henning.schild@siemens.com>]

Am Tue, 25 Oct 2016 18:54:52 +0200
schrieb Philippe Gerum <rpm@xenomai.org>:

> On 10/25/2016 05:37 PM, Henning Schild wrote:
> > Am Tue, 25 Oct 2016 15:26:09 +0200
> > schrieb Philippe Gerum <rpm@xenomai.org>:
> >   
> >> On 10/24/2016 07:39 PM, Henning Schild wrote:  
> >>> Starting at kernel 4.1 we have a per-cpu flag for kernel fpu and
> >>> do not abuse the flag of current anymore. This patch is a minimal
> >>> fix for something that is hard to get your head around, but
> >>> xenomai 2.6 will not go beyond 4.1. So not effort to fix or
> >>> document what is going on ...
> >>>
> >>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> >>> ---
> >>>  include/asm-x86/bits/pod.h | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/include/asm-x86/bits/pod.h
> >>> b/include/asm-x86/bits/pod.h index 678720a..8a16713 100644
> >>> --- a/include/asm-x86/bits/pod.h
> >>> +++ b/include/asm-x86/bits/pod.h
> >>> @@ -58,6 +58,10 @@ static inline void
> >>> xnarch_leave_root(xnarchtcb_t *rootcb) rootcb->spp =
> >>> &current->thread.x86reg_sp; rootcb->ipp = &current->thread.rip;
> >>>  #endif
> >>> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
> >>> +	if (kernel_fpu_disabled())
> >>> +		wrap_clear_fpu_used(current);
> >>> +#endif
> >>>  	rootcb->ts_usedfpu = !!wrap_test_fpu_used(current);    
> >>
> >> Does this fix work with regular RAID/crypto/mmx drivers enabled and
> >> active for instance?  
> > 
> > I did not try the actual RAID, but i tested with a kernel that runs
> > the switchtest in the kernel (!CONFIG_RAID456 ...). And i saw
> > primary mode interrupt secondary kernel doing fpu. In fact that is
> > exactly the case the patch is for.
> >  
> 
> Ack, provided the preempted code is within a section bracketed by
> fp_linux_begin/end() calls Xenomai-wise.

Yes they are bracketed.

> >> I would rather invert that logic, i.e. if !kernel_fpu_disabled(),
> >> then set_fpu_used(). Otherwise we might not notice when Xenomai
> >> preempts some regular kernel code which hijacked the fpu from
> >> kernel space. 
> > 
> > kernel_fpu_disabled() means that the kernel is currently using the
> > fpu, the name is misleading.  
> 
> Correct, I misread, my bad.
> 
>  With the kernel having its own per-cpu flag
> > we can now get the state where primary mode sees a state where the
> > kernel and the current task are "using" the fpu because both their
> > soft-flags are on. And if we call __switch_to linux code will save
> > the kernel fpu context into the task state save area, it does not
> > look at the kernel-flag since kernel-fpu cannot be interrupted in
> > regular linux.
> > 
> > Does that answer your question?
> >   
> 
> The reason to use a separate fpu area for saving the in-kernel fpu
> context Xenomai has preempted is clear, otherwise we would trash the
> current task's one upon root->xenomai transition, which I believe
> matches your description above. However we really have to make sure
> that every path leading to testing TS_USED_FPU either goes through
> that fix, or is not concerned by the root->xenomai transition.
> 
> Hence my question about the current testing level of that fix, because
> dealing with in-kernel fpu usage is a complete mess, and we had our
> share of breakage in the past, particularly when running with mmx
> helpers and RAID arrays.

The testing level is running the switchtest, also in kernel. That
should cover all possible cases. Gilles told me, that once this runs
the fpu handling should be correct.

> This said, I don't see any reason why this
> fix would make the situation worse, it can only make it better, which
> is a win since this code is not maintained anymore, so I'll merge it.

The patch kind of makes it worse because it introduces even more
non-obvious logic. But since the code is not maintained anymore i did
not invest in documentation or restructuring. Instead i chose the
minimal fix that keeps the old code as is.

Thanks for merging, and for starting this discussion. I did not write a
verbose commit comment, these mails in the archives might help if
anyone ever has to look into that again.

Henning