Re: [Xen-devel] [PATCH RFC 04/55] x86/mm: introduce l{1, 2}t local variables to map_pages_to_xen

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Apr 09, 2019 at 06:49:34AM -0600, Jan Beulich wrote:
> >>> On 09.04.19 at 14:22, <wei.liu2@xxxxxxxxxx> wrote:
> > On Fri, Mar 15, 2019 at 09:36:37AM -0600, Jan Beulich wrote:
> >> >>> On 07.02.19 at 17:44, <wei.liu2@xxxxxxxxxx> wrote:
> >> > The pl2e and pl1e variables are heavily (ab)used in that function. It
> >> > is fine at the moment because all page tables are always mapped so
> >> > there is no need to track the life time of each variable.
> >> > 
> >> > We will soon have the requirement to map and unmap page tables. We
> >> > need to track the life time of each variable to avoid leakage.
> >> > 
> >> > Introduce some l{1,2}t variables with limited scope so that we can
> >> > track life time of pointers to xen page tables more easily.
> >> 
> >> But you retain some uses of the old variables, and to be honest it's
> >> not really clear to me by what criteria (and having multiple instances
> >> of a variable name in a single function isn't necessarily less confusing).
> >> I think we either stick to what's there (doesn't look too bad to me),
> >> or you switch to scope restricted page table pointers throughout the
> >> function, such that the function scope symbols can go away
> >> altogether).
> > 
> > I thought my commit message was clear enough: it helped tracking the
> > lifetime of pointers more easily. There is nothing new in this trick:
> > the less variables of the same name the better.
> > 
> > In this patch, places where it is clear that using local scope variables
> > suffices are changed to use local variables.
> > 
> > In the end, pl*e are only used to point to entries which point to the
> > page tables related to the linear address being mapped. They won't be
> > used to point to any intermediate pointers which are used to manipulate
> > page tables.
> > 
> > We can't eliminate function scope variables because they need to stay
> > function scope -- see later patches where they are unmapped in the out
> > path.
> 
> Except that I'm not really happy with that approach either (not
> only but also because of the proliferation of goto-s).

The root cause is there are far too many exit paths in this function.
Previously it was okay because we didn't need to clean up. It won't be
okay once we have to.

I picked the option to unify them into one or two places.

One option is to duplicate cleanup code in each exit path. That's
repetitive and error-prone IMHO.

One option is to break up map_pages_to_xen into smaller functions which
don't require gotos but require more mapping / unmapping. I thought
about that but it was too much faff without getting us to where we
wanted to be.

Pick your poison. There isn't an easy solution to this...

Wei.

> 
> Jan
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel