<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 8, 2019 at 7:21 PM Jan Stancek <<a href="mailto:jstancek@redhat.com">jstancek@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
----- Original Message -----<br>
> Test issue:<br>
>    mtest01 start many children to alloc chunck of memory and do write<br>
>    page(with -w option), but occasionally some children were killed by<br>
>    oom-killer and exit with SIGCHLD signal sending. After the parent<br>
>    reciving this SIGCHLD signal it will report FAIL as a test result.<br>
> <br>
>    It seems not a real kernel bug if something just like that, it's<br>
>    trying to use 80% of memory and swap. Once it uses most of memory,<br>
>    system starts swapping, but the test is likely consuming memory at<br>
>    greater rate than kswapd can provide, which eventually triggers OOM.<br>
> <br>
>    ---- FAIL LOG ----<br>
>    mtest01     0  TINFO  :  Total memory already used on system = 1027392<br>
>    kbytes<br>
>    mtest01     0  TINFO  :  Total memory used needed to reach maximum =<br>
>    12715520 kbytes<br>
>    mtest01     0  TINFO  :  Filling up 80% of ram which is 11688128 kbytes<br>
>    mtest01     1  TFAIL  :  mtest01.c:314: child process exited unexpectedly<br>
>    -------------------<br>
> <br>
>  Rewrite changes:<br>
>    To make mtest01 more easier to understand, I just rewrite it into<br>
>    LTP new API and make a little changes in children behavior.<br>
> <br>
>    * decrease the pressure to 80% of free memory for testing<br>
>    * drop the signal SIGCHLD action becasue new API help to<br>
>    check_child_status<br>
>    * make child pause itself after finishing their memory allocating/writing<br>
>    * parent sends SIGCONT to make children continue and exit<br>
>    * use TST_PROCESS_STATE_WAIT to wait child changes to 'T' state<br>
>    * involve ALLOC_THRESHOLD to rework same code in defines<br>
>    * to make mtest01 support running with -i N > 1<br>
> <br>
> Signed-off-by: Li Wang <<a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a>><br>
> ---<br>
> <br>
> Notes:<br>
>     v2 --> v3<br>
>        * modify test description for new version<br>
>        * move some global variables to local<br>
>        * remove SIGCONT sending from cleanup<br>
>        * involve ALLOC_THRESHOLD to avoid same code in defines<br>
<br>
Hi,<br>
<br>
looks lot more readable than original version.<br>
<br>
Some nit-picks below / no need to re-post for that typo,<br>
I can fix it before commit.<br><span class="gmail_default" style="font-size:small"></span></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Thanks so much.</div></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">
<snip><br>
<br>
> +#if defined (_s390_)<br>
> +#define ALLOC_THRESHOLD              FIVE_HUNDRED_MB<br>
> +#elif __WORDSIZE == 32<br>
> +#define ALLOC_THRESHOL               (unsigned long long)(2*FIVE_HUNDRED_MB)<br>
<br>
typo here, missing 'D'<br>
<br>
> +#elif __WORDSIZE == 64<br>
> +#define ALLOC_THRESHOLD              (unsigned long long)(6*FIVE_HUNDRED_MB)<br>
> +#endif<br>
>  <br>
> -static void handler(int signo)<br>
> +#define STOP_THRESHOLD 15    /* seconds remaining before reaching timeout */<br>
> +<br>
> +static pid_t *pid_list;<br>
> +static sig_atomic_t pid_count;<br>
> +static int max_pids;<br>
> +static unsigned long long alloc_maxbytes;<br>
> +static unsigned long long original_maxbytes;<br>
<br>
We could use "original_maxbytes" in setup() and options(),<br>
then alloc_maxbytes could be local variable of mem_test().<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Yes, that's right.</div></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">
<br>
<snip> <br>
> +static void do_write_page(char *mem, int chunksize)<br>
> +{<br>
> +     int i;<br>
>  <br>
> -     free(pid_list);<br>
> +     for (i = 0; i < chunksize; i++)<br>
> +             *(mem + i) = 'a';<br>
<br>
We could do i+=pagesize to make it slightly faster.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Agreed. I didn't realized that before.</div></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">
<br>
<snip><br>
<br>
> +static void child_loop_alloc(unsigned long long alloc_bytes)<br>
> +{<br>
> +     unsigned long bytecount = (unsigned long)chunksize;<br>
<br>
Shouldn't this variable start at 0?<br></blockquote><div><br></div><div class="gmail_default" style="font-size:small">Yes.</div><div class="gmail_default" style="font-size:small"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +     char *mem;<br>
> +<br>
> +     while (1) {<br>
> +             mem = SAFE_MALLOC(chunksize);<br>
> +             if (dowrite)<br>
> +                     do_write_page(mem, chunksize);<br>
> +<br>
> +             if (verbose)<br>
> +                     tst_res(TINFO,<br>
> +                             "child %d allocated %lu bytes chunksize is %d",<br>
> +                             getpid(), bytecount, chunksize);<br>
> +             bytecount += chunksize;<br>
> +             if (bytecount >= alloc_bytes)<br>
> +                     break;<br>
>       }<br>
<br>
Overall, the rewrite looks good to me.<br>
<br>
Thanks,<br>
Jan<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>