Re: [Xen-devel] [RFC PATCH v1 02/21] x86: NUMA: Refactor NUMA code

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 09/02/17 16:11, Jan Beulich wrote:
> > > > On 09.02.17 at 16:56, <vijay.kilari@xxxxxxxxx> wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> >
> > Move common generic NUMA code to xen/common/numa.c from
> > xen/arch/x86/numa.c. Also move generic code in header file
> > xen/include/asm-x86/numa.h to xen/include/xen/numa.h
> >
> > This common code can be re-used later for ARM.
> >
> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> I would have been nice if you Cc-ed the maintainers of the code
> you're moving.
>
> > --- /dev/null
> > +++ b/xen/common/numa.c
> > @@ -0,0 +1,342 @@
> > +/*
> > + * Common NUMA handling functions for x86 and arm.
> > + * Original code extracted from arch/x86/numa.c
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +
> > +#include <xen/mm.h>
> > +#include <xen/string.h>
> > +#include <xen/init.h>
> > +#include <xen/ctype.h>
> > +#include <xen/nodemask.h>
> > +#include <xen/numa.h>
> > +#include <xen/keyhandler.h>
> > +#include <xen/time.h>
> > +#include <xen/smp.h>
> > +#include <xen/pfn.h>
> > +#include <xen/sched.h>
> > +#include <xen/errno.h>
> > +#include <xen/softirq.h>
> > +#include <asm/setup.h>
> This last one would better not be included here.
>
> > +struct node_data node_data[MAX_NUMNODES];
> > +
> > +/* Mapping from pdx to node id */
> > +int memnode_shift;
> > +unsigned long memnodemapsize;
> > +u8 *memnodemap;
> > +typeof(*memnodemap) _memnodemap[64];
> Careful with removing any "static" please. For the last one in
> particular you would need to change the name if it's really necessary
> to make non-static. Even better would be though to keep it static
> and provide suitable accessors.
>
> Also please sanitize types as you're moving stuff: memnode_shift
> looks like it really wants to be unsigned int, and u8 should really
> be uint8_t (as we're trying to phase out u8 & Co). This also applies
> to code further down.
I agree with the changes suggested. If possible I would prefer all those 
changes made in separate patches before the move. This would ease the 
review.
Cheers,

--
Julien Grall

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