Re: [RFC PATCH v3 08/24] NUMA: x86: Move numa code and make it generic

From: Vijay Kilari <vijay.kilari@gmail.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Tim Deegan <tim@xen.org>,
	kevin.tian@intel.com, Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Dario Faggioli <dario.faggioli@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Jan Beulich <jbeulich@suse.com>,
	Vijaya Kumar K <Vijaya.Kumar@cavium.com>
Subject: Re: [RFC PATCH v3 08/24] NUMA: x86: Move numa code and make it generic
Date: Wed, 19 Jul 2017 12:17:39 +0530	[thread overview]
Message-ID: <CALicx6vFsL0rbSU8ZUAJNPweC9ZM=F0=YWLoGN6dSR386XfSOw@mail.gmail.com> (raw)
In-Reply-To: <9ed2c785-6473-3404-2f0b-17dd4d50f1e1@arm.com>

On Tue, Jul 18, 2017 at 11:46 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
>
> On 18/07/17 16:29, Wei Liu wrote:
>>
>> On Tue, Jul 18, 2017 at 05:11:30PM +0530, vijay.kilari@gmail.com wrote:
>> [...]
>>>
>>> diff --git a/xen/common/numa.c b/xen/common/numa.c
>>> new file mode 100644
>>> index 0000000..0381f1b
>>> --- /dev/null
>>> +++ b/xen/common/numa.c
>>> @@ -0,0 +1,487 @@
>>> +/*
>>> + * 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/init.h>
>>> +#include <xen/ctype.h>
>>> +#include <xen/sched.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/mm.h>
>>> +#include <xen/softirq.h>
>>> +#include <xen/string.h>
>>> +#include <asm/acpi.h>
>>> +
>>
>>
>> Since you're moving code anyway, please sort the headers alphabetically.
>>
>>> +static int numa_setup(char *s);
>>> +custom_param("numa", numa_setup);
>>> +
>>> +struct node_data node_data[MAX_NUMNODES];
>>> +
>>> +/* Mapping from pdx to node id */
>>
>>
>> Is this comment applicable to ARM? Does arm has PDX?
>
>
> Yes ARM has PDX. For new architecture we expect the code to provide dummy
> helpers if they want to support NUMA.
>
>>
>>> +unsigned int memnode_shift;
>>> +
>>> +/*
>>> + * In case of numa init failure or numa off,
>>> + * memnode_shift is initialized to BITS_PER_LONG - 1. Hence allocate
>>> + * memnodemap[] of BITS_PER_LONG.
>>> + */
>>> +static typeof(*memnodemap) _memnodemap[BITS_PER_LONG];
>>> +unsigned long memnodemapsize;
>>> +uint8_t *memnodemap;
>>> +
>>> +nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
>>> +    [0 ... NR_CPUS-1] = NUMA_NO_NODE
>>> +};
>>> +
>>> +cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
>>> +
>>> +bool numa_off;
>>> +s8 acpi_numa = 0;
>>> +
>>> +int srat_disabled(void)
>>
>>
>> bool here.
>>
>> Should probably be done in a previous patch.
>
>
> Actually, the previous version had srat_disabled return bool. I am aware
> that Jan and I requested to keep acpi_numa as int, I didn't find any request
> of keep moving srat_disabled to int. So can you explain why??

My bad. I dropped patch #4 from v2. But this change was part of patch
#4 and missed it out.

>
>>
>>> +
>>> +void __init numa_init_array(void)
>>> +{
>>> +    int rr, i;
>>> +
>>> +    /* There are unfortunately some poorly designed mainboards around
>>> +       that only connect memory to a single CPU. This breaks the 1:1
>>> cpu->node
>>> +       mapping. To avoid this fill in the mapping for all possible
>>> +       CPUs, as the number of CPUs is not known yet.
>>> +       We round robin the existing nodes. */
>>
>>
>> Please fix the coding style issue here.
>>
>
> Cheers,
>
> --
> Julien Grall

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