Re: [Xen-devel] [RFC PATCH v2 06/25] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-05-09 Thread Julien Grall 



On 05/09/2017 08:02 AM, Vijay Kilari wrote:

On Mon, May 8, 2017 at 8:09 PM, Julien Grall  wrote:

Hi Vijay,

On 28/03/17 16:53, vijay.kil...@gmail.com wrote:


From: Vijaya Kumar K 

Add accessor for nodes[] and other static variables and



s/accessor/accessors/


used those accessors.



Also, I am not sure to understand the usefulness of those accessors over a
global variable.


These are static variables which needs to accessed from other files and
later moved to generic file.


101 of a contributor, always explaining in the commit message why you do 
something. Also, I am quite confused why sometimes you decide to use 
static and helper, other time you will use global variables.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH v2 06/25] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-05-09 Thread Vijay Kilari 
On Mon, May 8, 2017 at 8:09 PM, Julien Grall  wrote:
> Hi Vijay,
>
> On 28/03/17 16:53, vijay.kil...@gmail.com wrote:
>>
>> From: Vijaya Kumar K 
>>
>> Add accessor for nodes[] and other static variables and
>
>
> s/accessor/accessors/
>
>> used those accessors.
>
>
> Also, I am not sure to understand the usefulness of those accessors over a
> global variable.

These are static variables which needs to accessed from other files and
later moved to generic file.

>
>> Signed-off-by: Vijaya Kumar K 
>> ---
>>  xen/arch/x86/srat.c | 108
>> +++-
>>  1 file changed, 82 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
>> index ccacbcd..983e1d8 100644
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -41,7 +41,45 @@ 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 inline bool node_found(unsigned idx, unsigned pxm)
>> +static struct node *get_numa_node(int id)
>
>
> unsigned int.
OK
>
>> +{
>> +   return [id];
>> +}
>> +
>> +static nodeid_t get_memblk_nodeid(int id)
>
>
> unsigned int.
>
>> +{
>> +   return memblk_nodeid[id];
>> +}
>> +
>> +static nodeid_t *get_memblk_nodeid_map(void)
>> +{
>> +   return _nodeid[0];
>> +}
>> +
>> +static struct node *get_node_memblk_range(int memblk)
>
>
> unsigned int.
>
>> +{
>> +   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)
>
>
> Please don't make unrelated change in the same patch. In this case I don't
> see why you switch from "unsigned" to "unsigned int".
>
>>  {
>> return ((pxm2node[idx].pxm == pxm) &&
>> (pxm2node[idx].node != NUMA_NO_NODE));
>> @@ -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)
>
>
> Why the indentation changed here?

OK. will wrap these changes in other patches.

>
>
>> 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(int 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 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

Re: [Xen-devel] [RFC PATCH v2 06/25] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-05-08 Thread Julien Grall 

Hi Vijay,

On 28/03/17 16:53, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K 

Add accessor for nodes[] and other static variables and


s/accessor/accessors/


used those accessors.


Also, I am not sure to understand the usefulness of those accessors over 
a global variable.



Signed-off-by: Vijaya Kumar K 
---
 xen/arch/x86/srat.c | 108 +++-
 1 file changed, 82 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index ccacbcd..983e1d8 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -41,7 +41,45 @@ 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 inline bool node_found(unsigned idx, unsigned pxm)
+static struct node *get_numa_node(int id)


unsigned int.


+{
+   return [id];
+}
+
+static nodeid_t get_memblk_nodeid(int id)


unsigned int.


+{
+   return memblk_nodeid[id];
+}
+
+static nodeid_t *get_memblk_nodeid_map(void)
+{
+   return _nodeid[0];
+}
+
+static struct node *get_node_memblk_range(int memblk)


unsigned int.


+{
+   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)


Please don't make unrelated change in the same patch. In this case I 
don't see why you switch from "unsigned" to "unsigned int".



 {
return ((pxm2node[idx].pxm == pxm) &&
(pxm2node[idx].node != NUMA_NO_NODE));
@@ -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)


Why the indentation changed here?


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(int 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 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 {
+

[Xen-devel] [RFC PATCH v2 06/25] x86: NUMA: Add accessors for nodes[] and node_memblk_range[] structs

2017-03-28 Thread vijay . kilari 
From: Vijaya Kumar K 

Add accessor for nodes[] and other static variables and
used those accessors.

Signed-off-by: Vijaya Kumar K 
---
 xen/arch/x86/srat.c | 108 +++-
 1 file changed, 82 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index ccacbcd..983e1d8 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -41,7 +41,45 @@ 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 inline bool node_found(unsigned idx, unsigned pxm)
+static struct node *get_numa_node(int id)
+{
+   return [id];
+}
+
+static nodeid_t get_memblk_nodeid(int id)
+{
+   return memblk_nodeid[id];
+}
+
+static nodeid_t *get_memblk_nodeid_map(void)
+{
+   return _nodeid[0];
+}
+
+static struct node *get_node_memblk_range(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) &&
(pxm2node[idx].node != NUMA_NO_NODE));
@@ -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(int 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 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, end, node_to_pxm(memblk_nodeid[i]),
-  node_memblk_range[i].start, node_memblk_range[i].end);
+  pxm, start, end, node_to_pxm(get_memblk_nodeid(i)),