From: Jan S. <jst...@re...> - 2013-08-21 14:28:12
|
----- Original Message ----- > From: "Stanislav Kholmanskikh" <sta...@or...> > To: "Jan Stancek" <jst...@re...> > Cc: ltp...@li..., "vasily isaenko" <vas...@or...> > Sent: Wednesday, 21 August, 2013 3:22:04 PM > Subject: Re: [PATCH V2 3/3] lib/numa_helper.c: fix nodemask_size > > > On 08/21/2013 04:29 PM, Jan Stancek wrote: > > > > > > > > ----- Original Message ----- > >> From: "Stanislav Kholmanskikh" <sta...@or...> > >> To: ltp...@li... > >> Cc: "vasily isaenko" <vas...@or...>, jst...@re... > >> 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. > 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. > > > >> Signed-off-by: Stanislav Kholmanskikh <sta...@or...> > >> --- > >> testcases/kernel/lib/numa_helper.c | 6 +++--- > >> 1 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/testcases/kernel/lib/numa_helper.c > >> b/testcases/kernel/lib/numa_helper.c > >> index 4157816..9151583 100644 > >> --- a/testcases/kernel/lib/numa_helper.c > >> +++ b/testcases/kernel/lib/numa_helper.c > >> @@ -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; > 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. > > > > >> 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; > > Same as above: > > unsigned long nodemask_size = ALIGN(max_node, 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; > > 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. 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: diff --git a/testcases/kernel/lib/numa_helper.c b/testcases/kernel/lib/numa_helper.c index 4157816..a2b6b4a 100644 --- a/testcases/kernel/lib/numa_helper.c +++ b/testcases/kernel/lib/numa_helper.c @@ -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 |