Re: [PATCH][RFC] FPU LWP 1/5: clean up the FPU code

[Keir Fraser <keir@xen.org>]

On 14/04/2011 21:37, "Wei Huang" <wei.huang2@amd.com> wrote:

> This patch reorganizes and cleans up the FPU code. Main changes include:
> 
> 1. Related functions are re-arranged together.
> 2. FPU save/restore code are extracted out as independent functions
> (fsave/frstor, fxsave/fxrstor, xsave/xrstor).
> 3. Two FPU entry functions, fpu_save() and fpu_restore(), are defined.
> They utilize xsave/xrstor to save/restore FPU state if CPU supports
> extended state. Otherwise they fall back to fxsave/fxrstor or fsave/frstor.

Wei,

If you're going to clean this up, please don't politely skirt around the
existing file- and function-name conventions. The fact is that i387/fpu is
but one item of state handled by XSAVE/XRSTOR and friends. Thus it is the
tail wagging the dog to keep all the new mechanism in files and functions
called {i387,fpu}*. I'd prefer you to move some or all functionality into
newly-named files and functions -- perhaps using a prefix like xstate_ on
functions and sticking it all in files xstate.[ch]. Perhaps the old i387
files would disappear completely, or perhaps you'll find a few bits and
pieces logically remain in them, I don't mind either way as long as
everything is in a logical place with a logical name.

The naming in your 3rd patch is also unnecessarily confusing: is it really
clear the difference between *_reload and *_restore, for example, that one
is done on context switch and the other on #NM? We need better names; for
example:
 xstate_save: Straightforward I guess as we always unlazily save everything
that's dirty straight away during context switch.
 xstate_restore_lazy: Handle stuff we restore lazily on #NM.
 xstate_restore_eager: Handle stuff we restore unconditionally (if in use by
the guest) on ctxt switch.

These names would mean a lot more to me than what you chose. However, feel
free to work out a comprehensive naming scheme that you think works best.
All I'm saying is that our current naming scheme is already pretty crappy,
and you make it quite a bit worse by following the existing direction way
too much!

 -- Keir

> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel