Re: [Xen-devel] [PATCH v3 3/4] libxc: rework vnuma bits in setup_guest

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 03, 2015 at 11:57:33AM +0100, Ian Campbell wrote:
> On Wed, 2015-06-03 at 11:39 +0100, Wei Liu wrote:
> > On Wed, Jun 03, 2015 at 11:34:49AM +0100, Ian Campbell wrote:
> > > On Mon, 2015-06-01 at 16:18 -0400, Boris Ostrovsky wrote:
> > > > On 06/01/2015 06:19 AM, Wei Liu wrote:
> > > > > Make the setup process similar to PV counterpart. That is, to 
> > > > > allocate a
> > > > > P2M array that covers the whole memory range and start from there. 
> > > > > This
> > > > > is clearer than using an array with no holes in it.
> > > > >
> > > > > Also the dummy layout should take MMIO hole into consideration. We 
> > > > > might
> > > > > end up having two vmemranges in the dummy layout.
> > > > >
> > > > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > > 
> > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > > 
> > > + my ack on all 4 -> applied.
> > > 
> > > > with a couple of nits below that you might consider.
> > > 
> > > These can be done in followups, so I applied.
> > > 
> > > I have a passing comment on one of the nits:
> > > 
> > > > > +    for ( vmemid = 0; vmemid < args->nr_vmemranges; vmemid++ )
> > > > > +    {
> > > > > +        uint64_t pfn;
> > > > > +
> > > > > +        for ( pfn = args->vmemranges[vmemid].start >> PAGE_SHIFT;
> > > > > +              pfn < args->vmemranges[vmemid].end >> PAGE_SHIFT;
> > > > > +              pfn++ )
> > > > > +            page_array[pfn] = pfn;
> > > > > +    }
> > > > >   
> > > > >       /*
> > > > >        * Try to claim pages for early warning of insufficient memory 
> > > > > available.
> > > > > @@ -645,6 +679,12 @@ static int setup_guest(xc_interface *xch,
> > > > >    error_out:
> > > > >       rc = -1;
> > > > >    out:
> > > > > +    if ( use_dummy )
> > > > 
> > > > Or 'if (args->vmemranges == dummy_vmemrange)' and drop use_dummy 
> > > > variable.
> > > 
> > > FWIW a more normal approach would be to do all the calculations on local
> > > variables and propagate them to the caller in the !use_dummy case. I
> > > don't know if that implies some huge restructure of this function
> > > though.
> > 
> > There is no calculation with regard to vNUMA that needs to be propagated
> > to the caller in !use_dummy case. So this point is moot.
> 
> The code returns with args->{nr_vnodes,vmemranges,vnode_to_pnode} having
> been updated, if that information should not be propoagated to the
> caller then surely it should be blown away irrespective of the dummy or
> not (and in that case why is it being done in that struct at all and not
> in some local variables freed on exit)
> 

That's because the code below references those fields in args and
nothing more. I can refactor this function to have local pointers either
point to dummy or args->*.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel