Re: [LTP] [PATCH] Fix short of nodemask array.

Lans Zhang
2013-04-17T03:09:01Z

On 04/16/2013 02:58 PM, Jan Stancek wrote:
>
>
>
>
> ----- Original Message -----
>> From: "Zhouping Liu"<zliu@redhat.com>
>> To: "gmail"<lans.zhang2008@gmail.com>
>> Cc: ltp-list@lists.sourceforge.net, "Wanlong Gao"<gaowanlong@cn.fujitsu.com>, "Jan Stancek"<jstancek@redhat.com>
>> Sent: Tuesday, 16 April, 2013 6:34:53 AM
>> Subject: Re: [LTP] [PATCH] Fix short of nodemask array.
>>
>> Hi Lans,
>>
>> ----- Original Message -----
>>> From: "gmail"<lans.zhang2008@gmail.com>
>>> To: ltp-list@lists.sourceforge.net
>>> Sent: Monday, March 4, 2013 12:49:45 PM
>>> Subject: [LTP] [PATCH] Fix short of nodemask array.
>>>
>>> In kernel, when the user specified more nodes, e.g, 512 nodes, than
>>> supported, e.g, 4 nodes, just check if the non supported part is all
>>> zero. If user space just specified the nodemask array with only one
>>> element, which makes the check in kernel actually become invalid.
>>
>> I think the comments here is not clear, you should explain more details
>> about the 'one element' or the reason why the checking in kernel will
>> be invalid.
>
> I think what it says is that we are overrunning "nmask", whose length is
> shorter than MAXNODES.

Correct.

>
>>
>>> In addition, some test cases in current implement doesn't consider
>>> if a node number is greater than sizeof(unsigned long) * 8 - 1.
>>
>> yes, you are right, we don't care the system which has more than 512 numa
>> nodes.
>> and the current code only support 64 numa nodes, only one unsigned long node
>> mask.
>>
>> I checked kernel code in RHEL6, CONFIG_NODES_SHIFT=9, so 512 is the biggest
>> value to support in RHEL6 for x86_64, and I didn't meet any large NUMA system
>> which have 512+ numa nodes so far.
>>
>>>
>>> Signed-off-by: Lans Zhang<lans.zhang2008@gmail.com>
>>> ---
>>> testcases/kernel/mem/cpuset/cpuset01.c | 6 +++---
>>> testcases/kernel/mem/ksm/ksm02.c | 6 +++---
>>> testcases/kernel/mem/ksm/ksm04.c | 6 +++---
>>> testcases/kernel/mem/lib/mem.c | 6 +++---
>>> 4 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/testcases/kernel/mem/cpuset/cpuset01.c
>>> b/testcases/kernel/mem/cpuset/cpuset01.c
>>> index b473e45..e459306 100644
>>> --- a/testcases/kernel/mem/cpuset/cpuset01.c
>>> +++ b/testcases/kernel/mem/cpuset/cpuset01.c
>>> @@ -93,7 +93,7 @@ static void testcpuset(void)
>>> {
>>> int lc;
>>> int child, i, status;
>>> - unsigned long nmask = 0;
>>> + unsigned long nmask[MAXNODES / (8*sizeof(unsigned long))] = { 0 };
>>> char mems[BUFSIZ], buf[BUFSIZ];
>>>
>>> read_cpuset_files(CPATH, "cpus", buf);
>>> @@ -108,8 +108,8 @@ static void testcpuset(void)
>>> tst_brkm(TBROK | TERRNO, cleanup, "fork");
>>> case 0:
>>> for (i = 0; i< nnodes; i++)
>>> - nmask += 1<< nodes[i];
>>> - if (set_mempolicy(MPOL_BIND,&nmask, MAXNODES) == -1)
>>> + nmask[i / (8*sizeof(unsigned long))] |= 1<< (i % (8*sizeof(unsigned
>>> long)));
>>> + if (set_mempolicy(MPOL_BIND, nmask, MAXNODES) == -1)
>>> tst_brkm(TBROK | TERRNO, cleanup, "set_mempolicy");
>>> exit(mem_hog_cpuset(ncpus> 1 ? ncpus : 1));
>>> }
>>> diff --git a/testcases/kernel/mem/ksm/ksm02.c
>>> b/testcases/kernel/mem/ksm/ksm02.c
>>> index be9ff96..9d2d142 100644
>>> --- a/testcases/kernel/mem/ksm/ksm02.c
>>> +++ b/testcases/kernel/mem/ksm/ksm02.c
>>> @@ -87,7 +87,7 @@ int main(int argc, char *argv[])
>>> int lc;
>>> char *msg;
>>> int size = 128, num = 3, unit = 1;
>>> - unsigned long nmask = 0;
>>> + unsigned long nmask[MAXNODES / (8*sizeof(unsigned long))] = { 0 };
>>> unsigned int node;
>>>
>>> msg = parse_opts(argc, argv, ksm_options, ksm_usage);
>>> @@ -95,7 +95,7 @@ int main(int argc, char *argv[])
>>> tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
>>>
>>> node = get_a_numa_node(tst_exit);
>>> - nmask = 1<< node;
>>> + nmask[node / (8*sizeof(unsigned long))] = 1<< (node % (8*sizeof(unsigned
>>> long)));
>>
>> I found '1<< (node % (8*sizeof(unsigned long)))' is used frequently, I
>> suggest we
>> can create a macro, such as:
>> # define GET_NODE_MASK(nid) 1<< (nid % (8*sizeof(unsigned long)))
>
> I agree, also 'nmask[i / (8*sizeof(unsigned long))]' is there often.
> How about something like:
>
> #define bitsperlong (8 * sizeof(unsigned long))
> void set_bit(unsigned long *b, unsigned int n, unsigned int v)
> {
> if (v)
> b[n / bitsperlong] |= 1UL<< (n % bitsperlong);
> else
> b[n / bitsperlong]&= ~(1UL<< (n % bitsperlong));
> }
> set_bit(nmask, node, 1);

