Added ALIGN, __ALIGN_MASK macroses

PATCH Superseded Stanislav Kholmanskikh 3 participants 18 messages Aug 21, 2013 – Aug 28, 2013 2 messages not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stanislav Kholmanskikh

author

2013-08-21 11:55 UTC #1 lore raw

PATCH v2 1/3 Added ALIGN, __ALIGN_MASK macroses

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 include/test.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

include/test.h +6 -0

index d9eba31..a517ff9 100644
@@ -105,6 +105,12 @@
 #define MAP_PRIVATE_EXCEPT_UCLINUX	MAP_PRIVATE
 #endif
 
+/* Round x to the next multiple of a.
+ * a should be a power of 2.
+ */
+#define ALIGN(x, a)	__ALIGN_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+
 /*
  * lib/forker.c
  */

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-21 11:55 UTC #2 lore raw

PATCH v2 2/3 syscalls/migrate_pages: fix nodemask memory allocation

In reply to a message not in the archive

In accordance to man migrate_pages(), mbind() the bit mask size should be rounded to next multiple of sizeof(unsigned long).

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 .../syscalls/migrate_pages/migrate_pages01.c       |    2 +-
 .../syscalls/migrate_pages/migrate_pages02.c       |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

testcases/kernel/syscalls/migrate_pages/migrate_pages01.c +1 -1

index c23e8b0..6361e9f 100644
@@ -248,7 +248,7 @@ static void setup(void)
 			 ret);
 
 	sane_max_node = get_max_node();
-	sane_nodemask_size = sane_max_node / 8 + 1;
+	sane_nodemask_size = ALIGN(sane_max_node, sizeof(unsigned long)*8) / 8;
 	sane_old_nodes = SAFE_MALLOC(NULL, sane_nodemask_size);
 	sane_new_nodes = SAFE_MALLOC(NULL, sane_nodemask_size);
 	memset(sane_old_nodes, 0, sane_nodemask_size);

testcases/kernel/syscalls/migrate_pages/migrate_pages02.c +1 -1

index 49129e0..5790abd 100644
@@ -110,7 +110,7 @@ static int migrate_to_node(pid_t pid, int node)
 	tst_resm(TINFO, "pid(%d) migrate pid %d to node -> %d",
 		 getpid(), pid, node);
 	max_node = get_max_node();
-	nodemask_size = max_node / 8 + 1;
+	nodemask_size = ALIGN(max_node, sizeof(unsigned long)*8) / 8;
 	old_nodes = SAFE_MALLOC(NULL, nodemask_size);
 	new_nodes = SAFE_MALLOC(NULL, nodemask_size);

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and AppDynamics. Performance Central is your source for news, insights, analysis and resources for efficient Application Performance Management. Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-21 11:55 UTC #4 lore raw

PATCH v2 3/3 lib/numa_helper.c: fix nodemask_size

In reply to a message not in the archive

Now nodemask_size is rounded up to the next multiple of sizeof(nodemask_t).

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 testcases/kernel/lib/numa_helper.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

testcases/kernel/lib/numa_helper.c +3 -3

index 4157816..9151583 100644
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
 #if HAVE_NUMA_H
 static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
 {
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
 	int i;
 	char fn[64];
 	struct stat st;

@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
 static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long max_node)
 {
 #if MPOL_F_MEMS_ALLOWED
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
 	memset(nodemask, 0, nodemask_size);
 	/*
 	 * avoid numa_get_mems_allowed(), because of bug in getpol()

@@ -165,7 +165,7 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int **nodes)
 
 #if HAVE_NUMA_H
 	unsigned long max_node = get_max_node();
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
 
 	nodemask = malloc(nodemask_size);
 	if (nodes)

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Jan Stancek

2013-08-21 12:29 UTC #5 lore raw

Re: [LTP] [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size

----- Original Message -----

Quoted 8 lines

From: "Stanislav Kholmanskikh" <[email protected]>
To: [email protected]
Cc: "vasily isaenko" <[email protected]>, [email protected]
Sent: Wednesday, 21 August, 2013 1:54:58 PM
Subject: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size
Now nodemask_size is rounded up to the next multiple of sizeof(nodemask_t).

Hi,

Why multiple of nodemask_t? It can be quite large.

Quoted 12 lines

Signed-off-by Stanislav Kholmanskikh <[email protected]>
---
 testcases/kernel/lib/numa_helper.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
testcases/kernel/lib/numa_helper.c +0 -0
index 4157816..9151583 100644

testcases/kernel/lib/numa_helper.c unsigned long get_max_node(void) · line 64

Quoted 6 lines

testcases/kernel/lib/numa_helper.c +1 -1

@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
 #if HAVE_NUMA_H
 static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long
 max_node)
 {
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;

Because mask is passed as parameter, we should respect max_node and clear only up to byte which holds max_node. So I think we should align to next byte only:

        unsigned long nodemask_size = ALIGN(max_node, 8) / 8;

testcases/kernel/lib/numa_helper.c static void get_nodemask_allnodes(nodemask_t * nodemask, · line 81

Quoted 11 lines

testcases/kernel/lib/numa_helper.c +0 -0

 	int i;
 	char fn[64];

testcases/kernel/lib/numa_helper.c +1 -1

 	struct stat st;

@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask,
 unsigned long max_node)
 static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long
 max_node)
 {
 #if MPOL_F_MEMS_ALLOWED
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;

Same as above:

        unsigned long nodemask_size = ALIGN(max_node, 8) / 8;

testcases/kernel/lib/numa_helper.c int get_allowed_nodes_arr(int flag, int *num_nodes, int · line 169

Quoted 10 lines

testcases/kernel/lib/numa_helper.c +0 -0

 	memset(nodemask, 0, nodemask_size);

testcases/kernel/lib/numa_helper.c +1 -1

 	/*
 	 * avoid numa_get_mems_allowed(), because of bug in getpol()

@@ -165,7 +165,7 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int
 **nodes)
 
 #if HAVE_NUMA_H
 	unsigned long max_node = get_max_node();
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;

This function allocates the nodemask, so we can align to as much as we need. I'd expect this to be same as in migrate_pages, align to next long:

        unsigned long nodemask_size = ALIGN(max_node / 8, sizeof(long));

Regards,
Jan

Quoted 7 lines

testcases/kernel/lib/numa_helper.c +0 -1
 	nodemask = malloc(nodemask_size);
 	if (nodes)
--
1.7.1

Stanislav Kholmanskikh

author

2013-08-21 13:22 UTC #6 lore raw

Re: [LTP] [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size

Quoted 10 lines · Jan Stancek

----- Original Message -----
Quoted 6 lines
From: "Stanislav Kholmanskikh" <[email protected]>
To: [email protected]
Cc: "vasily isaenko" <[email protected]>, [email protected]
Sent: Wednesday, 21 August, 2013 1:54:58 PM
Subject: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size

Quoted 5 lines

Quoted 2 lines
Now nodemask_size is rounded up to the next multiple of sizeof(nodemask_t).
Hi,
Why multiple of nodemask_t? It can be quite large.

Hi.

As nodemask is a pointer to nodemask_t type, so it should point to memory areas
multiple of sizeof(nodemask_t).

Isn't it?

Quoted 22 lines

Quoted 12 lines
Signed-off-by Stanislav Kholmanskikh <[email protected]>
---
  testcases/kernel/lib/numa_helper.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)
testcases/kernel/lib/numa_helper.c +0 -0
index 4157816..9151583 100644
testcases/kernel/lib/numa_helper.c unsigned long get_max_node(void) · line 64
Quoted 6 lines
testcases/kernel/lib/numa_helper.c +1 -1
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
  #if HAVE_NUMA_H
  static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long
  max_node)
  {
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
Because mask is passed as parameter, we should respect max_node and clear only up to byte which holds max_node. So I think we should align to next byte only:

Quoted 1 line

         unsigned long nodemask_size = ALIGN(max_node, 8) / 8;

I agree but I'm not sure how bytes comprising nodemask_t are handled. If they are handled in an endianness-dependant way then your approach will work only on little-endian systems.

So I decided to clear entire region. The same for filter_nodemask_mem.

Quoted 21 lines

testcases/kernel/lib/numa_helper.c static void get_nodemask_allnodes(nodemask_t * nodemask, · line 81
Quoted 12 lines
testcases/kernel/lib/numa_helper.c +0 -0
  	int i;
  	char fn[64];
struct stat st;
testcases/kernel/lib/numa_helper.c +1 -1
@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask,
 unsigned long max_node)
  static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long
  max_node)
  {
  #if MPOL_F_MEMS_ALLOWED
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
Same as above:
         unsigned long nodemask_size = ALIGN(max_node, 8) / 8;
Quoted 6 lines
testcases/kernel/lib/numa_helper.c +0 -0
  	memset(nodemask, 0, nodemask_size);
/*
* avoid numa_get_mems_allowed(), because of bug in getpol()
testcases/kernel/lib/numa_helper.c +0 -0
@@ -165,7 +165,7 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int
 **nodes)
 

Quoted 8 lines

testcases/kernel/lib/numa_helper.c int get_allowed_nodes_arr(int flag, int *num_nodes, int **nodes) · line 168
Quoted 4 lines
testcases/kernel/lib/numa_helper.c +1 -1
  #if HAVE_NUMA_H
  	unsigned long max_node = get_max_node();
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
This function allocates the nodemask, so we can align to as much as we need. I'd expect this to be same as in migrate_pages, align to next long:
         unsigned long nodemask_size = ALIGN(max_node / 8, sizeof(long));

This formula may give incorrect results. For example, if max_mode = 66 and sizeof(long) = 8, then
ALIGN(max_node / 8, sizeof(long)) will output 8 and we will lost 2 bits. The correct output should be 16.

I think as max_node contains number of bits so we should align it on sizeof(long)*8 boundary and after that divide the final result by 8.

Quoted 10 lines

Regards,
Jan
Quoted 7 lines
testcases/kernel/lib/numa_helper.c +0 -1
  	nodemask = malloc(nodemask_size);
  	if (nodes)
--
1.7.1

Jan Stancek

2013-08-21 14:28 UTC #7 lore raw

Re: [LTP] [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size

----- Original Message -----

Quoted 22 lines

From: "Stanislav Kholmanskikh" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], "vasily isaenko" <[email protected]>
Sent: Wednesday, 21 August, 2013 3:22:04 PM
Subject: Re: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size
Quoted 14 lines · Jan Stancek
----- Original Message -----
Quoted 8 lines
From: "Stanislav Kholmanskikh" <[email protected]>
To: [email protected]
Cc: "vasily isaenko" <[email protected]>, [email protected]
Sent: Wednesday, 21 August, 2013 1:54:58 PM
Subject: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size
Now nodemask_size is rounded up to the next multiple of sizeof(nodemask_t).
Hi,

Quoted 8 lines

Quoted 1 line
Why multiple of nodemask_t? It can be quite large.
Hi.
As nodemask is a pointer to nodemask_t type, so it should point to memory areas
multiple of sizeof(nodemask_t).
Isn't it?

typedef struct {
        unsigned long n[NUMA_NUM_NODES/(sizeof(unsigned long)*8)];
} nodemask_t;

It's used more like trailing array in this case, because NUMA_NUM_NODES is not always correct (I think it was version < 2.0 that had this issue).

I kept the type so I can reuse some trivial functions from numa.h and kernel gets 'n' field directly so it doesn't care about nodemask_t.

Quoted 59 lines

Quoted 22 lines
Quoted 12 lines
Signed-off-by Stanislav Kholmanskikh <[email protected]>
---
  testcases/kernel/lib/numa_helper.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)
testcases/kernel/lib/numa_helper.c +0 -0
index 4157816..9151583 100644
testcases/kernel/lib/numa_helper.c unsigned long get_max_node(void) · line 64
Quoted 6 lines
testcases/kernel/lib/numa_helper.c +1 -1
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
  #if HAVE_NUMA_H
  static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long
  max_node)
  {
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
Because mask is passed as parameter, we should respect max_node and clear only up to byte which holds max_node. So I think we should align to next byte only:
Quoted 1 line
         unsigned long nodemask_size = ALIGN(max_node, 8) / 8;
I agree but I'm not sure how bytes comprising nodemask_t are handled. If they are handled in an endianness-dependant way then your approach will work only on little-endian systems.
So I decided to clear entire region. The same for filter_nodemask_mem.
Quoted 30 lines
testcases/kernel/lib/numa_helper.c static void get_nodemask_allnodes(nodemask_t * nodemask, · line 81
Quoted 12 lines
testcases/kernel/lib/numa_helper.c +0 -0
  	int i;
  	char fn[64];
struct stat st;
testcases/kernel/lib/numa_helper.c +1 -1
@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask,
 unsigned long max_node)
  static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long
  max_node)
  {
  #if MPOL_F_MEMS_ALLOWED
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
Same as above:
         unsigned long nodemask_size = ALIGN(max_node, 8) / 8;
testcases/kernel/lib/numa_helper.c
Quoted 11 lines
testcases/kernel/lib/numa_helper.c +0 -0
  	memset(nodemask, 0, nodemask_size);
/*
* avoid numa_get_mems_allowed(), because of bug in getpol()
testcases/kernel/lib/numa_helper.c +0 -0
@@ -165,7 +165,7 @@ int get_allowed_nodes_arr(int flag, int *num_nodes,
int
**nodes)
  #if HAVE_NUMA_H
  	unsigned long max_node = get_max_node();
testcases/kernel/lib/numa_helper.c +1 -1
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
This function allocates the nodemask, so we can align to as much as we need.
I'd expect this to be same as in migrate_pages, align to next long:

Quoted 8 lines

Quoted 1 line
         unsigned long nodemask_size = ALIGN(max_node / 8, sizeof(long));
This formula may give incorrect results. For example, if max_mode = 66 and sizeof(long) = 8, then
ALIGN(max_node / 8, sizeof(long)) will output 8 and we will lost 2 bits. The correct output should be 16.
I think as max_node contains number of bits so we should align it on sizeof(long)*8 boundary and after that divide the final result by 8.

Agreed, we should align on bits then divide.

What if we align max_node? Then we can be sure that nodemask_size in all functions is also aligned:

testcases/kernel/lib/numa_helper.c +4 -4

index 4157816..a2b6b4a 100644
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
 #if HAVE_NUMA_H
 static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
 {
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long nodemask_size = max_node / 8;
        int i;
        char fn[64];
        struct stat st;

@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
 static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long max_node)
 {
 #if MPOL_F_MEMS_ALLOWED
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long nodemask_size = max_node / 8;
        memset(nodemask, 0, nodemask_size);
        /*
         * avoid numa_get_mems_allowed(), because of bug in getpol()

@@ -164,8 +164,8 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int **nodes)
                *nodes = NULL;
 
 #if HAVE_NUMA_H
-       unsigned long max_node = get_max_node();
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long max_node = ALIGN(get_max_node(), sizeof(long)*8);
+       unsigned long nodemask_size = max_node / 8;
 
        nodemask = malloc(nodemask_size);
        if (nodes)

Regards,
Jan

Stanislav Kholmanskikh

author

2013-08-22 06:13 UTC #8 lore raw

Re: [LTP] [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size

Quoted 35 lines · Jan Stancek

----- Original Message -----
Quoted 21 lines
From: "Stanislav Kholmanskikh" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], "vasily isaenko" <[email protected]>
Sent: Wednesday, 21 August, 2013 3:22:04 PM
Subject: Re: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size
Quoted 13 lines · Jan Stancek
----- Original Message -----
Quoted 8 lines
From: "Stanislav Kholmanskikh" <[email protected]>
To: [email protected]
Cc: "vasily isaenko" <[email protected]>, [email protected]
Sent: Wednesday, 21 August, 2013 1:54:58 PM
Subject: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size
Now nodemask_size is rounded up to the next multiple of sizeof(nodemask_t).
Hi,
Quoted 8 lines
Quoted 1 line
Why multiple of nodemask_t? It can be quite large.
Hi.
As nodemask is a pointer to nodemask_t type, so it should point to memory areas
multiple of sizeof(nodemask_t).
Isn't it?
typedef struct {
         unsigned long n[NUMA_NUM_NODES/(sizeof(unsigned long)*8)];
} nodemask_t;

Quoted 5 lines

It's used more like trailing array in this case, because NUMA_NUM_NODES is not always correct (I think it was version < 2.0 that had this issue).
I kept the type so I can reuse some trivial functions from numa.h and kernel gets 'n' field directly so it doesn't care about nodemask_t.

Thank you. Now it's clear.

Quoted 21 lines

Quoted 21 lines
Quoted 21 lines
Quoted 11 lines
Signed-off-by Stanislav Kholmanskikh <[email protected]>
---
   testcases/kernel/lib/numa_helper.c |    6 +++---
   1 files changed, 3 insertions(+), 3 deletions(-)
testcases/kernel/lib/numa_helper.c +0 -0
index 4157816..9151583 100644
testcases/kernel/lib/numa_helper.c unsigned long get_max_node(void) · line 64
Quoted 6 lines
testcases/kernel/lib/numa_helper.c +1 -1
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
   #if HAVE_NUMA_H
   static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long
   max_node)
   {
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
Because mask is passed as parameter, we should respect max_node and clear only up to byte which holds max_node. So I think we should align to next byte only:

Quoted 6 lines

Quoted 6 lines
Quoted 1 line
          unsigned long nodemask_size = ALIGN(max_node, 8) / 8;
I agree but I'm not sure how bytes comprising nodemask_t are handled. If they are handled in an endianness-dependant way then your approach will work only on little-endian systems.
So I decided to clear entire region. The same for filter_nodemask_mem.

Given a definition of _setbit, _getbit functions (from numactl-2.0.8/libnuma.c) used in nodemask_t (bitmask) handling functions:

static void
_setbit(struct bitmask *bmp, unsigned int n, unsigned int v)

{
         if (n < bmp->size) {
                 if (v)
                         bmp->maskp[n/bitsperlong] |= 1UL << (n % 
bitsperlong);
                 else
                         bmp->maskp[n/bitsperlong] &= ~(1UL << (n % 
bitsperlong));
         }
}

I think we cannot clear area by zeroing only few bytes of a long. Each byte of a long should
be zeroed.

Quoted 36 lines

Quoted 36 lines
Quoted 30 lines
testcases/kernel/lib/numa_helper.c static void get_nodemask_allnodes(nodemask_t * nodemask, · line 81
Quoted 12 lines
testcases/kernel/lib/numa_helper.c +0 -0
   	int i;
   	char fn[64];
   	struct stat st;
testcases/kernel/lib/numa_helper.c +1 -1
@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask,
 unsigned long max_node)
   static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long
   max_node)
   {
   #if MPOL_F_MEMS_ALLOWED
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
Same as above:
          unsigned long nodemask_size = ALIGN(max_node, 8) / 8;
testcases/kernel/lib/numa_helper.c
Quoted 11 lines
testcases/kernel/lib/numa_helper.c +0 -0
   	memset(nodemask, 0, nodemask_size);
/*
* avoid numa_get_mems_allowed(), because of bug in getpol()
testcases/kernel/lib/numa_helper.c +0 -0
@@ -165,7 +165,7 @@ int get_allowed_nodes_arr(int flag, int *num_nodes,
int
**nodes)
   #if HAVE_NUMA_H
   	unsigned long max_node = get_max_node();
testcases/kernel/lib/numa_helper.c +1 -1
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = ALIGN(max_node, sizeof(nodemask_t)*8) / 8;
This function allocates the nodemask, so we can align to as much as we need.
I'd expect this to be same as in migrate_pages, align to next long:
Quoted 1 line
          unsigned long nodemask_size = ALIGN(max_node / 8, sizeof(long));
This formula may give incorrect results. For example, if max_mode = 66 and sizeof(long) = 8, then
ALIGN(max_node / 8, sizeof(long)) will output 8 and we will lost 2 bits. The correct output should be 16.

Quoted 6 lines

Quoted 2 lines
I think as max_node contains number of bits so we should align it on sizeof(long)*8 boundary and after that divide the final result by 8.
Agreed, we should align on bits then divide.
What if we align max_node? Then we can be sure that nodemask_size in all functions is also aligned:

Ok. And for convenience the same for migrate_pages fix ?

Quoted 35 lines

testcases/kernel/lib/numa_helper.c +4 -4

index 4157816..a2b6b4a 100644
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
  #if HAVE_NUMA_H
  static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
  {
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long nodemask_size = max_node / 8;
         int i;
         char fn[64];
         struct stat st;

@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
  static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long max_node)
  {
  #if MPOL_F_MEMS_ALLOWED
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long nodemask_size = max_node / 8;
         memset(nodemask, 0, nodemask_size);
         /*
          * avoid numa_get_mems_allowed(), because of bug in getpol()

@@ -164,8 +164,8 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int **nodes)
                 *nodes = NULL;
 
  #if HAVE_NUMA_H
-       unsigned long max_node = get_max_node();
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long max_node = ALIGN(get_max_node(), sizeof(long)*8);
+       unsigned long nodemask_size = max_node / 8;
 
         nodemask = malloc(nodemask_size);
         if (nodes)

Quoted 2 lines

Regards,
Jan

Jan Stancek

2013-08-22 07:01 UTC #9 lore raw

Re: [LTP] [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size

----- Original Message -----

Quoted 5 lines

From: "Stanislav Kholmanskikh" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], "vasily isaenko" <[email protected]>
Sent: Thursday, 22 August, 2013 8:13:34 AM
Subject: Re: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size

<snip>

Quoted 3 lines

I think we cannot clear area by zeroing only few bytes of a long. Each byte of a long should
be zeroed.

Agreed, that was good point.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
This formula may give incorrect results. For example, if max_mode = 66 and sizeof(long) = 8, then
ALIGN(max_node / 8, sizeof(long)) will output 8 and we will lost 2 bits. The correct output should be 16.

Quoted 7 lines

Quoted 6 lines
Quoted 2 lines
I think as max_node contains number of bits so we should align it on sizeof(long)*8 boundary and after that divide the final result by 8.
Agreed, we should align on bits then divide.
What if we align max_node? Then we can be sure that nodemask_size in all functions is also aligned:
Ok. And for convenience the same for migrate_pages fix ?

I'll leave it up to you.
Man page says, that "bits beyond those specified by maxnode are ignored" so both ways should work equally well in those testcases.

Regards,
Jan

Quoted 39 lines

testcases/kernel/lib/numa_helper.c +1 -1

index 4157816..a2b6b4a 100644
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
  #if HAVE_NUMA_H
  static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long
  max_node)
  {
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long nodemask_size = max_node / 8;
         int i;
         char fn[64];

         struct stat st;

testcases/kernel/lib/numa_helper.c +1 -1

@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask,
 unsigned long max_node)
  static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long
  max_node)
  {
  #if MPOL_F_MEMS_ALLOWED
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long nodemask_size = max_node / 8;
         memset(nodemask, 0, nodemask_size);

/*
* avoid numa_get_mems_allowed(), because of bug in getpol()

testcases/kernel/lib/numa_helper.c +2 -2

@@ -164,8 +164,8 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int
 **nodes)
                 *nodes = NULL;
 
  #if HAVE_NUMA_H
-       unsigned long max_node = get_max_node();
-       unsigned long nodemask_size = max_node / 8 + 1;
+       unsigned long max_node = ALIGN(get_max_node(), sizeof(long)*8);
+       unsigned long nodemask_size = max_node / 8;

Quoted 7 lines

Quoted 5 lines
testcases/kernel/lib/numa_helper.c +0 -0
         nodemask = malloc(nodemask_size);
if (nodes)
Regards,
Jan

Stanislav Kholmanskikh

author

2013-08-22 07:42 UTC #10 lore raw

PATCH v3 1/3 Added ALIGN, __ALIGN_MASK macroses

In reply to a message not in the archive

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 include/test.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

include/test.h +6 -0

index d9eba31..a517ff9 100644
@@ -105,6 +105,12 @@
 #define MAP_PRIVATE_EXCEPT_UCLINUX	MAP_PRIVATE
 #endif
 
+/* Round x to the next multiple of a.
+ * a should be a power of 2.
+ */
+#define ALIGN(x, a)	__ALIGN_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+
 /*
  * lib/forker.c
  */

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Jan Stancek

Reviewed-by on v3 1/3

2013-08-23 07:57 UTC #11 lore raw

----- Original Message -----

Quoted 7 lines

From: "Stanislav Kholmanskikh" <[email protected]>
To: [email protected]
Cc: "vasily isaenko" <[email protected]>, [email protected]
Sent: Thursday, 22 August, 2013 9:41:47 AM
Subject: [PATCH V3 1/3] Added ALIGN, __ALIGN_MASK macroses
Signed-off-by Stanislav Kholmanskikh <[email protected]>

The series looks good to me. I also ran it on small NUMA system, it worked fine. My only nit is this patch's subject, I believe plural of macro is macros :-).

Reviewed-by Jan Stancek <[email protected]>

Quoted 22 lines

---
 include/test.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

include/test.h +6 -1

index d9eba31..a517ff9 100644
@@ -105,6 +105,12 @@
 #define MAP_PRIVATE_EXCEPT_UCLINUX	MAP_PRIVATE
 #endif
 
+/* Round x to the next multiple of a.
+ * a should be a power of 2.
+ */
+#define ALIGN(x, a)	__ALIGN_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+
 /*
  * lib/forker.c
  */
--

Quoted 3 lines

1.7.1

Stanislav Kholmanskikh

author

2013-08-23 08:26 UTC #12 lore raw

PATCH 1/3 Added ALIGN, __ALIGN_MASK macros

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 include/test.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

include/test.h +6 -0

index d9eba31..a517ff9 100644
@@ -105,6 +105,12 @@
 #define MAP_PRIVATE_EXCEPT_UCLINUX	MAP_PRIVATE
 #endif
 
+/* Round x to the next multiple of a.
+ * a should be a power of 2.
+ */
+#define ALIGN(x, a)	__ALIGN_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+
 /*
  * lib/forker.c
  */

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

[email protected]

2013-08-27 12:01 UTC #13 lore raw

Re: [LTP] [PATCH V3.1 1/3] Added ALIGN, __ALIGN_MASK macros

Hi!

Quoted 10 lines

Signed-off-by Stanislav Kholmanskikh <[email protected]>
---
 include/test.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
include/test.h +0 -0
index d9eba31..a517ff9 100644

include/test.h line 113

Quoted 9 lines

include/test.h +6 -0

@@ -105,6 +105,12 @@
 #define MAP_PRIVATE_EXCEPT_UCLINUX	MAP_PRIVATE
 #endif
 
+/* Round x to the next multiple of a.
+ * a should be a power of 2.
+ */
+#define ALIGN(x, a)	__ALIGN_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+

Generally looks good to me. I'm a bit concerned that macros named as ALIGN() and __ALING_MASK() may eventually collide with some system definiton. What about prefixing them with LTP_?

Also test.h is a bit crowded, what about adding them into compiler.h instead?

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-23 08:28 UTC #14 lore raw

In reply to Jan Stancek

Quoted 11 lines · Jan Stancek

----- Original Message -----
Quoted 7 lines
From: "Stanislav Kholmanskikh" <[email protected]>
To: [email protected]
Cc: "vasily isaenko" <[email protected]>, [email protected]
Sent: Thursday, 22 August, 2013 9:41:47 AM
Subject: [PATCH V3 1/3] Added ALIGN, __ALIGN_MASK macroses
Signed-off-by Stanislav Kholmanskikh <[email protected]>
The series looks good to me. I also ran it on small NUMA system, it worked fine. My only nit is this patch's subject, I believe plural of macro is macros :-).

Shame on me :) Sent V3.1 of this patch.

Quoted 15 lines

Reviewed-by Jan Stancek <[email protected]>

Quoted 12 lines

---
  include/test.h |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

include/test.h +0 -0

index d9eba31..a517ff9 100644
@@ -105,6 +105,12 @@
  #define MAP_PRIVATE_EXCEPT_UCLINUX	MAP_PRIVATE
  #endif

Quoted 13 lines

Quoted 10 lines

include/test.h +6 -1

+/* Round x to the next multiple of a.
+ * a should be a power of 2.
+ */
+#define ALIGN(x, a)	__ALIGN_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+
  /*
   * lib/forker.c
   */
--

Quoted 3 lines

1.7.1

Stanislav Kholmanskikh

author

2013-08-22 07:42 UTC #15 lore raw

PATCH v3 2/3 syscalls/migrate_pages: fix nodemask memory allocation

In reply to a message not in the archive

In accordance to man migrate_pages(), mbind() the bit mask size should be rounded to the next multiple of sizeof(unsigned long).

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 .../syscalls/migrate_pages/migrate_pages01.c       |    4 ++--
 .../syscalls/migrate_pages/migrate_pages02.c       |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

testcases/kernel/syscalls/migrate_pages/migrate_pages01.c +2 -2

index c23e8b0..9af4f14 100644
@@ -247,8 +247,8 @@ static void setup(void)
 		tst_brkm(TBROK | TERRNO, NULL, "get_allowed_nodes_arr: %d",
 			 ret);
 
-	sane_max_node = get_max_node();
-	sane_nodemask_size = sane_max_node / 8 + 1;
+	sane_max_node = ALIGN(get_max_node(), sizeof(unsigned long)*8);
+	sane_nodemask_size = sane_max_node / 8;
 	sane_old_nodes = SAFE_MALLOC(NULL, sane_nodemask_size);
 	sane_new_nodes = SAFE_MALLOC(NULL, sane_nodemask_size);
 	memset(sane_old_nodes, 0, sane_nodemask_size);

testcases/kernel/syscalls/migrate_pages/migrate_pages02.c +2 -2

index 49129e0..ba0e625 100644
@@ -109,8 +109,8 @@ static int migrate_to_node(pid_t pid, int node)
 
 	tst_resm(TINFO, "pid(%d) migrate pid %d to node -> %d",
 		 getpid(), pid, node);
-	max_node = get_max_node();
-	nodemask_size = max_node / 8 + 1;
+	max_node = ALIGN(get_max_node(), sizeof(unsigned long)*8);
+	nodemask_size = max_node / 8;
 	old_nodes = SAFE_MALLOC(NULL, nodemask_size);
 	new_nodes = SAFE_MALLOC(NULL, nodemask_size);

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

[email protected]

2013-08-27 12:04 UTC #16 lore raw

