Re: [LTP] [PATCH 1/3] mm/ksm: set the merge_across_nodes knob before testing

[Wanlong Gao <gaowanlong@cn.fujitsu.com>]

On 05/10/2013 06:18 PM, Zhouping Liu wrote:
> Hi Wanlong,
> 
> ----- Original Message -----
>> From: "Wanlong Gao" <gaowanlong@cn.fujitsu.com>
>> To: "Zhouping Liu" <zliu@redhat.com>
>> Cc: "LTP List" <ltp-list@lists.sourceforge.net>
>> Sent: Friday, May 10, 2013 5:43:52 PM
>> Subject: Re: [LTP] [PATCH 1/3] mm/ksm: set the merge_across_nodes knob before testing
>>
>> On 05/10/2013 05:13 PM, Zhouping Liu wrote:
>>> This kernel commit 90bd6fd31c809(ksm: allow trees per NUMA node)
>>> introduced a new KSM sysfs knob /sys/kernel/mm/ksm/merge_across_nodes,
>>> when it is set to zero, only pages from the same node are merged,
>>> which is different with the previous behavior, and ksm test cases
>>> sometimes will fail in NUMA system.
>>>
>>> Signed-off-by: Zhouping Liu <zliu@redhat.com>
>>
>> While how about wrap the set and reset "merge_across_nodes" value to
>> functions,
>> so that we can reduce the dup code and dup comments?
> 
> yeah, it sounds good, but... look at the codes again:
> 
> ...
>  void cleanup(void)
>  {
> +       if (access(PATH_KSM "merge_across_nodes", F_OK) == 0)
> +               /* recover the old value */
> +               SAFE_FILE_PRINTF(NULL, PATH_KSM "merge_across_nodes",
> +                                "%d", merge_across_nodes);
> 
> resetting "merge_across_nodes" only needs one statement.
> 
> +
>         umount_mem(CPATH, CPATH_NEW);
>         TEST_CLEANUP;
>  }
> @@ -136,6 +143,16 @@ void setup(void)
>         if (access(PATH_KSM, F_OK) == -1)
>                 tst_brkm(TCONF, NULL, "KSM configuration is not enabled");
> 
> +       if (access(PATH_KSM "merge_across_nodes", F_OK) == 0) {
> +               /*
> +                * Save the current value of merge_across_nodes knob,
> +                * and make it perform as the default behavior.
> +                */
> +               SAFE_FILE_SCANF(NULL, PATH_KSM "merge_across_nodes",
> +                               "%d", &merge_across_nodes);
> +               SAFE_FILE_PRINTF(NULL, PATH_KSM "merge_across_nodes", "1");
> 
> and setting the value only needs three statements, and it looks easy to understand,
> I can remove the comments from ksm02.c ksm03.c ksm04.c to reduce the duplicated comments.
> 
> also "merge_across_nodes" is a long string, I can't think of a better function name to
> implement set/unset merge_across_nodes, so... IMO, the current codes is good, do you think so?

OK to me.

Thanks,
Wanlong Gao

> 
> +       }
> +
> ...
>  
> 


------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and 
their applications. This 200-page book is written by three acclaimed 
leaders in the field. The early access version is available now. 
Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list