Re: [Xen-devel] [RFC PATCH v3 05/24] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-07-20 Thread Julien Grall 



On 20/07/17 08:41, Vijay Kilari wrote:

On Wed, Jul 19, 2017 at 10:48 PM, Julien Grall  wrote:



On 19/07/17 07:40, Vijay Kilari wrote:


On Tue, Jul 18, 2017 at 8:59 PM, Wei Liu  wrote:


On Tue, Jul 18, 2017 at 05:11:27PM +0530, vijay.kil...@gmail.com wrote:


From: Vijaya Kumar K 

Add accessors for nodes[] and other static variables and
use those accessors. These variables are later accessed
outside the file when the code made generic in later
patches. However the coding style is not changed.

Signed-off-by: Vijaya Kumar K 
---
v3: - Changed accessors parameter from int to unsigned int
- Updated commit message
- Fixed wrong indentation
---
 xen/arch/x86/srat.c | 106
+++-
 1 file changed, 81 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 535c9d7..42cca5a 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -41,6 +41,44 @@ static struct node
node_memblk_range[NR_NODE_MEMBLKS];
 static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
 static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);

+static struct node *get_numa_node(unsigned int id)
+{
+ return [id];
+}
+
+static nodeid_t get_memblk_nodeid(unsigned int id)
+{
+ return memblk_nodeid[id];
+}
+
+static nodeid_t *get_memblk_nodeid_map(void)
+{
+ return _nodeid[0];
+}
+
+static struct node *get_node_memblk_range(unsigned int memblk)
+{
+ return _memblk_range[memblk];
+}
+
+static int get_num_node_memblks(void)
+{
+ return num_node_memblks;
+}



They should all be inline functions. And maybe at once lift to a header
and add proper prefix since you mention they are going to be used later.



Currently these are static variables in x86/srat.c file.
In patch #9 I move them to common/numa.c file and make these functions
non-static.

If I lift them to header file and make inline, then I have to make these
as
global variables.



As I said on v2, I am not sure to understand the usefulness of those
accessors over global variables...


These are static variables. To access across other files (arch specific)
these accessors are added. I have to make them global variables to use
outside of this file.

I am happy to make them global and make these accessors static inline
as suggested by Wei.


I don't think the helpers are useful... They just return a value without 
any sanity check. You can directly use the global variable in the code.


I would much prefer if you use plain global.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH v3 05/24] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-07-20 Thread Vijay Kilari 
On Wed, Jul 19, 2017 at 10:48 PM, Julien Grall  wrote:
>
>
> On 19/07/17 07:40, Vijay Kilari wrote:
>>
>> On Tue, Jul 18, 2017 at 8:59 PM, Wei Liu  wrote:
>>>
>>> On Tue, Jul 18, 2017 at 05:11:27PM +0530, vijay.kil...@gmail.com wrote:

 From: Vijaya Kumar K 

 Add accessors for nodes[] and other static variables and
 use those accessors. These variables are later accessed
 outside the file when the code made generic in later
 patches. However the coding style is not changed.

 Signed-off-by: Vijaya Kumar K 
 ---
 v3: - Changed accessors parameter from int to unsigned int
 - Updated commit message
 - Fixed wrong indentation
 ---
  xen/arch/x86/srat.c | 106
 +++-
  1 file changed, 81 insertions(+), 25 deletions(-)

 diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
 index 535c9d7..42cca5a 100644
 --- a/xen/arch/x86/srat.c
 +++ b/xen/arch/x86/srat.c
 @@ -41,6 +41,44 @@ static struct node
 node_memblk_range[NR_NODE_MEMBLKS];
  static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
  static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);

 +static struct node *get_numa_node(unsigned int id)
 +{
 + return [id];
 +}
 +
 +static nodeid_t get_memblk_nodeid(unsigned int id)
 +{
 + return memblk_nodeid[id];
 +}
 +
 +static nodeid_t *get_memblk_nodeid_map(void)
 +{
 + return _nodeid[0];
 +}
 +
 +static struct node *get_node_memblk_range(unsigned int memblk)
 +{
 + return _memblk_range[memblk];
 +}
 +
 +static int get_num_node_memblks(void)
 +{
 + return num_node_memblks;
 +}
>>>
>>>
>>> They should all be inline functions. And maybe at once lift to a header
>>> and add proper prefix since you mention they are going to be used later.
>>
>>
>> Currently these are static variables in x86/srat.c file.
>> In patch #9 I move them to common/numa.c file and make these functions
>> non-static.
>>
>> If I lift them to header file and make inline, then I have to make these
>> as
>> global variables.
>
>
> As I said on v2, I am not sure to understand the usefulness of those
> accessors over global variables...

These are static variables. To access across other files (arch specific)
these accessors are added. I have to make them global variables to use
outside of this file.

I am happy to make them global and make these accessors static inline
as suggested by Wei.

>
> You don't have any kind of sanity check, so they would do exactly the same
> job. The global variables would avoid so much churn.
>
> More that you tend to sometimes use global and other time static helpers...
>
> Cheers,
>
> --
> Julien Grall

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

Re: [Xen-devel] [RFC PATCH v3 05/24] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-07-19 Thread Julien Grall 



On 19/07/17 07:40, Vijay Kilari wrote:

On Tue, Jul 18, 2017 at 8:59 PM, Wei Liu  wrote:

On Tue, Jul 18, 2017 at 05:11:27PM +0530, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K 

Add accessors for nodes[] and other static variables and
use those accessors. These variables are later accessed
outside the file when the code made generic in later
patches. However the coding style is not changed.

Signed-off-by: Vijaya Kumar K 
---
v3: - Changed accessors parameter from int to unsigned int
- Updated commit message
- Fixed wrong indentation
---
 xen/arch/x86/srat.c | 106 +++-
 1 file changed, 81 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 535c9d7..42cca5a 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -41,6 +41,44 @@ static struct node node_memblk_range[NR_NODE_MEMBLKS];
 static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
 static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);

+static struct node *get_numa_node(unsigned int id)
+{
+ return [id];
+}
+
+static nodeid_t get_memblk_nodeid(unsigned int id)
+{
+ return memblk_nodeid[id];
+}
+
+static nodeid_t *get_memblk_nodeid_map(void)
+{
+ return _nodeid[0];
+}
+
+static struct node *get_node_memblk_range(unsigned int memblk)
+{
+ return _memblk_range[memblk];
+}
+
+static int get_num_node_memblks(void)
+{
+ return num_node_memblks;
+}


They should all be inline functions. And maybe at once lift to a header
and add proper prefix since you mention they are going to be used later.


Currently these are static variables in x86/srat.c file.
In patch #9 I move them to common/numa.c file and make these functions
non-static.

If I lift them to header file and make inline, then I have to make these as
global variables.


As I said on v2, I am not sure to understand the usefulness of those 
accessors over global variables...


You don't have any kind of sanity check, so they would do exactly the 
same job. The global variables would avoid so much churn.


