<div dir="ltr">Hi Cyril,<br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 22, 2016 at 10:15 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br>
<div><div>> +/*<br>
> + * DESCRIPTION<br>
> + *<br>
> + * Page fault occurs in spite that madvise(WILLNEED) system call is called<br>
> + * to prefetch the page. This issue is reproduced by running a program<br>
> + * which sequentially accesses to a shared memory and calls madvise(WILLNEED)<br>
> + * to the next page on a page fault.<br>
> + *<br>
> + * This bug is present in all RHEL7 versions. It looks like this was fixed in<br>
> + * mainline kernel > v3.15 by the following patch:<br>
> + *<br>
> + * commit 55231e5c898c5c03c14194001e349f40f59bd300<br>
> + * Author: Johannes Weiner <<a href="mailto:hannes@cmpxchg.org" target="_blank">hannes@cmpxchg.org</a>><br>
> + * Date: Thu May 22 11:54:17 2014 -0700<br>
> + *<br>
> + * mm: madvise: fix MADV_WILLNEED on shmem swapouts<br>
> + */<br>
> +<br>
> +#include <stdio.h><br>
> +#include <sys/shm.h><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 = 1;<br>
> +<br>
> +#if __x86_64__<br>
</div></div> ^<br>
#ifdef<br>
<div><div><br>
> +#define GB_SZ (1024*1024*1024)<br>
> +#define PG_SZ (4*1024)<br>
> +<br>
> +#define INV_PTR ((char *)-1)<br>
> +<br>
> +static int id_src_1gb;<br>
> +static char *src_1gb;<br>
> +<br>
> +static int *id_dst;<br>
> +static char **dst;<br>
> +static long dst_num;<br>
> +<br>
> +static struct sysinfo *sys_buf;<br>
> +static long mem_total;<br>
> +<br>
> +static int page_fault_num1;<br>
> +static int page_fault_num2;<br>
> +<br>
> +static void setup(void);<br>
> +static void cleanup(void);<br>
> +static void free_shm(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 lc;<br>
> +<br>
> + tst_parse_opts(argc, argv, NULL, NULL);<br>
> +<br>
> + sys_buf = malloc(sizeof(struct sysinfo));<br>
> + sysinfo(sys_buf);<br>
> + mem_total = sys_buf->totalram;<br>
<br>
</div></div>Just why do you use malloc to allocate small structure? Why can't you<br>
just create it on the stack instead?<br>
<br>
Also why do you have global pointer to it when it's not used outside of<br>
the main() function?<br>
<br>
And also getting the amount of system memory is something that should be<br>
done in the setup() function instead.<br></blockquote><div><br></div><div>ok, I will make them be created on the stack in setup().<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><br>
> + setup();<br>
> +<br>
> + for (lc = 0; TEST_LOOPING(lc); lc++) {<br>
> + test_advice_willneed();<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>The TPASS/TFAIL should be printed in the test_advice_willneed()<br>
function and the page_fault* variables shouldn't be global. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><br>
> + free_shm();<br>
> + }<br>
> +<br>
> + free(sys_buf);<br>
> + tst_exit();<br>
> +}<br>
> +<br>
> +static void setup(void)<br>
> +{<br>
> + tst_sig(NOFORK, DEF_HANDLER, cleanup);<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 (mem_total < 2L * GB_SZ) {<br>
> + tst_resm(TCONF, "Test requires more than 2GB of RAM");<br>
> + tst_exit();<br>
<br>
</span>tst_brkm(TCONF, NULL, "...");<br>
<span><br>
> + } else if (mem_total > 100L * GB_SZ) {<br>
<br>
</span>Why is the else branch here if you call tst_exit() in the body of the if<br>
above?<br>
<span><br>
> + tst_resm(TCONF, "System RAM is too large, skip this test.");<br>
> + tst_exit();<br>
<br>
</span>tst_brkm(TCONF, NULL, ...);<br>
<br>
Also why do we skip the test on systems with large RAM?<br>
<br>
Does it take too long?<br></blockquote><div><br></div><div>Yes, the memmove() takes too long on larger RAM, so I limit the test on less than 100G system.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
> + }<br>
> +<br>
> + TEST_PAUSE;<br>
> +}<br>
> +<br>
> +static void cleanup(void)<br>
> +{<br>
> + free_shm();<br>
> +}<br>
> +<br>
> +static void free_shm(void)<br>
> +{<br>
> + int i;<br>
> +<br>
> + if (id_src_1gb != -1) {<br>
> + if (shmctl(id_src_1gb, IPC_RMID, NULL) == -1)<br>
> + tst_brkm(TBROK | TERRNO, NULL, "shmctl(id_src_1gb)");<br>
> + }<br>
> +<br>
> + for (i = 0; i < dst_num; ++i) {<br>
> + if (id_dst[i] != -1) {<br>
> + if (shmctl(id_dst[i], IPC_RMID, NULL) == -1)<br>
> + tst_brkm(TBROK | TERRNO, NULL, "shmctl(id_dst[i])");<br>
> + }<br>
> + }<br>
<br>
</div></div>You can shmctl(..., IPC_RMID) the memory right after you did shmat() and<br>
you don't have to remember the ids that way, the memory will be<br>
accesible till you detach it with shmdt(). In other words this will not<br>
free the memory at all. Does the test runs with -i 100?<br></blockquote><div><br></div><div>run with -i 100, it triggers the oom-killer.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><br>
> +}<br>
> +<br>
> +static int get_page_fault_num(void)<br>
> +{<br>
> + FILE *fp;<br>
> + char s[11][256];<br>
> + int pg, ret;<br>
> +<br>
> + fp = fopen("/proc/self/stat", "r");<br>
> + if (fp == NULL)<br>
> + tst_brkm(TBROK | TERRNO, cleanup, "Failed to open /proc/self/stat.");<br>
> +<br>
> + ret = fscanf(fp, "%s %s %s %s %s %s %s %s %s %s %s %d",<br>
> + s[0], s[1], s[2], s[3], s[4], s[5], s[6], s[7], s[8], s[9], s[10], &pg);<br>
<br>
</span>fscanf has assigment-supression operator '*' so you can just do:<br>
<br>
SAFE_FILE_SCANF("/proc/self/stat",<br>
"%*s %*s %*s %*s %*s %*s %*s %*s %*s %*s %d",<br>
<span> &pg);<br></span></blockquote><div><br></div><div>pretty good! it makes the code much less but clearer.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
<br>
> + if (ret != 12) {<br>
> + tst_resm(TFAIL, "Failed to get page fault num.");<br>
> + pg = -1;<br>
> + }<br>
> +<br>
> + fclose(fp);<br>
> + return pg;<br>
> +}<br>
> +<br>
> +static void test_advice_willneed(void)<br>
> +{<br>
> + int i;<br>
> +<br>
> + dst_num = mem_total / GB_SZ;<br>
> + tst_resm(TINFO, "dst_num = %ld", dst_num);<br>
> +<br>
> + if ((id_dst = malloc(dst_num * sizeof(int))) == NULL)<br>
> + tst_brkm(TBROK | TERRNO, NULL, "malloc failed.");<br>
> +<br>
> + if ((dst = malloc(dst_num * sizeof(char *))) == NULL)<br>
> + tst_brkm(TBROK | TERRNO, NULL, "malloc failed.");<br>
<br>
</span>Make use of SAFE_MALLOC().<br>
<span><br>
> + id_src_1gb = -1;<br>
> + src_1gb = INV_PTR;<br>
> + for (i = 0; i < dst_num; ++i) {<br>
> + id_dst[i] = -1;<br>
> + dst[i] = INV_PTR;<br>
> + }<br>
> +<br>
> + /* allocate source memory (1GB only) */<br>
> + id_src_1gb = shmget(IPC_PRIVATE, 1 * GB_SZ, IPC_CREAT);<br>
> + if (id_src_1gb == -1)<br>
> + tst_brkm(TBROK | TERRNO, cleanup, "shmget failed");<br>
> + src_1gb = shmat(id_src_1gb, 0, 0);<br>
> + if (src_1gb == INV_PTR)<br>
> + tst_brkm(TBROK | TERRNO, cleanup, "shmat failed");<br>
> +<br>
> + /* allocate destination memory (array) */<br>
> + for (i = 0; i < dst_num; ++i) {<br>
> + id_dst[i] = shmget(IPC_PRIVATE, 1 * GB_SZ, IPC_CREAT);<br>
> + if (id_dst[i] == -1)<br>
> + tst_brkm(TBROK | TERRNO, cleanup, "shmget failed");<br>
> +<br>
> + dst[i] = shmat(id_dst[i], 0, 0);<br>
> + if (dst[i] == INV_PTR)<br>
> + tst_brkm(TBROK | TERRNO, cleanup, "shmat failed");<br>
<br>
</span>Why can't we just use mmap() with MAP_ANONYMOUS instead? Do we really<br>
need to use the System V shm?<br></blockquote><div><br></div><div>Just tried mmap() with flag 'MAP_SHARED | MAP_ANONYMOUS', it reproduced the bug as we expected. Now, I decide to use mmap(). thanks! <br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><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>
> + fflush(stdout);<br>
<br>
</span>Why do we fflush(stdout) here? We do not write to it at all.<br>
<span><br>
> + }<br>
> +<br>
> + /* Do madvice() to dst[0] */<br>
> + tst_resm(TINFO, "PageFault(before madvice): %d", get_page_fault_num());<br>
> +<br>
> + TEST(madvise(dst[0], PG_SZ, MADV_WILLNEED));<br>
> + if (TEST_RETURN == -1)<br>
> + tst_brkm(TBROK | TERRNO, cleanup, "madvise failed");<br>
> + sleep(3); /* wait for read from SWAP */<br>
<br>
</span>Can't we just loop here with a usleep(1000) waiting for the page_fault<br>
count to increase?<br>
<br>
Doing blind sleep(3) does not guarantee that the event we are wating<br>
for really happened and will slown down the testing.<br>
<span><br>
> + /* Read dst[0] data */<br>
<br>
</span>Eh? You are writing to dst[0] here.<br>
<span><br>
> + page_fault_num1 = get_page_fault_num();<br>
> + tst_resm(TINFO, "PageFault(after madvice / before Mem Access): %d", page_fault_num1);<br>
> +<br>
> + *dst[0] = 10;<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>You are allocating the dst arrays on each test iteration but they are<br>
not freed. You should rather allocate them once in the setup and free<br>
then in the cleanup.<br>
<br>
Also the test would not run if you have more than 100GB so we can easily<br>
setup static global array for 100 pointers and be done with it.<br></blockquote><div><br></div><div>sure, I tend to declare the global arrays as id_des[100], dst[100].<br><br></div><div>And, I accepted all the other comments in above. Thanks for reviewing, PATCH V2 will be sent out soon.<br></div></div><br clear="all"><br>-- <br><div><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>