Re: [PATCH 054/208] x86/fpu: Get rid of PF_USED_MATH usage, convert it to fpu->fpstate_active

Ingo Molnar
2015-05-06T03:24:20Z

* Andy Lutomirski <luto@amacapital.net> wrote:

> > diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> > index efb520dcf38e..f6317d9aa808 100644
> > --- a/arch/x86/include/asm/fpu/types.h
> > +++ b/arch/x86/include/asm/fpu/types.h
> > @@ -137,6 +137,12 @@ struct fpu {
> > * deal with bursty apps that only use the FPU for a short time:
> > */
> > unsigned char counter;
> > + /*
> > + * This flag indicates whether this context is fpstate_active: if the task is
> > + * not running then we can restore from this context, if the task
> > + * is running then we should save into this context.
> > + */
> > + unsigned char fpstate_active;
>
> I don't understand. What does it mean if !fpstate_active?

Yeah, so this was just the 'simple' migration patch from PF_USED_FPU
to ->fpstate_active.

At the end of the series those fields get more love and a more
detailed explanation:

/*
* @fpstate_active:
*
* This flag indicates whether this context is active: if the task
* is not running then we can restore from this context, if the task
* is running then we should save into this context.
*/
unsigned char fpstate_active;

and its interaction with fpregs_active is explained as well:

/*
* @fpregs_active:
*
* This flag determines whether a given context is actively
* loaded into the FPU's registers and that those registers
* represent the task's current FPU state.
*
* Note the interaction with fpstate_active:
*
* # task does not use the FPU:
* fpstate_active == 0
*
* # task uses the FPU and regs are active:
* fpstate_active == 1 && fpregs_active == 1
*
* # the regs are inactive but still match fpstate:
* fpstate_active == 1 && fpregs_active == 0 && fpregs_owner == fpu
*
* The third state is what we use for the lazy restore optimization
* on lazy-switching CPUs.
*/
unsigned char fpregs_active;

Basically the 'fpstate' is the in-memory FPU state, and if it's
active, it means it can be copied to (saved to) and copied from
(restored from). Whether this fpstate is the currently representative
FPU state depends on the other state flag(s), as described.

Maybe I should have broken out the fourth state as well:

* # the fpstate holds all of a task's FPU state:
* fpstate_active == 1 && fpregs_active == 0 && fpregs_owner != fpu

?

active/inactive was one idiom that I felt worked pretty well - but I
considered others as well:

- dirty/clean (didn't work so well and too MM-ish)
- valid/invalid (likewise)
- used/unused (yuck)

Note:

There's a fifth valid state as well, but I did not want
to complicate the description even more: kernel_user_begin()/end()
users create this state with its own private in_kernel_fpu flag, in
that they use FPU registers but don't touch these (user-)flags.
kernel_user_begin()/end() is atomic and (beyond zapping pending lazy
restore state in fpu_fpregs_owner_ctx) it restores the FPU to the
previous state so it's pretty orthogonal as far as the other states
are concerned.

Note2:

I also considered renaming kernel_fpu_begin()/end() to the new
nomenclature, but it has a good name and I did not want too much
churn with a well-established API, which also mirrors
user_fpu_begin() conceptually. I also couldn't find a better name:
maybe fpu__kernel_save()/restore(), but that felt a bit strained.

Does this make things clearer? I can work on it some more if I got it
wrong or if the text is confusing somewhere, this is crucial IMHO.

Instead of binary states we could also unify them into a single state
variable - didn't find any really convincing naming concept for that
though, mostly because I think those states are fundamentally
separate, just interrelated.

Thanks,

Ingo