Re: [LTP] [PATCH V3 2/3] syscalls/migrate_pages: fix nodemask memory allocation

Hi!

Quoted 4 lines

In accordance to man migrate_pages(), mbind() the bit mask size should be rounded to the next multiple of sizeof(unsigned long).
Signed-off-by Stanislav Kholmanskikh <[email protected]>

The 2/3 and 3/3 looks good. Acked.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

A message that is not in the archive (sent off-list or not yet received): 1377607792-5731-1-git-send-email-stanislav.kholmanskikh@oracle.com

[email protected]

2013-08-28 13:42 UTC #18 lore raw

Re: [LTP] [PATCH V4] syscalls/migrate_pages: fix nodemask_memory_allocation

Hi!

Quoted 3 lines

Changes since V3:
* Macros ALIGN, __ALIGN_MASK renamed to LTP_ALIGN, __LTP_ALIGN_MASK
* Both macros are placed in compiler.h (instead of test.h)

Pushed, thanks.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-22 07:42 UTC #19 lore raw

PATCH v3 3/3 lib/numa_helper.c: fix nodemask_size

In reply to a message not in the archive

Now nodemask_size is rounded up to the next
multiple of sizeof(unsigned long).

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 testcases/kernel/lib/numa_helper.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

testcases/kernel/lib/numa_helper.c +4 -4

index 4157816..ab294f0 100644
@@ -60,7 +60,7 @@ unsigned long get_max_node(void)
 #if HAVE_NUMA_H
 static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
 {
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = max_node / 8;
 	int i;
 	char fn[64];
 	struct stat st;

@@ -76,7 +76,7 @@ static void get_nodemask_allnodes(nodemask_t * nodemask, unsigned long max_node)
 static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long max_node)
 {
 #if MPOL_F_MEMS_ALLOWED
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long nodemask_size = max_node / 8;
 	memset(nodemask, 0, nodemask_size);
 	/*
 	 * avoid numa_get_mems_allowed(), because of bug in getpol()

@@ -164,8 +164,8 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int **nodes)
 		*nodes = NULL;
 
 #if HAVE_NUMA_H
-	unsigned long max_node = get_max_node();
-	unsigned long nodemask_size = max_node / 8 + 1;
+	unsigned long max_node = ALIGN(get_max_node(), sizeof(unsigned long)*8);
+	unsigned long nodemask_size = max_node / 8;
 
 	nodemask = malloc(nodemask_size);
 	if (nodes)

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and 
AppDynamics. Performance Central is your source for news, insights, 
analysis and resources for efficient Application Performance Management. 
Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list