Re: [Xen-devel] [RFC PATCH v2 10/25] x86: NUMA: Move numa code and make it generic

[Julien Grall <julien.grall@xxxxxxx>]

On 05/09/2017 08:36 AM, Vijay Kilari wrote:
> On Mon, May 8, 2017 at 10:11 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi Vijay,
> >
> >
> > On 28/03/17 16:53, vijay.kilari@xxxxxxxxx wrote:
> > > diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> > > index 3bdab9a..33c6806 100644
> > > --- a/xen/arch/x86/numa.c
> > > +++ b/xen/arch/x86/numa.c
> > > @@ -10,286 +10,13 @@
> > >  #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 <asm/acpi.h>
> > > -#include <xen/sched.h>
> > > -#include <xen/softirq.h>
> > > -
> > > -static int numa_setup(char *s);
> > > -custom_param("numa", numa_setup);
> > > -
> > > -struct node_data node_data[MAX_NUMNODES];
> > > -
> > > -/* Mapping from pdx to node id */
> > > -unsigned int memnode_shift;
> > > -static typeof(*memnodemap) _memnodemap[64];
> > > -unsigned long memnodemapsize;
> > > -uint8_t *memnodemap;
> > > -
> > > -nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
> > > -    [0 ... NR_CPUS-1] = NUMA_NO_NODE
> > > -};
> > > -/*
> > > - * Keep BIOS's CPU2node information, should not be used for memory
> > > allocaion
> > > - */
> > > -nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
> > > -    [0 ... MAX_LOCAL_APIC-1] = NUMA_NO_NODE
> > > -};
> >
> > Why this is moved in this patch from here to x86/srat.c?
> This is x86 specific. I will make a separate patch for this
> move.
But x86/numa.c is specific specific.... So why do you move it????

> > [...]
> >
> > > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> > > index 7cf4771..2cc87a3 100644
> > > --- a/xen/arch/x86/srat.c
> > > +++ b/xen/arch/x86/srat.c
> > > @@ -27,6 +27,13 @@ static nodemask_t __initdata memory_nodes_parsed;
> > >  static nodemask_t __initdata processor_nodes_parsed;
> > >  static struct node __initdata nodes[MAX_NUMNODES];
> > >
> > > +/*
> > > + * Keep BIOS's CPU2node information, should not be used for memory
> > > allocaion
> > > + */
> > > +nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
> > > +    [0 ... MAX_LOCAL_APIC-1] = NUMA_NO_NODE
> > > +};
> > > +
> >
> > This does not belong to this patch...
> Ok
> > >  struct pxm2node {
> > >         unsigned int pxm;
> > >         nodeid_t node;
> >
> > [...]
> >
> >
> > > diff --git a/xen/common/numa.c b/xen/common/numa.c
> > > new file mode 100644
> > > index 0000000..207ebd8
> > > --- /dev/null
> > > +++ b/xen/common/numa.c
> > > @@ -0,0 +1,488 @@
> > > +/*
> > > + * 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 and conditions of the GNU General Public
> > > + * License, version 2, as published by the Free Software Foundation.
> > > + *
> > > + * 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 <asm/acpi.h>
> > > +#include <xen/sched.h>
> > > +#include <xen/softirq.h>
> >
> > Whilst you are moving this in a newfile, please order the includes.
> I understand that you don't like any code changes in code movement
> patch.
Are you saying you blindly copied the headers without even checking they 
are necessary?
Surely, you only added the one necessary which means it would be ok to 
sort them as if one is missing this would be catch by compilation.
Cheers,

--
Julien Grall

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