<div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 31, 2016 at 7:55 PM, Cyril Hrubis <span dir="ltr"><<a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi!<br>
<div><div class="h5">...<br>
> +#include <stdio.h><br>
> +#include <sys/shm.h><br>
<br>
</div></div>We don't need this header now, right?<br></blockquote><div><br></div><div>right! I forgot to remove that.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +#include <errno.h><br>
> +#include <sys/sysinfo.h><br>
> +<br>
> +#include "test.h"<br>
> +#include "safe_macros.h"<br>
> +<br>
> +char *TCID = "madvise06";<br>
> +int TST_TOTAL = 3;<br>
> +<br>
> +#ifdef __x86_64__<br>
> +<br>
> +#define GB_SZ (1024*1024*1024)<br>
> +#define PG_SZ (4*1024)<br>
> +<br>
> +static long dst_num;<br>
> +<br>
> +static void setup(void);<br>
> +static int get_page_fault_num(void);<br>
> +static void test_advice_willneed(void);<br>
> +<br>
> +int main(int argc, char *argv[])<br>
> +{<br>
> + int i, lc;<br>
> +<br>
> + tst_parse_opts(argc, argv, NULL, NULL);<br>
> +<br>
> + setup();<br>
> +<br>
> + for (lc = 0; TEST_LOOPING(lc); lc++) {<br>
> + tst_count = 0;<br>
> +<br>
> + for(i = 0; i < TST_TOTAL; i++)<br>
> + test_advice_willneed();<br>
<br>
</div></div>So we do three iterations of the test to increase the likehood of<br>
hitting the bug, right? In that case we should just add -i 3 to the<br>
runtest file.<br></blockquote><div><br></div><div>ok. Even no need to add -i 3 in runtest file, It is easy to reproduce the issue with un-patched kernel. I will set the TST_TOTAL = 1.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + }<br>
> +<br>
> + tst_exit();<br>
> +}<br>
> +<br>
> +static void setup(void)<br>
> +{<br>
> + struct sysinfo sys_buf;<br>
> +<br>
> + sysinfo(&sys_buf);<br>
> +<br>
> + dst_num = sys_buf.totalram / GB_SZ;<br>
> + tst_resm(TINFO, "dst_num = %ld", dst_num);<br>
<br>
</span>The variable should be named as dst_max instead of dst_num. Which would<br>
make the message more understandable...<br></blockquote><div><br></div><div>agreed.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> + tst_sig(NOFORK, DEF_HANDLER, NULL);<br>
> + if (tst_kvercmp(3, 9, 0) < 0)<br>
> + tst_brkm(TCONF, NULL, "madvise(MADV_WILLNEED) swap file"<br>
> + "prefetch available only since 3.9");<br>
> +<br>
> + if (sys_buf.totalram < 2L * GB_SZ)<br>
> + tst_brkm(TCONF, NULL, "Test requires more than 2GB of RAM.");<br>
> + if (sys_buf.totalram > 100L * GB_SZ)<br>
> + tst_brkm(TCONF, NULL, "System RAM is too large, skip this test.");<br>
> +<br>
> + TEST_PAUSE;<br>
> +}<br>
> +<br>
> +static int get_page_fault_num(void)<br>
> +{<br>
> + int pg;<br>
> +<br>
> + SAFE_FILE_SCANF(NULL, "/proc/self/stat",<br>
> + "%*s %*s %*s %*s %*s %*s %*s %*s %*s %*s %*s %d",<br>
> + &pg);<br>
> +<br>
> + return pg;<br>
> +}<br>
> +<br>
> +static void test_advice_willneed(void)<br>
> +{<br>
> + int i;<br>
> + char *src_1gb;<br>
> + char *dst[100];<br>
> + int page_fault_num1;<br>
> + int page_fault_num2;<br>
> +<br>
> + /* allocate source memory (1GB only) */<br>
> + src_1gb = mmap(NULL, 1 * GB_SZ, PROT_READ | PROT_WRITE,<br>
> + MAP_SHARED | MAP_ANONYMOUS,<br>
> + -1, 0);<br>
> + if (src_1gb == MAP_FAILED)<br>
> + tst_brkm(TFAIL | TERRNO, NULL, "mmap failed");<br>
<br>
</div></div>Use SAFE_MMAP()<br>
<span class=""><br>
> + /* allocate destination memory (array) */<br>
> + for (i = 0; i < dst_num; ++i) {<br>
> + dst[i] = mmap(NULL, 1 * GB_SZ, PROT_READ | PROT_WRITE,<br>
> + MAP_SHARED | MAP_ANONYMOUS,<br>
> + -1, 0);<br>
> + if (dst[i] == MAP_FAILED)<br>
> + tst_brkm(TFAIL | TERRNO, NULL, "mmap failed");<br>
<br>
</span>Here as well.<br>
<span class=""><br>
> + }<br>
> +<br>
> + /* memmove source to each destination memories (for SWAP-OUT) */<br>
> + for (i = 0; i < dst_num; ++i)<br>
> + memmove(dst[i], src_1gb, 1 * GB_SZ);<br>
> +<br>
> + tst_resm(TINFO, "PageFault(before madvice): %d", get_page_fault_num());<br>
> +<br>
> + /* Do madvice() to dst[0] */<br>
> + TEST(madvise(dst[0], PG_SZ, MADV_WILLNEED));<br>
> + if (TEST_RETURN == -1)<br>
> + tst_brkm(TBROK | TERRNO, NULL, "madvise failed");<br>
> + usleep(1000); /* wait for read from SWAP */<br>
<br>
</span>Again, what exactly do we wait here for?<br>
<br>
"wait for read from SWAP" is a bit too vague.<br></blockquote><div><br></div><div>okay, will remove the sleep() function.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + page_fault_num1 = get_page_fault_num();<br>
> + tst_resm(TINFO, "PageFault(after madvice / before Mem Access): %d", page_fault_num1);<br>
<br>
</span>This line is over 80 chars. Try making the message shorter.<br>
<br>
You can use <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> (shipped with Linux kernel) to check patches<br>
before submission.<br>
<span class=""><br>
> + *dst[0] = 'a';<br>
> + page_fault_num2 = get_page_fault_num();<br>
> + tst_resm(TINFO, "PageFault(after madvice / after Mem Access): %d", page_fault_num2);<br>
<br>
</span>Here as well.<br>
<span class=""><br>
> +<br>
> + if (page_fault_num1 != page_fault_num2)<br>
> + tst_resm(TFAIL, "Bug has been reproduced!");<br>
> + else<br>
> + tst_resm(TPASS, "Regression test pass!");<br>
<br>
</span>No need for the ! in the TPASS message.<br>
<span class=""><br>
> + if (munmap(src_1gb, 1 * GB_SZ) == -1)<br>
> + tst_brkm(TFAIL | TERRNO, NULL, "munmap failed");<br>
<br>
</span>Use SAFE_MUNMAP()<br>
<span class=""><br>
> + for (i = 0; i < dst_num; ++i) {<br>
> + if (munmap(dst[i], 1 * GB_SZ) == -1)<br>
> + tst_brkm(TFAIL | TERRNO, NULL, "munmap failed");<br>
<br>
</span>Here as well.<br>
<div class="HOEnZb"><div class="h5"><span class="HOEnZb"><font color="#888888"><br></font></span></div></div></blockquote><div><br></div><div>accepted all the other suggestions, will fix the format isssue in patch V3. <br><br>Thank you.<br></div></div><br>-- <br><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div>Regards,<br></div>Li Wang<br></div><div>Email: <a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a><br></div></div></div></div></div></div>
</div></div>