Looks better. I will use this style in V2.

Thanks,
lz

>
> Regards,
> Jan
>
>>
>> also please update the patch, as there's some updates before your patch in
>> lib/mem.c,
>> which makes some conflict with the latest tree.
>>
>> Thanks,
>> Zhouping
>>
>>>
>>> setup();
>>>
>>> @@ -103,7 +103,7 @@ int main(int argc, char *argv[])
>>> Tst_count = 0;
>>> check_ksm_options(&size,&num,&unit);
>>>
>>> - if (set_mempolicy(MPOL_BIND,&nmask, MAXNODES) == -1) {
>>> + if (set_mempolicy(MPOL_BIND, nmask, MAXNODES) == -1) {
>>> if (errno != ENOSYS)
>>> tst_brkm(TBROK | TERRNO, cleanup,
>>> "set_mempolicy");
>>> diff --git a/testcases/kernel/mem/ksm/ksm04.c
>>> b/testcases/kernel/mem/ksm/ksm04.c
>>> index e4aa417..7829afd 100644
>>> --- a/testcases/kernel/mem/ksm/ksm04.c
>>> +++ b/testcases/kernel/mem/ksm/ksm04.c
>>> @@ -87,7 +87,7 @@ int main(int argc, char *argv[])
>>> int lc;
>>> char *msg;
>>> int size = 128, num = 3, unit = 1;
>>> - unsigned long nmask = 0;
>>> + unsigned long nmask[MAXNODES / (8*sizeof(unsigned long))] = { 0 };
>>> unsigned int node;
>>>
>>> msg = parse_opts(argc, argv, ksm_options, ksm_usage);
>>> @@ -95,7 +95,7 @@ int main(int argc, char *argv[])
>>> tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
>>>
>>> node = get_a_numa_node(tst_exit);
>>> - nmask = 1<< node;
>>> + nmask[node / (8*sizeof(unsigned long))] = 1<< (node % (8*sizeof(unsigned
>>> long)));
>>>
>>> setup();
>>>
>>> @@ -105,7 +105,7 @@ int main(int argc, char *argv[])
>>>
>>> write_memcg();
>>>
>>> - if (set_mempolicy(MPOL_BIND,&nmask, MAXNODES) == -1) {
>>> + if (set_mempolicy(MPOL_BIND, nmask, MAXNODES) == -1) {
>>> if (errno != ENOSYS)
>>> tst_brkm(TBROK | TERRNO, cleanup,
>>> "set_mempolicy");
>>> diff --git a/testcases/kernel/mem/lib/mem.c
>>> b/testcases/kernel/mem/lib/mem.c
>>> index f095fe1..abe29fd 100644
>>> --- a/testcases/kernel/mem/lib/mem.c
>>> +++ b/testcases/kernel/mem/lib/mem.c
>>> @@ -66,12 +66,12 @@ void oom(int testcase, int mempolicy, int lite)
>>> int status;
>>> #if HAVE_NUMA_H&& HAVE_LINUX_MEMPOLICY_H&& HAVE_NUMAIF_H \
>>> && HAVE_MPOL_CONSTANTS
>>> - unsigned long nmask = 0;
>>> + unsigned long nmask[MAXNODES / (8*sizeof(unsigned long))] = { 0 };
>>> unsigned int node;
>>>
>>> if (mempolicy)
>>> node = get_a_numa_node(cleanup);
>>> - nmask += 1<< node;
>>> + nmask[node / (8*sizeof(unsigned long))] = 1<< (node % (8*sizeof(unsigned
>>> long)));
>>> #endif
>>>
>>> switch (pid = fork()) {
>>> @@ -81,7 +81,7 @@ void oom(int testcase, int mempolicy, int lite)
>>> #if HAVE_NUMA_H&& HAVE_LINUX_MEMPOLICY_H&& HAVE_NUMAIF_H \
>>> && HAVE_MPOL_CONSTANTS
>>> if (mempolicy)
>>> - if (set_mempolicy(MPOL_BIND,&nmask, MAXNODES) == -1)
>>> + if (set_mempolicy(MPOL_BIND, nmask, MAXNODES) == -1)
>>> tst_brkm(TBROK | TERRNO, cleanup,
>>> "set_mempolicy");
>>> #endif
>>> --
>>> 1.7.8.110.g4cb5d
>>>
>>> ------------------------------------------------------------------------------
>>> Minimize network downtime and maximize team effectiveness.
>>> Reduce network management and security costs.Learn how to hire
>>> the most talented Cisco Certified professionals. Visit the
>>> Employer Resources Portal
>>> http://www.cisco.com/web/learning/employer_resources/index.html
>>> _______________________________________________
>>> Ltp-list mailing list
>>> Ltp-list@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/ltp-list
>>>
>>
>> --
>> Thanks,
>> Zhouping
>>
>

------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr