Re: [PATCH V3] LoongArch: Provide kernel fpu functions

[Huacai Chen]

Hi, David,

On Tue, Mar 7, 2023 at 1:17 AM David Laight <David.Laight@xxxxxxxxxx> wrote:
>
> From: Huacai Chen
> > Sent: 06 March 2023 10:00
> >
> > Provide kernel_fpu_begin()/kernel_fpu_end() to allow the kernel itself
> > to use fpu. They can be used by some other kernel components, e.g., the
> > AMDGPU graphic driver for DCN.
> >
> ...
> > diff --git a/arch/loongarch/kernel/kfpu.c b/arch/loongarch/kernel/kfpu.c
> > new file mode 100644
> > index 000000000000..cd2a18fecdcc
> > --- /dev/null
> > +++ b/arch/loongarch/kernel/kfpu.c
> > @@ -0,0 +1,41 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2023 Loongson Technology Corporation Limited
> > + */
> > +
> > +#include <linux/cpu.h>
> > +#include <linux/init.h>
> > +#include <asm/fpu.h>
> > +#include <asm/smp.h>
> > +
> > +static DEFINE_PER_CPU(bool, in_kernel_fpu);
> > +
> > +void kernel_fpu_begin(void)
> > +{
> > +     if (this_cpu_read(in_kernel_fpu))
> > +             return;
>
> Isn't this check entirely broken?
> It absolutely needs to be inside the preempt_disable().
Yes, you are right, this check should be after preempt_disable().

> If there are nested requests then fpu use is disabled by the first
> kernel_fpu_end() call.
This check should be changed to WARN_ON() as x86 does since nested
requests are unexpected use cases.

>
> > +
> > +     preempt_disable();
> > +     this_cpu_write(in_kernel_fpu, true);
> > +
> > +     if (!is_fpu_owner())
> > +             enable_fpu();
> > +     else
> > +             _save_fp(&current->thread.fpu);
> > +}
>
> More interestingly, unless the kernel is doing the kind of
> 'lazy fpu switch' that x86 used to do (not sure it still does in Linux)
> where the fpu registers can contain values for a different process
> isn't it actually enough for kernel_fpu_begin() to just be:
>
>         preempt_disable();
>         if (current->fpu_regs_live)
I think this condition is the same as is_fpu_owner(). Moreover,
LoongArch doesn't allow fpu-disabled exception occured in kernel, so
we should make sure fpu is enabled at kernel_fpu_begin().

>                 __save_fp(current);
>         preempt_enable();
>
> and for kernel_fpu_end() to basically be a nop.
>
> Then rely on the 'return to user' path to pick up the
> live fpu registers from the save area.
>
> After all, you pretty much don't want to load the fpu regs
> every time a process wakes up and goes back to sleep without
> returning to user.
I think this is not a common case, so it isn't worthy to modify many
files to optimize for now.

Huacai
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>