More that you tend to sometimes use global and other time static helpers...

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH v3 05/24] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-07-19 Thread Vijay Kilari 
On Tue, Jul 18, 2017 at 8:59 PM, Wei Liu  wrote:
> On Tue, Jul 18, 2017 at 05:11:27PM +0530, vijay.kil...@gmail.com wrote:
>> From: Vijaya Kumar K 
>>
>> Add accessors for nodes[] and other static variables and
>> use those accessors. These variables are later accessed
>> outside the file when the code made generic in later
>> patches. However the coding style is not changed.
>>
>> Signed-off-by: Vijaya Kumar K 
>> ---
>> v3: - Changed accessors parameter from int to unsigned int
>> - Updated commit message
>> - Fixed wrong indentation
>> ---
>>  xen/arch/x86/srat.c | 106 
>> +++-
>>  1 file changed, 81 insertions(+), 25 deletions(-)
>>
>> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
>> index 535c9d7..42cca5a 100644
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -41,6 +41,44 @@ static struct node node_memblk_range[NR_NODE_MEMBLKS];
>>  static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>>  static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
>>
>> +static struct node *get_numa_node(unsigned int id)
>> +{
>> + return [id];
>> +}
>> +
>> +static nodeid_t get_memblk_nodeid(unsigned int id)
>> +{
>> + return memblk_nodeid[id];
>> +}
>> +
>> +static nodeid_t *get_memblk_nodeid_map(void)
>> +{
>> + return _nodeid[0];
>> +}
>> +
>> +static struct node *get_node_memblk_range(unsigned int memblk)
>> +{
>> + return _memblk_range[memblk];
>> +}
>> +
>> +static int get_num_node_memblks(void)
>> +{
>> + return num_node_memblks;
>> +}
>
> They should all be inline functions. And maybe at once lift to a header
> and add proper prefix since you mention they are going to be used later.

Currently these are static variables in x86/srat.c file.
In patch #9 I move them to common/numa.c file and make these functions
non-static.

If I lift them to header file and make inline, then I have to make these as
global variables.

Regards
Vijay

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

Re: [Xen-devel] [RFC PATCH v3 05/24] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-07-18 Thread Wei Liu 
On Tue, Jul 18, 2017 at 05:11:27PM +0530, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> Add accessors for nodes[] and other static variables and
> use those accessors. These variables are later accessed
> outside the file when the code made generic in later
> patches. However the coding style is not changed.
> 
> Signed-off-by: Vijaya Kumar K 
> ---
> v3: - Changed accessors parameter from int to unsigned int
> - Updated commit message
> - Fixed wrong indentation
> ---
>  xen/arch/x86/srat.c | 106 
> +++-
>  1 file changed, 81 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index 535c9d7..42cca5a 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -41,6 +41,44 @@ static struct node node_memblk_range[NR_NODE_MEMBLKS];
>  static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>  static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
>  
> +static struct node *get_numa_node(unsigned int id)
> +{
> + return [id];
> +}
> +
> +static nodeid_t get_memblk_nodeid(unsigned int id)
> +{
> + return memblk_nodeid[id];
> +}
> +
> +static nodeid_t *get_memblk_nodeid_map(void)
> +{
> + return _nodeid[0];
> +}
> +
> +static struct node *get_node_memblk_range(unsigned int memblk)
> +{
> + return _memblk_range[memblk];
> +}
> +
> +static int get_num_node_memblks(void)
> +{
> + return num_node_memblks;
> +}

They should all be inline functions. And maybe at once lift to a header
and add proper prefix since you mention they are going to be used later.

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

[Xen-devel] [RFC PATCH v3 05/24] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-07-18 Thread vijay . kilari 
From: Vijaya Kumar K 

Add accessors for nodes[] and other static variables and
use those accessors. These variables are later accessed
outside the file when the code made generic in later
patches. However the coding style is not changed.

Signed-off-by: Vijaya Kumar K 
---
v3: - Changed accessors parameter from int to unsigned int
- Updated commit message
- Fixed wrong indentation
---
 xen/arch/x86/srat.c | 106 +++-
 1 file changed, 81 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 535c9d7..42cca5a 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -41,6 +41,44 @@ static struct node node_memblk_range[NR_NODE_MEMBLKS];
 static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
 static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
 
+static struct node *get_numa_node(unsigned int id)
+{
+   return [id];
+}
+
+static nodeid_t get_memblk_nodeid(unsigned int id)
+{
+   return memblk_nodeid[id];
+}
+
+static nodeid_t *get_memblk_nodeid_map(void)
+{
+   return _nodeid[0];
+}
+
+static struct node *get_node_memblk_range(unsigned int memblk)
+{
+   return _memblk_range[memblk];
+}
+
+static int get_num_node_memblks(void)
+{
+   return num_node_memblks;
+}
+
+static int __init numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t 
size)
+{
+   if (nodeid >= NR_NODE_MEMBLKS)
+   return -EINVAL;
+
+   node_memblk_range[num_node_memblks].start = start;
+   node_memblk_range[num_node_memblks].end = start + size;
+   memblk_nodeid[num_node_memblks] = nodeid;
+   num_node_memblks++;
+
+   return 0;
+}
+
 static inline bool node_found(unsigned int idx, unsigned int pxm)
 {
return ((pxm2node[idx].pxm == pxm) &&
@@ -107,11 +145,11 @@ int valid_numa_range(paddr_t start, paddr_t end, nodeid_t 
node)
 {
int i;
 
-   for (i = 0; i < num_node_memblks; i++) {
-   struct node *nd = _memblk_range[i];
+   for (i = 0; i < get_num_node_memblks(); i++) {
+   struct node *nd = get_node_memblk_range(i);
 
if (nd->start <= start && nd->end > end &&
-   memblk_nodeid[i] == node )
+   get_memblk_nodeid(i) == node)
return 1;
}
 
@@ -122,8 +160,8 @@ static int __init conflicting_memblks(paddr_t start, 
paddr_t end)
 {
int i;
 
-   for (i = 0; i < num_node_memblks; i++) {
-   struct node *nd = _memblk_range[i];
+   for (i = 0; i < get_num_node_memblks(); i++) {
+   struct node *nd = get_node_memblk_range(i);
if (nd->start == nd->end)
continue;
if (nd->end > start && nd->start < end)
@@ -136,7 +174,8 @@ static int __init conflicting_memblks(paddr_t start, 
paddr_t end)
 
 static void __init cutoff_node(nodeid_t i, paddr_t start, paddr_t end)
 {
-   struct node *nd = [i];
+   struct node *nd = get_numa_node(i);
+
if (nd->start < start) {
nd->start = start;
if (nd->end < nd->start)
@@ -278,6 +317,7 @@ acpi_numa_memory_affinity_init(const struct 
acpi_srat_mem_affinity *ma)
unsigned int pxm;
nodeid_t node;
int i;
+   struct node *memblk;
 
if (srat_disabled())
return;
@@ -288,7 +328,7 @@ acpi_numa_memory_affinity_init(const struct 
acpi_srat_mem_affinity *ma)
if (!(ma->flags & ACPI_SRAT_MEM_ENABLED))
return;
 
-   if (num_node_memblks >= NR_NODE_MEMBLKS)
+   if (get_num_node_memblks() >= NR_NODE_MEMBLKS)
{
dprintk(XENLOG_WARNING,
 "Too many numa entry, try bigger NR_NODE_MEMBLKS \n");
@@ -310,27 +350,31 @@ acpi_numa_memory_affinity_init(const struct 
acpi_srat_mem_affinity *ma)
i = conflicting_memblks(start, end);
if (i < 0)
/* everything fine */;
-   else if (memblk_nodeid[i] == node) {
+   else if (get_memblk_nodeid(i) == node) {
bool mismatch = !(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) !=
!test_bit(i, memblk_hotplug);
 
+   memblk = get_node_memblk_range(i);
+
printk("%sSRAT: PXM %u (%"PRIx64"-%"PRIx64") overlaps with 
itself (%"PRIx64"-%"PRIx64")\n",
   mismatch ? KERN_ERR : KERN_WARNING, pxm, start, end,
-  node_memblk_range[i].start, node_memblk_range[i].end);
+  memblk->start, memblk->end);
if (mismatch) {
bad_srat();
return;
}
} else {
+   memblk = get_node_memblk_range(i);
+
printk(KERN_ERR
   "SRAT: PXM %u (%"PRIx64"-%"PRIx64") overlaps with PXM %u 
(%"PRIx64"-%"PRIx64")\n",
-  pxm, start,