From: Petr Vorel <pvorel@suse.cz>
To: Li Wang <liwang@redhat.com>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH] mem: make use of save_restore to simplify code
Date: Wed, 21 Jun 2023 10:35:06 +0200 [thread overview]
Message-ID: <20230621083506.GC361973@pevik> (raw)
In-Reply-To: <20230601105127.55155-1-liwang@redhat.com>
Hi Li,
very nice cleanup, thanks!
...
> +++ b/testcases/kernel/mem/tunable/overcommit_memory.c
> @@ -70,7 +70,6 @@
> #define EXPECT_FAIL 1
> static char *R_opt;
> -static long old_overcommit_memory = -1;
> static long old_overcommit_ratio = -1;
> static long overcommit_ratio;
> static long sum_total;
> @@ -90,16 +89,11 @@ static void setup(void)
> long mem_total, swap_total;
> struct rlimit lim;
> - if (access(PATH_SYSVM "overcommit_memory", F_OK) == -1 ||
> - access(PATH_SYSVM "overcommit_ratio", F_OK) == -1)
> - tst_brk(TCONF, "system doesn't support overcommit_memory");
Here is used TCONF instead of TBROK.
> -
> if (R_opt)
> overcommit_ratio = SAFE_STRTOL(R_opt, 0, LONG_MAX);
> else
> overcommit_ratio = DEFAULT_OVER_RATIO;
> - old_overcommit_memory = get_sys_tune("overcommit_memory");
> old_overcommit_ratio = get_sys_tune("overcommit_ratio");
> mem_total = SAFE_READ_MEMINFO("MemTotal:");
> @@ -128,14 +122,6 @@ static void setup(void)
> tst_res(TINFO, "TotalBatchSize is %ld kB", total_batch_size);
> }
> -static void cleanup(void)
> -{
> - if (old_overcommit_memory != -1)
> - set_sys_tune("overcommit_memory", old_overcommit_memory, 0);
Also third parameter of set_sys_tune() (check) is 0.
> - if (old_overcommit_ratio != -1)
> - set_sys_tune("overcommit_ratio", old_overcommit_ratio, 0);
> -}
> -
> static void overcommit_memory_test(void)
> {
> @@ -269,6 +255,10 @@ static struct tst_test test = {
> {}
> },
> .setup = setup,
> - .cleanup = cleanup,
> .test_all = overcommit_memory_test,
> + .save_restore = (const struct tst_path_val[]) {
> + {"/proc/sys/vm/overcommit_memory", NULL, TST_SR_TBROK},
> + {"/proc/sys/vm/overcommit_ratio", NULL, TST_SR_TBROK},
=> shouldn't be here TST_SR_TCONF instead of TST_SR_TBROK?
I also wonder if testcases/kernel/mem/tunable/max_map_count.c
can replace old_max_map_count with .save_restore (with TST_SR_TCONF).
Also testcases/kernel/mem/tunable/min_free_kbytes.c could use
.save_restore on panic_on_oom and min_free_kbytes, right?
But these two can be done as a separate effort.
Otherwise LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
next prev parent reply other threads:[~2023-06-21 8:35 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-01 10:51 [LTP] [PATCH] mem: make use of save_restore to simplify code Li Wang
2023-06-21 8:35 ` Petr Vorel [this message]
2023-06-22 8:53 ` Li Wang
2023-06-22 10:05 ` Petr Vorel
2023-06-23 1:34 ` Li Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230621083506.GC361973@pevik \
--to=pvorel@suse.cz \
--cc=liwang@redhat.com \
--cc=ltp@lists.linux.it \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).