* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-05 3:15 [LTP] [PATCH] perf_event_open: improve the memory leak detection Li Wang
@ 2024-07-05 3:30 ` Li Wang
2024-07-17 6:50 ` Li Wang
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Li Wang @ 2024-07-05 3:30 UTC (permalink / raw)
To: ltp, Cyril Hrubis
On Fri, Jul 5, 2024 at 11:15 AM Li Wang <liwang@redhat.com> wrote:
> The goal is to add more robust memory leak detection by periodically
> sampling memory availability during the test run and checking for
> significant decreases in available memory.
>
> To avoid false postive:
> perf_event_open03.c:95: TFAIL: Likely kernel memory leak detected
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
> .../perf_event_open/perf_event_open03.c | 32 +++++++++++++++----
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> b/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> index 7dd31d3d2..1aab43e82 100644
> --- a/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> +++ b/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> @@ -26,6 +26,7 @@
> const int iterations = 12000000;
> static int fd = -1;
> static int runtime;
> +static int sample;
>
> static void setup(void)
> {
> @@ -77,22 +78,41 @@ static void check_progress(int i)
>
> static void run(void)
> {
> - long diff;
> + long diff, diff_total, mem_avail, mem_avail_prev;
> int i;
>
> - diff = SAFE_READ_MEMINFO("MemAvailable:");
> + sample = 0;
> + diff_total = 0;
> +
> + mem_avail_prev = SAFE_READ_MEMINFO("MemAvailable:");
> tst_timer_start(CLOCK_MONOTONIC);
>
> /* leak about 100MB of RAM */
> for (i = 0; i < iterations; i++) {
> ioctl(fd, PERF_EVENT_IOC_SET_FILTER, "filter,0/0@abcd");
> check_progress(i);
> - }
>
> - diff -= SAFE_READ_MEMINFO("MemAvailable:");
> + /*
> + * Every 1200000 iterations, calculate the difference in
> memory
> + * availability. If the difference is greater than 10 *
> 1024 (10MB),
> + * increment the sample counter and log the event.
> + */
> + if ((i % 1200000) == 0) {
> + mem_avail = SAFE_READ_MEMINFO("MemAvailable:");
> + diff = mem_avail_prev - mem_avail;
> + diff_total += diff;
> +
> + if (diff > 20 * 1024) {
> + sample++;
> + tst_res(TINFO, "MemAvailable decreased by
> %ld kB at iteration %d", diff, i);
> + }
> +
> + mem_avail_prev = mem_avail;
> + }
> + }
>
> - if (diff > 50 * 1024)
> - tst_res(TFAIL, "Likely kernel memory leak detected");
> + if ((sample > 5) || (diff_total > 100 * 1024))
>
Here maybe diff_toal is still the interference factor in reporting false
positives,
I'm hesitating to remove or keep it.
Or we could use it first to see how the test performs in the large-scale CI
testing.
> + tst_res(TFAIL, "Likely kernel memory leak detected, total
> decrease: %ld kB", diff_total);
> else
> tst_res(TPASS, "No memory leak found");
> }
> --
> 2.45.2
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-05 3:15 [LTP] [PATCH] perf_event_open: improve the memory leak detection Li Wang
2024-07-05 3:30 ` Li Wang
@ 2024-07-17 6:50 ` Li Wang
2024-07-17 8:31 ` Cyril Hrubis
2024-07-18 15:07 ` Martin Doucha
3 siblings, 0 replies; 9+ messages in thread
From: Li Wang @ 2024-07-17 6:50 UTC (permalink / raw)
To: ltp, Cyril Hrubis
Hi Cyril, All,
Any comments on this method?
On Fri, Jul 5, 2024 at 11:15 AM Li Wang <liwang@redhat.com> wrote:
> The goal is to add more robust memory leak detection by periodically
> sampling memory availability during the test run and checking for
> significant decreases in available memory.
>
> To avoid false postive:
> perf_event_open03.c:95: TFAIL: Likely kernel memory leak detected
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
> .../perf_event_open/perf_event_open03.c | 32 +++++++++++++++----
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> b/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> index 7dd31d3d2..1aab43e82 100644
> --- a/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> +++ b/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> @@ -26,6 +26,7 @@
> const int iterations = 12000000;
> static int fd = -1;
> static int runtime;
> +static int sample;
>
> static void setup(void)
> {
> @@ -77,22 +78,41 @@ static void check_progress(int i)
>
> static void run(void)
> {
> - long diff;
> + long diff, diff_total, mem_avail, mem_avail_prev;
> int i;
>
> - diff = SAFE_READ_MEMINFO("MemAvailable:");
> + sample = 0;
> + diff_total = 0;
> +
> + mem_avail_prev = SAFE_READ_MEMINFO("MemAvailable:");
> tst_timer_start(CLOCK_MONOTONIC);
>
> /* leak about 100MB of RAM */
> for (i = 0; i < iterations; i++) {
> ioctl(fd, PERF_EVENT_IOC_SET_FILTER, "filter,0/0@abcd");
> check_progress(i);
> - }
>
> - diff -= SAFE_READ_MEMINFO("MemAvailable:");
> + /*
> + * Every 1200000 iterations, calculate the difference in
> memory
> + * availability. If the difference is greater than 10 *
> 1024 (10MB),
> + * increment the sample counter and log the event.
> + */
> + if ((i % 1200000) == 0) {
> + mem_avail = SAFE_READ_MEMINFO("MemAvailable:");
> + diff = mem_avail_prev - mem_avail;
> + diff_total += diff;
> +
> + if (diff > 20 * 1024) {
> + sample++;
> + tst_res(TINFO, "MemAvailable decreased by
> %ld kB at iteration %d", diff, i);
> + }
> +
> + mem_avail_prev = mem_avail;
> + }
> + }
>
> - if (diff > 50 * 1024)
> - tst_res(TFAIL, "Likely kernel memory leak detected");
> + if ((sample > 5) || (diff_total > 100 * 1024))
> + tst_res(TFAIL, "Likely kernel memory leak detected, total
> decrease: %ld kB", diff_total);
> else
> tst_res(TPASS, "No memory leak found");
> }
> --
> 2.45.2
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-05 3:15 [LTP] [PATCH] perf_event_open: improve the memory leak detection Li Wang
2024-07-05 3:30 ` Li Wang
2024-07-17 6:50 ` Li Wang
@ 2024-07-17 8:31 ` Cyril Hrubis
2024-07-18 15:17 ` Martin Doucha
2024-07-18 15:07 ` Martin Doucha
3 siblings, 1 reply; 9+ messages in thread
From: Cyril Hrubis @ 2024-07-17 8:31 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
Hi!
> + /*
> + * Every 1200000 iterations, calculate the difference in memory
> + * availability. If the difference is greater than 10 * 1024 (10MB),
> + * increment the sample counter and log the event.
> + */
> + if ((i % 1200000) == 0) {
> + mem_avail = SAFE_READ_MEMINFO("MemAvailable:");
> + diff = mem_avail_prev - mem_avail;
> + diff_total += diff;
> +
> + if (diff > 20 * 1024) {
Shouldn't this be 10 * 1024 or possibly slightly less than 10 * 1024?
> + sample++;
> + tst_res(TINFO, "MemAvailable decreased by %ld kB at iteration %d", diff, i);
> + }
> +
> + mem_avail_prev = mem_avail;
> + }
> + }
>
> - if (diff > 50 * 1024)
> - tst_res(TFAIL, "Likely kernel memory leak detected");
> + if ((sample > 5) || (diff_total > 100 * 1024))
Maybe this can rather be if ((sample > 5) && (diff_total > 100 * 1024))
That means that the available memory has been eaten by something and
that it happened more or less in a linear fashion when the program was
running.
> + tst_res(TFAIL, "Likely kernel memory leak detected, total decrease: %ld kB", diff_total);
> else
> tst_res(TPASS, "No memory leak found");
> }
> --
> 2.45.2
>
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-17 8:31 ` Cyril Hrubis
@ 2024-07-18 15:17 ` Martin Doucha
2024-07-18 15:20 ` Cyril Hrubis
0 siblings, 1 reply; 9+ messages in thread
From: Martin Doucha @ 2024-07-18 15:17 UTC (permalink / raw)
To: Cyril Hrubis, Li Wang; +Cc: ltp
On 17. 07. 24 10:31, Cyril Hrubis wrote:
> Hi!
>> + /*
>> + * Every 1200000 iterations, calculate the difference in memory
>> + * availability. If the difference is greater than 10 * 1024 (10MB),
>> + * increment the sample counter and log the event.
>> + */
>> + if ((i % 1200000) == 0) {
>> + mem_avail = SAFE_READ_MEMINFO("MemAvailable:");
>> + diff = mem_avail_prev - mem_avail;
>> + diff_total += diff;
>> +
>> + if (diff > 20 * 1024) {
>
> Shouldn't this be 10 * 1024 or possibly slightly less than 10 * 1024?
This should be fine. The test leaks around 38MB between sample checks
and over 350MB in a whole test run.
>
>> + sample++;
>> + tst_res(TINFO, "MemAvailable decreased by %ld kB at iteration %d", diff, i);
>> + }
>> +
>> + mem_avail_prev = mem_avail;
>> + }
>> + }
>>
>> - if (diff > 50 * 1024)
>> - tst_res(TFAIL, "Likely kernel memory leak detected");
>> + if ((sample > 5) || (diff_total > 100 * 1024))
>
> Maybe this can rather be if ((sample > 5) && (diff_total > 100 * 1024))
>
> That means that the available memory has been eaten by something and
> that it happened more or less in a linear fashion when the program was
> running.
Imagine that some other process releases 300MB of memory while the test
is running. If you change the || to &&, you'll get a false negative in
that case. The sampling approach will protect against such interference.
That being said, if the available memory on your test system fluctuates
by 100+MB during a test run that takes <10 seconds, I'd recommend
investigating what's causing such fluctuation. On the test machine I
used to verify this patch, I can see <10MB of difference before and
after running the test on a fixed kernel.
>
>> + tst_res(TFAIL, "Likely kernel memory leak detected, total decrease: %ld kB", diff_total);
>> else
>> tst_res(TPASS, "No memory leak found");
>> }
>> --
>> 2.45.2
>>
>
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-18 15:17 ` Martin Doucha
@ 2024-07-18 15:20 ` Cyril Hrubis
2024-07-18 15:25 ` Martin Doucha
0 siblings, 1 reply; 9+ messages in thread
From: Cyril Hrubis @ 2024-07-18 15:20 UTC (permalink / raw)
To: Martin Doucha; +Cc: ltp
Hi!
> >> + /*
> >> + * Every 1200000 iterations, calculate the difference in memory
> >> + * availability. If the difference is greater than 10 * 1024 (10MB),
> >> + * increment the sample counter and log the event.
> >> + */
> >> + if ((i % 1200000) == 0) {
> >> + mem_avail = SAFE_READ_MEMINFO("MemAvailable:");
> >> + diff = mem_avail_prev - mem_avail;
> >> + diff_total += diff;
> >> +
> >> + if (diff > 20 * 1024) {
> >
> > Shouldn't this be 10 * 1024 or possibly slightly less than 10 * 1024?
>
> This should be fine. The test leaks around 38MB between sample checks
> and over 350MB in a whole test run.
Then we should fix the comment.
> >
> >> + sample++;
> >> + tst_res(TINFO, "MemAvailable decreased by %ld kB at iteration %d", diff, i);
> >> + }
> >> +
> >> + mem_avail_prev = mem_avail;
> >> + }
> >> + }
> >>
> >> - if (diff > 50 * 1024)
> >> - tst_res(TFAIL, "Likely kernel memory leak detected");
> >> + if ((sample > 5) || (diff_total > 100 * 1024))
> >
> > Maybe this can rather be if ((sample > 5) && (diff_total > 100 * 1024))
> >
> > That means that the available memory has been eaten by something and
> > that it happened more or less in a linear fashion when the program was
> > running.
>
> Imagine that some other process releases 300MB of memory while the test
> is running. If you change the || to &&, you'll get a false negative in
> that case. The sampling approach will protect against such interference.
>
> That being said, if the available memory on your test system fluctuates
> by 100+MB during a test run that takes <10 seconds, I'd recommend
> investigating what's causing such fluctuation. On the test machine I
> used to verify this patch, I can see <10MB of difference before and
> after running the test on a fixed kernel.
So shall we remove the diff_total completely?
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-18 15:20 ` Cyril Hrubis
@ 2024-07-18 15:25 ` Martin Doucha
2024-07-19 2:02 ` Li Wang
0 siblings, 1 reply; 9+ messages in thread
From: Martin Doucha @ 2024-07-18 15:25 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp
On 18. 07. 24 17:20, Cyril Hrubis wrote:
>>> Maybe this can rather be if ((sample > 5) && (diff_total > 100 * 1024))
>>>
>>> That means that the available memory has been eaten by something and
>>> that it happened more or less in a linear fashion when the program was
>>> running.
>>
>> Imagine that some other process releases 300MB of memory while the test
>> is running. If you change the || to &&, you'll get a false negative in
>> that case. The sampling approach will protect against such interference.
>>
>> That being said, if the available memory on your test system fluctuates
>> by 100+MB during a test run that takes <10 seconds, I'd recommend
>> investigating what's causing such fluctuation. On the test machine I
>> used to verify this patch, I can see <10MB of difference before and
>> after running the test on a fixed kernel.
>
> So shall we remove the diff_total completely?
No, diff_total is still useful because it checks for smaller leak than
the sum of samples.
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-18 15:25 ` Martin Doucha
@ 2024-07-19 2:02 ` Li Wang
0 siblings, 0 replies; 9+ messages in thread
From: Li Wang @ 2024-07-19 2:02 UTC (permalink / raw)
To: Martin Doucha; +Cc: ltp
Hi Martin, Cyril,
On Thu, Jul 18, 2024 at 11:25 PM Martin Doucha <mdoucha@suse.cz> wrote:
> On 18. 07. 24 17:20, Cyril Hrubis wrote:
> >>> Maybe this can rather be if ((sample > 5) && (diff_total > 100 * 1024))
> >>>
> >>> That means that the available memory has been eaten by something and
> >>> that it happened more or less in a linear fashion when the program was
> >>> running.
> >>
> >> Imagine that some other process releases 300MB of memory while the test
> >> is running. If you change the || to &&, you'll get a false negative in
> >> that case. The sampling approach will protect against such interference.
> >>
> >> That being said, if the available memory on your test system fluctuates
> >> by 100+MB during a test run that takes <10 seconds, I'd recommend
> >> investigating what's causing such fluctuation. On the test machine I
> >> used to verify this patch, I can see <10MB of difference before and
> >> after running the test on a fixed kernel.
> >
> > So shall we remove the diff_total completely?
>
> No, diff_total is still useful because it checks for smaller leak than
> the sum of samples.
>
Thanks so much for your confirmation and test.
I pushed the patch with improvements as you suggested. Let's see how it
goes in future tests.
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [LTP] [PATCH] perf_event_open: improve the memory leak detection
2024-07-05 3:15 [LTP] [PATCH] perf_event_open: improve the memory leak detection Li Wang
` (2 preceding siblings ...)
2024-07-17 8:31 ` Cyril Hrubis
@ 2024-07-18 15:07 ` Martin Doucha
3 siblings, 0 replies; 9+ messages in thread
From: Martin Doucha @ 2024-07-18 15:07 UTC (permalink / raw)
To: Li Wang, ltp
Hi,
I've tested the patch on an affected kernel and it still reproduces the
leak. Small nit below, but otherwise:
Reviewed-by: Martin Doucha <mdoucha@suse.cz>
On 05. 07. 24 5:15, Li Wang wrote:
> The goal is to add more robust memory leak detection by periodically
> sampling memory availability during the test run and checking for
> significant decreases in available memory.
>
> To avoid false postive:
> perf_event_open03.c:95: TFAIL: Likely kernel memory leak detected
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
> .../perf_event_open/perf_event_open03.c | 32 +++++++++++++++----
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c b/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> index 7dd31d3d2..1aab43e82 100644
> --- a/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> +++ b/testcases/kernel/syscalls/perf_event_open/perf_event_open03.c
> @@ -26,6 +26,7 @@
> const int iterations = 12000000;
> static int fd = -1;
> static int runtime;
> +static int sample;
Does this variable need to be global? It's used only in run() and it
gets reset to zero between test iterations.
>
> static void setup(void)
> {
> @@ -77,22 +78,41 @@ static void check_progress(int i)
>
> static void run(void)
> {
> - long diff;
> + long diff, diff_total, mem_avail, mem_avail_prev;
> int i;
>
> - diff = SAFE_READ_MEMINFO("MemAvailable:");
> + sample = 0;
> + diff_total = 0;
> +
> + mem_avail_prev = SAFE_READ_MEMINFO("MemAvailable:");
> tst_timer_start(CLOCK_MONOTONIC);
>
> /* leak about 100MB of RAM */
> for (i = 0; i < iterations; i++) {
> ioctl(fd, PERF_EVENT_IOC_SET_FILTER, "filter,0/0@abcd");
> check_progress(i);
> - }
>
> - diff -= SAFE_READ_MEMINFO("MemAvailable:");
> + /*
> + * Every 1200000 iterations, calculate the difference in memory
> + * availability. If the difference is greater than 10 * 1024 (10MB),
> + * increment the sample counter and log the event.
> + */
> + if ((i % 1200000) == 0) {
> + mem_avail = SAFE_READ_MEMINFO("MemAvailable:");
> + diff = mem_avail_prev - mem_avail;
> + diff_total += diff;
> +
> + if (diff > 20 * 1024) {
> + sample++;
> + tst_res(TINFO, "MemAvailable decreased by %ld kB at iteration %d", diff, i);
> + }
> +
> + mem_avail_prev = mem_avail;
> + }
> + }
>
> - if (diff > 50 * 1024)
> - tst_res(TFAIL, "Likely kernel memory leak detected");
> + if ((sample > 5) || (diff_total > 100 * 1024))
> + tst_res(TFAIL, "Likely kernel memory leak detected, total decrease: %ld kB", diff_total);
> else
> tst_res(TPASS, "No memory leak found");
> }
--
Martin Doucha mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread