[LTP] [PATCH v1] swapping01.c: Reporting /proc/meminfo before test and at the moment of the failure

[LTP] [PATCH v1] swapping01.c: Reporting /proc/meminfo before test and at the moment of the failure

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 testcases/kernel/mem/swapping/swapping01.c | 24 +++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
index fc225e4a6..07d89f44b 100644
--- a/testcases/kernel/mem/swapping/swapping01.c
+++ b/testcases/kernel/mem/swapping/swapping01.c
@@ -60,6 +60,17 @@ static long mem_over_max;
 static pid_t pid;
 static unsigned int start_runtime;
 
+static void print_meminfo(char *path)
+{
+	FILE *file;
+	char line[PATH_MAX];
+
+	file = SAFE_FOPEN(path, "r");
+	while (fgets(line, sizeof(line), file))
+		tst_res(TINFO, "%s", line);
+	SAFE_FCLOSE(file);
+}
+
 static void test_swapping(void)
 {
 #ifdef TST_ABI32
@@ -84,6 +95,8 @@ static void test_swapping(void)
 	switch (pid = SAFE_FORK()) {
 	case 0:
 		do_alloc(0);
+		tst_res(TINFO, "Meminfo before check:");
+		print_meminfo("/proc/meminfo");
 		do_alloc(1);
 		exit(0);
 	default:
@@ -108,9 +121,11 @@ static void do_alloc(int allow_raise)
 	long mem_count;
 	void *s;
 
-	if (allow_raise == 1)
+	if (allow_raise == 1) {
+		init_meminfo();
 		tst_res(TINFO, "available physical memory: %ld MB",
 				mem_available_init / 1024);
+	}
 	mem_count = mem_available_init + mem_over;
 	if (allow_raise == 1)
 		tst_res(TINFO, "try to allocate: %ld MB", mem_count / 1024);
@@ -127,6 +142,7 @@ static void check_swapping(void)
 {
 	int status;
 	long swap_free_now, swapped;
+	char proc_status_path[PATH_MAX];
 
 	/* wait child stop */
 	SAFE_WAITPID(pid, &status, WUNTRACED);
@@ -138,6 +154,7 @@ static void check_swapping(void)
 		swap_free_now = SAFE_READ_MEMINFO("SwapFree:");
 		sleep(1);
 		long diff = labs(swap_free_now - SAFE_READ_MEMINFO("SwapFree:"));
+
 		if (diff < 10)
 			break;
 
@@ -146,6 +163,11 @@ static void check_swapping(void)
 
 	swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
 	if (swapped > mem_over_max) {
+		tst_res(TINFO, "Heavy swapping detected, print meminfo:");
+		print_meminfo("/proc/meminfo");
+		tst_res(TINFO, "Heavy swapping detected, print proc status:");
+		sprintf(proc_status_path, "/proc/%d/status", pid);
+		print_meminfo(proc_status_path);
 		kill(pid, SIGCONT);
 		tst_brk(TFAIL, "heavy swapping detected: "
 				"%ld MB swapped.", swapped / 1024);
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapping01.c: Reporting /proc/meminfo before test and at the moment of the failure

[Petr Vorel]

Hi Wei,

nit: the subject is:
"swapping01.c: Reporting /proc/meminfo before test and at the moment of the failure"

This is way too long, please make it shorter (it should be up to 72 chars),
you can write more at other lines (not everything have to be on the subject).

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/mem/swapping/swapping01.c | 24 +++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)

> diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
> index fc225e4a6..07d89f44b 100644
> --- a/testcases/kernel/mem/swapping/swapping01.c
> +++ b/testcases/kernel/mem/swapping/swapping01.c
> @@ -60,6 +60,17 @@ static long mem_over_max;
>  static pid_t pid;
>  static unsigned int start_runtime;

NOTE: we have tst_available_mem() and tst_available_swap() in
lib/tst_memutils.c. Wouldn't be they enough? Or other line from /proc/meminfo
(new function would need to be added)? I'm not sure myself.

Also, is it really needed to print both /proc/meminfo and /proc/PID/status?
And if it's needed, why other testcases/kernel/mem/ tests don't need it?
Notes below are if we decide that we want this (I'd like to hear others).

> +static void print_meminfo(char *path)

You use print_meminfo() for printing /proc/meminfo and /proc/PID/status.
Instead, there could be a generic function safe_print_file(char *path) either in
safe_macros.c or tst_print_file(char *path) in tst_safe_file_at.c (I slightly
prefer the first, there is a mess with files in lib/*.c, there are way too many
files with just few functions).
The function would also print TINFO message which file is printing.

And then, in lib/tst_memutils.c would be tst_print_meminfo() which would call
safe_print_file("/proc/meminfo") and tst_print_status(pid_t pid), which would do
sprintf(proc_status_path, "/proc/%d/status", pid) and call safe_print_file()
with the result.

NOTE: you need to add also function signature to corresponding include/*.h file.
This should be done as a separate commit, using function in swapping01.c would
be as a separate commit.

> +{
> +	FILE *file;
> +	char line[PATH_MAX];
> +
> +	file = SAFE_FOPEN(path, "r");
> +	while (fgets(line, sizeof(line), file))
> +		tst_res(TINFO, "%s", line);
> +	SAFE_FCLOSE(file);
> +}
> +
>  static void test_swapping(void)
>  {
>  #ifdef TST_ABI32
> @@ -84,6 +95,8 @@ static void test_swapping(void)
>  	switch (pid = SAFE_FORK()) {
>  	case 0:
>  		do_alloc(0);
> +		tst_res(TINFO, "Meminfo before check:");
IMHO this TINFO should be removed (TINFO in the printing function is enough).

> +		print_meminfo("/proc/meminfo");


>  		do_alloc(1);
>  		exit(0);
>  	default:
> @@ -108,9 +121,11 @@ static void do_alloc(int allow_raise)
>  	long mem_count;
>  	void *s;

> -	if (allow_raise == 1)
> +	if (allow_raise == 1) {
> +		init_meminfo();
Why this? You haven't mentioned it in the commit message.

>  		tst_res(TINFO, "available physical memory: %ld MB",
>  				mem_available_init / 1024);
> +	}
...

>  	swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
>  	if (swapped > mem_over_max) {
> +		tst_res(TINFO, "Heavy swapping detected, print meminfo:");
Again, I'd prefer generic message in print_meminfo() (on a single place).
IMHO it will be obvious that first print is before test and the second after.

Kind regards,
Petr
> +		print_meminfo("/proc/meminfo");
> +		tst_res(TINFO, "Heavy swapping detected, print proc status:");
> +		sprintf(proc_status_path, "/proc/%d/status", pid);
> +		print_meminfo(proc_status_path);
>  		kill(pid, SIGCONT);
>  		tst_brk(TFAIL, "heavy swapping detected: "
>  				"%ld MB swapped.", swapped / 1024);

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 0/2] Add tst_print_meminfo function in swapping01

[Wei Gao via ltp]

Wei Gao (2):
  tst_memutils.c: Add tst_print_meminfo function
  swapping01.c: Reporting /proc/meminfo during test

 include/tst_memutils.h                     |  6 ++++++
 include/tst_safe_macros.h                  |  2 ++
 lib/safe_macros.c                          | 16 ++++++++++++++++
 lib/tst_memutils.c                         |  5 +++++
 testcases/kernel/mem/swapping/swapping01.c |  6 ++++--
 5 files changed, 33 insertions(+), 2 deletions(-)

-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 1/2] tst_memutils.c: Add tst_print_meminfo function

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 include/tst_memutils.h    |  6 ++++++
 include/tst_safe_macros.h |  2 ++
 lib/safe_macros.c         | 16 ++++++++++++++++
 lib/tst_memutils.c        |  5 +++++
 4 files changed, 29 insertions(+)

diff --git a/include/tst_memutils.h b/include/tst_memutils.h
index 19b593430..439b2485a 100644
--- a/include/tst_memutils.h
+++ b/include/tst_memutils.h
@@ -58,4 +58,10 @@ void tst_enable_oom_protection(pid_t pid);
  */
 void tst_disable_oom_protection(pid_t pid);
 
+void tst_print_meminfo(void);
+
+void tst_print_meminfo_(const char *file, const int lineno);
+
+#define tst_print_meminfo() tst_print_meminfo_(__FILE__, __LINE__)
+
 #endif /* TST_MEMUTILS_H__ */
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index c899c4f2c..520a173dd 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -671,4 +671,6 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
 #define SAFE_SYSINFO(info) \
 	safe_sysinfo(__FILE__, __LINE__, (info))
 
+int safe_print_file(const char *file, const int lineno, char *path);
+
 #endif /* SAFE_MACROS_H__ */
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 951e1b064..bb67467b7 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -1352,3 +1352,19 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info)
 
 	return ret;
 }
+
+int safe_print_file(const char *file, const int lineno, char *path)
+{
+	int ret;
+	FILE *pfile;
+	char line[PATH_MAX];
+
+	pfile = safe_fopen(file, lineno, NULL, path, "r");
+
+	while (fgets(line, sizeof(line), pfile))
+		tst_resm_(file, lineno, TINFO, "%s", line);
+
+	ret = safe_fclose(file, lineno, NULL, pfile);
+
+	return ret;
+}
diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
index c5382ff10..6c1e7c29e 100644
--- a/lib/tst_memutils.c
+++ b/lib/tst_memutils.c
@@ -182,3 +182,8 @@ void tst_disable_oom_protection(pid_t pid)
 {
 	set_oom_score_adj(pid, 0);
 }
+
+void tst_print_meminfo_(const char *file, const int lineno)
+{
+	safe_print_file(file, lineno, "/proc/meminfo");
+}
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 2/2] swapping01.c: Reporting /proc/meminfo during test

[Wei Gao via ltp]

Get clear overview memory status during test is good for debug, such as
get report before the test and also at the moment of the failure.

Also i move init_meminfo() into do_alloc function since do_alloc will be
called twice during test and we need fresh data. This will be a small
improvement for 1b53999.

Signed-off-by: Wei Gao <wegao@suse.com>
---
 testcases/kernel/mem/swapping/swapping01.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
index fc225e4a6..114456a19 100644
--- a/testcases/kernel/mem/swapping/swapping01.c
+++ b/testcases/kernel/mem/swapping/swapping01.c
@@ -79,11 +79,10 @@ static void test_swapping(void)
 	}
 	SAFE_FCLOSE(file);
 
-	init_meminfo();
-
 	switch (pid = SAFE_FORK()) {
 	case 0:
 		do_alloc(0);
+		tst_print_meminfo();
 		do_alloc(1);
 		exit(0);
 	default:
@@ -108,6 +107,8 @@ static void do_alloc(int allow_raise)
 	long mem_count;
 	void *s;
 
+	init_meminfo();
+
 	if (allow_raise == 1)
 		tst_res(TINFO, "available physical memory: %ld MB",
 				mem_available_init / 1024);
@@ -146,6 +147,7 @@ static void check_swapping(void)
 
 	swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
 	if (swapped > mem_over_max) {
+		tst_print_meminfo();
 		kill(pid, SIGCONT);
 		tst_brk(TFAIL, "heavy swapping detected: "
 				"%ld MB swapped.", swapped / 1024);
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 0/2] Add tst_print_meminfo function into swapping01

[Wei Gao via ltp]

Wei Gao (2):
  tst_memutils.c: Add tst_print_meminfo function
  swapping01.c: Reporting /proc/meminfo during test

 include/tst_memutils.h                     |  6 ++++++
 include/tst_safe_macros.h                  |  2 ++
 lib/safe_macros.c                          | 16 ++++++++++++++++
 lib/tst_memutils.c                         |  5 +++++
 testcases/kernel/mem/swapping/swapping01.c |  3 +++
 5 files changed, 32 insertions(+)

-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 include/tst_memutils.h    |  6 ++++++
 include/tst_safe_macros.h |  2 ++
 lib/safe_macros.c         | 16 ++++++++++++++++
 lib/tst_memutils.c        |  5 +++++
 4 files changed, 29 insertions(+)

diff --git a/include/tst_memutils.h b/include/tst_memutils.h
index 19b593430..439b2485a 100644
--- a/include/tst_memutils.h
+++ b/include/tst_memutils.h
@@ -58,4 +58,10 @@ void tst_enable_oom_protection(pid_t pid);
  */
 void tst_disable_oom_protection(pid_t pid);
 
+void tst_print_meminfo(void);
+
+void tst_print_meminfo_(const char *file, const int lineno);
+
+#define tst_print_meminfo() tst_print_meminfo_(__FILE__, __LINE__)
+
 #endif /* TST_MEMUTILS_H__ */
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index c899c4f2c..520a173dd 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -671,4 +671,6 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
 #define SAFE_SYSINFO(info) \
 	safe_sysinfo(__FILE__, __LINE__, (info))
 
+int safe_print_file(const char *file, const int lineno, char *path);
+
 #endif /* SAFE_MACROS_H__ */
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 951e1b064..bb67467b7 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -1352,3 +1352,19 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info)
 
 	return ret;
 }
+
+int safe_print_file(const char *file, const int lineno, char *path)
+{
+	int ret;
+	FILE *pfile;
+	char line[PATH_MAX];
+
+	pfile = safe_fopen(file, lineno, NULL, path, "r");
+
+	while (fgets(line, sizeof(line), pfile))
+		tst_resm_(file, lineno, TINFO, "%s", line);
+
+	ret = safe_fclose(file, lineno, NULL, pfile);
+
+	return ret;
+}
diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
index c5382ff10..6c1e7c29e 100644
--- a/lib/tst_memutils.c
+++ b/lib/tst_memutils.c
@@ -182,3 +182,8 @@ void tst_disable_oom_protection(pid_t pid)
 {
 	set_oom_score_adj(pid, 0);
 }
+
+void tst_print_meminfo_(const char *file, const int lineno)
+{
+	safe_print_file(file, lineno, "/proc/meminfo");
+}
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Petr Vorel]

Hi Wei,

...
> +++ b/include/tst_memutils.h
> @@ -58,4 +58,10 @@ void tst_enable_oom_protection(pid_t pid);
>   */
>  void tst_disable_oom_protection(pid_t pid);

> +void tst_print_meminfo(void);
> +
> +void tst_print_meminfo_(const char *file, const int lineno);
> +
> +#define tst_print_meminfo() tst_print_meminfo_(__FILE__, __LINE__)
Most of the macros we have upper case, can it be please TST_PRINT_MEMINFO() ?
I guess it does not have to be SAFE_PRINT_MEMINFO().

And because it's just one liner, could it be:

#define TST_PRINT_MEMINFO() safe_print_file(__FILE__, __LINE__, "/proc/meminfo")

...

> +++ b/lib/safe_macros.c

We don't want to add anything to the legacy API (otherwise it would go to
lib/safe_file_ops.c), please add this to lib/tst_safe_macros.c.

BTW I'm slightly confused, what would be the best place for this,
lib/tst_safe_macros.c is being used nowadays for everything. But there is also
include/tst_safe_file_ops.h, which does not have C file
(lib/tst_safe_file_ops.c) because it internally use lib/tst_safe_macros.c.
I guess creating lib/tst_safe_macros.c was postponed until we rewrite all tests,
maybe it's a time to create it.

@Li @Cyril: Also include/tst_safe_file_ops.h has SAFE_READ_MEMINFO() and
SAFE_READ_PROC_STATUS(), IMHO these should be in include/tst_memutils.h.
Or, we shouldn't have 2 headers for similar thing, it would be good to merge
these two.

> @@ -1352,3 +1352,19 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info)

>  	return ret;
>  }
> +
> +int safe_print_file(const char *file, const int lineno, char *path)
> +{
> +	int ret;
> +	FILE *pfile;
> +	char line[PATH_MAX];
> +
> +	pfile = safe_fopen(file, lineno, NULL, path, "r");
> +
> +	while (fgets(line, sizeof(line), pfile))
> +		tst_resm_(file, lineno, TINFO, "%s", line);
> +
> +	ret = safe_fclose(file, lineno, NULL, pfile);
> +
> +	return ret;
> +}
> diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
> index c5382ff10..6c1e7c29e 100644
> --- a/lib/tst_memutils.c
> +++ b/lib/tst_memutils.c
> @@ -182,3 +182,8 @@ void tst_disable_oom_protection(pid_t pid)
>  {
>  	set_oom_score_adj(pid, 0);
>  }
> +
> +void tst_print_meminfo_(const char *file, const int lineno)
> +{
> +	safe_print_file(file, lineno, "/proc/meminfo");
As I mentioned above, we try to avoid function wrappers.
> +}

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Li Wang]

Hi Petr, All,

On Sat, Dec 16, 2023 at 2:58 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Wei,
>
> ...
> > +++ b/include/tst_memutils.h
> > @@ -58,4 +58,10 @@ void tst_enable_oom_protection(pid_t pid);
> >   */
> >  void tst_disable_oom_protection(pid_t pid);
>
> > +void tst_print_meminfo(void);
> > +
> > +void tst_print_meminfo_(const char *file, const int lineno);
> > +
> > +#define tst_print_meminfo() tst_print_meminfo_(__FILE__, __LINE__)
> Most of the macros we have upper case, can it be please
> TST_PRINT_MEMINFO() ?
> I guess it does not have to be SAFE_PRINT_MEMINFO().
>
> And because it's just one liner, could it be:
>
> #define TST_PRINT_MEMINFO() safe_print_file(__FILE__, __LINE__,
> "/proc/meminfo")
>
> ...
>
> > +++ b/lib/safe_macros.c
>
> We don't want to add anything to the legacy API (otherwise it would go to
> lib/safe_file_ops.c), please add this to lib/tst_safe_macros.c.
>
> BTW I'm slightly confused, what would be the best place for this,
> lib/tst_safe_macros.c is being used nowadays for everything. But there is
> also
>


> include/tst_safe_file_ops.h, which does not have C file
> (lib/tst_safe_file_ops.c) because it internally use lib/tst_safe_macros.c.
>

No, basically it does not use the lib/tst_safe_macros.c.

From what I understand, 'tst_safe_file_ops.h' is just a header for
collecting
all the file operations for Linux, it doesn't touch other subcomponent ops.

'tst_safe_file_ops.h' defines macros for all functions in
'safe_file_ops_fn.h'
and archived them in 'safe_file_ops.c' lib.

Something like this combination:

tst_safe_file_ops.h:
    safe_file_ops_fn.h
    safe_file_ops.c

tst_safe_macros.h
tst_safe_macros.c



> I guess creating lib/tst_safe_macros.c was postponed until we rewrite all
> tests,
> maybe it's a time to create it.
>



> @Li @Cyril: Also include/tst_safe_file_ops.h has SAFE_READ_MEMINFO() and
> SAFE_READ_PROC_STATUS(), IMHO these should be in include/tst_memutils.h.
> Or, we shouldn't have 2 headers for similar thing, it would be good to
> merge
> these two.
>

Agreed, anything related to the dedicated ops should be put into the
corresponding header files. tst_safe_file_ops.h is a generic operation
for Linux (but not for specific) files. So I vote for adding *_MEMINFO()
to tst_memutil.h.



>
> > @@ -1352,3 +1352,19 @@ int safe_sysinfo(const char *file, const int
> lineno, struct sysinfo *info)
>
> >       return ret;
> >  }
> > +
> > +int safe_print_file(const char *file, const int lineno, char *path)
> > +{
> > +     int ret;
> > +     FILE *pfile;
> > +     char line[PATH_MAX];
> > +
> > +     pfile = safe_fopen(file, lineno, NULL, path, "r");
> > +
> > +     while (fgets(line, sizeof(line), pfile))
> > +             tst_resm_(file, lineno, TINFO, "%s", line);
> > +
> > +     ret = safe_fclose(file, lineno, NULL, pfile);
> > +
> > +     return ret;
> > +}
> > diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
> > index c5382ff10..6c1e7c29e 100644
> > --- a/lib/tst_memutils.c
> > +++ b/lib/tst_memutils.c
> > @@ -182,3 +182,8 @@ void tst_disable_oom_protection(pid_t pid)
> >  {
> >       set_oom_score_adj(pid, 0);
> >  }
> > +
> > +void tst_print_meminfo_(const char *file, const int lineno)
> > +{
> > +     safe_print_file(file, lineno, "/proc/meminfo");
> As I mentioned above, we try to avoid function wrappers.
> > +}
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Li Wang]

Li Wang <liwang@redhat.com> wrote:


>
>> > +++ b/lib/safe_macros.c
>>
>> We don't want to add anything to the legacy API (otherwise it would go to
>> lib/safe_file_ops.c), please add this to lib/tst_safe_macros.c.
>>
>> BTW I'm slightly confused, what would be the best place for this,
>> lib/tst_safe_macros.c is being used nowadays for everything. But there is
>> also
>>
>
>
>> include/tst_safe_file_ops.h, which does not have C file
>> (lib/tst_safe_file_ops.c) because it internally use lib/tst_safe_macros.c.
>>
>
> No, basically it does not use the lib/tst_safe_macros.c.
>
> From what I understand, 'tst_safe_file_ops.h' is just a header for
> collecting
> all the file operations for Linux, it doesn't touch other subcomponent ops.
>
> 'tst_safe_file_ops.h' defines macros for all functions in
> 'safe_file_ops_fn.h'
> and archived them in 'safe_file_ops.c' lib.
>
> Something like this combination:
>
> tst_safe_file_ops.h:
>     safe_file_ops_fn.h
>     safe_file_ops.c
>

The reason to split them into two headers is for backward compatibility.

Technically we should declare 'safe_file_ops_fn.h' functions in one header
'tst_safe_file_ops.h' but you know we have 'old_safe_file_ops.h'.

tst_safe_file_ops.h:
    safe_file_ops_fn.h
    safe_file_ops.c

old_safe_file_ops.h:
    safe_file_ops_fn.h
    safe_file_ops.c

In the future, the ideal cleanup direction is just to have:

tst_safe_file_ops.h
tst_safe_file_ops.c


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Petr Vorel]

> Li Wang <liwang@redhat.com> wrote:

> >> > +++ b/lib/safe_macros.c

> >> We don't want to add anything to the legacy API (otherwise it would go to
> >> lib/safe_file_ops.c), please add this to lib/tst_safe_macros.c.

> >> BTW I'm slightly confused, what would be the best place for this,
> >> lib/tst_safe_macros.c is being used nowadays for everything. But there is
> >> also



> >> include/tst_safe_file_ops.h, which does not have C file
> >> (lib/tst_safe_file_ops.c) because it internally use lib/tst_safe_macros.c.


> > No, basically it does not use the lib/tst_safe_macros.c.

> > From what I understand, 'tst_safe_file_ops.h' is just a header for
> > collecting
> > all the file operations for Linux, it doesn't touch other subcomponent ops.

> > 'tst_safe_file_ops.h' defines macros for all functions in
> > 'safe_file_ops_fn.h'
> > and archived them in 'safe_file_ops.c' lib.

> > Something like this combination:

> > tst_safe_file_ops.h:
> >     safe_file_ops_fn.h
> >     safe_file_ops.c


> The reason to split them into two headers is for backward compatibility.

> Technically we should declare 'safe_file_ops_fn.h' functions in one header
> 'tst_safe_file_ops.h' but you know we have 'old_safe_file_ops.h'.

> tst_safe_file_ops.h:
>     safe_file_ops_fn.h
>     safe_file_ops.c

> old_safe_file_ops.h:
>     safe_file_ops_fn.h
>     safe_file_ops.c

> In the future, the ideal cleanup direction is just to have:

> tst_safe_file_ops.h
> tst_safe_file_ops.c

Yep, the problem that Cyril originally implemented new API via legacy to
support both APIs (good and needed solution) and now we add only into new API
(also good solution) + that approach specific (and thus small) C sources
initially and later more generic (and longer) makes library a bit inconsistent.
I know, it will be solved once we get rid of the legacy API, but that will take
time.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Petr Vorel]

Hi Li, all,

> Hi Petr, All,

> On Sat, Dec 16, 2023 at 2:58 AM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi Wei,

> > ...
> > > +++ b/include/tst_memutils.h
> > > @@ -58,4 +58,10 @@ void tst_enable_oom_protection(pid_t pid);
> > >   */
> > >  void tst_disable_oom_protection(pid_t pid);

> > > +void tst_print_meminfo(void);
> > > +
> > > +void tst_print_meminfo_(const char *file, const int lineno);
> > > +
> > > +#define tst_print_meminfo() tst_print_meminfo_(__FILE__, __LINE__)
> > Most of the macros we have upper case, can it be please
> > TST_PRINT_MEMINFO() ?
> > I guess it does not have to be SAFE_PRINT_MEMINFO().

> > And because it's just one liner, could it be:

> > #define TST_PRINT_MEMINFO() safe_print_file(__FILE__, __LINE__,
> > "/proc/meminfo")

> > ...

> > > +++ b/lib/safe_macros.c

> > We don't want to add anything to the legacy API (otherwise it would go to
> > lib/safe_file_ops.c), please add this to lib/tst_safe_macros.c.

> > BTW I'm slightly confused, what would be the best place for this,
> > lib/tst_safe_macros.c is being used nowadays for everything. But there is
> > also



> > include/tst_safe_file_ops.h, which does not have C file
> > (lib/tst_safe_file_ops.c) because it internally use lib/tst_safe_macros.c.


> No, basically it does not use the lib/tst_safe_macros.c.

You're right.

> From what I understand, 'tst_safe_file_ops.h' is just a header for
> collecting
> all the file operations for Linux, it doesn't touch other subcomponent ops.

Thanks! Now it's obvious.

> 'tst_safe_file_ops.h' defines macros for all functions in
> 'safe_file_ops_fn.h'
> and archived them in 'safe_file_ops.c' lib.

> Something like this combination:

> tst_safe_file_ops.h:
>     safe_file_ops_fn.h
>     safe_file_ops.c

> tst_safe_macros.h
> tst_safe_macros.c



> > I guess creating lib/tst_safe_macros.c was postponed until we rewrite all
> > tests,
> > maybe it's a time to create it.




> > @Li @Cyril: Also include/tst_safe_file_ops.h has SAFE_READ_MEMINFO() and
> > SAFE_READ_PROC_STATUS(), IMHO these should be in include/tst_memutils.h.
> > Or, we shouldn't have 2 headers for similar thing, it would be good to
> > merge
> > these two.


> Agreed, anything related to the dedicated ops should be put into the
> corresponding header files. tst_safe_file_ops.h is a generic operation
> for Linux (but not for specific) files. So I vote for adding *_MEMINFO()
> to tst_memutil.h.

+1

I understand that it's a good idea when we separate things according to e.g.
libc header. But we also have separated C files in lib/.
It's probably easier if we have more shorter files than fewer very long files,
but I wonder if some sourcers should not be in single files, e.g. these:
tst_supported_fs_types.c, tst_fs_type.c => tst_fs.c
or
tst_fill_fs.c, tst_fill_file.c => tst_fill.c
or
tst_fs_setup.c, tst_path_has_mnt_flags.c => tst_mount.c
(into some more generic name)

Nothing critical, it just having 1-3 functions in separate source makes actually
search harder, because file name is very specific

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/2] tst_memutils.c: Add tst_print_meminfo function

[Li Wang]

Petr Vorel <pvorel@suse.cz> wrote:


>
> > Agreed, anything related to the dedicated ops should be put into the
> > corresponding header files. tst_safe_file_ops.h is a generic operation
> > for Linux (but not for specific) files. So I vote for adding *_MEMINFO()
> > to tst_memutil.h.
>
> +1
>
> I understand that it's a good idea when we separate things according to
> e.g.
> libc header. But we also have separated C files in lib/.
> It's probably easier if we have more shorter files than fewer very long
> files,
> but I wonder if some sourcers should not be in single files, e.g. these:
> tst_supported_fs_types.c, tst_fs_type.c => tst_fs.c
> or
> tst_fill_fs.c, tst_fill_file.c => tst_fill.c
> or
> tst_fs_setup.c, tst_path_has_mnt_flags.c => tst_mount.c
> (into some more generic name)
>


Yes, I think so. Unless there is some necessary reason (I overlooked) to
prevent mergeing them together.


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 2/2] swapping01.c: Reporting /proc/meminfo during test

[Wei Gao via ltp]

Get clear overview memory status during test is good for debug, such as
get report before the test and also at the moment of the failure.

Signed-off-by: Wei Gao <wegao@suse.com>
---
 testcases/kernel/mem/swapping/swapping01.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
index fc225e4a6..f7724e7e1 100644
--- a/testcases/kernel/mem/swapping/swapping01.c
+++ b/testcases/kernel/mem/swapping/swapping01.c
@@ -83,7 +83,9 @@ static void test_swapping(void)
 
 	switch (pid = SAFE_FORK()) {
 	case 0:
+		tst_print_meminfo();
 		do_alloc(0);
+		tst_print_meminfo();
 		do_alloc(1);
 		exit(0);
 	default:
@@ -146,6 +148,7 @@ static void check_swapping(void)
 
 	swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
 	if (swapped > mem_over_max) {
+		tst_print_meminfo();
 		kill(pid, SIGCONT);
 		tst_brk(TFAIL, "heavy swapping detected: "
 				"%ld MB swapped.", swapped / 1024);
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/2] swapping01.c: Reporting /proc/meminfo during test

[Li Wang]

Hi Wei,

The print of meminfo will help to debug but the negative
is to mess up the test log in LTP.

So, can we achieve an implicit way for debug printing and only
enable it by using additional parameters?

e.g. ./swapping01 -D (--debug)


On Thu, Dec 14, 2023 at 3:14 PM Wei Gao via ltp <ltp@lists.linux.it> wrote:

> Get clear overview memory status during test is good for debug, such as
> get report before the test and also at the moment of the failure.
>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/mem/swapping/swapping01.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/testcases/kernel/mem/swapping/swapping01.c
> b/testcases/kernel/mem/swapping/swapping01.c
> index fc225e4a6..f7724e7e1 100644
> --- a/testcases/kernel/mem/swapping/swapping01.c
> +++ b/testcases/kernel/mem/swapping/swapping01.c
> @@ -83,7 +83,9 @@ static void test_swapping(void)
>
>         switch (pid = SAFE_FORK()) {
>         case 0:
> +               tst_print_meminfo();
>                 do_alloc(0);
> +               tst_print_meminfo();
>                 do_alloc(1);
>                 exit(0);
>         default:
> @@ -146,6 +148,7 @@ static void check_swapping(void)
>
>         swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
>         if (swapped > mem_over_max) {
> +               tst_print_meminfo();
>                 kill(pid, SIGCONT);
>                 tst_brk(TFAIL, "heavy swapping detected: "
>                                 "%ld MB swapped.", swapped / 1024);
> --
> 2.35.3
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 0/2] Add tst_print_meminfo function into swapping01

[Wei Gao via ltp]

Wei Gao (2):
  tst_memutils.c: Add tst_print_meminfo function
  swapping01.c: Reporting /proc/meminfo during test

 include/tst_memutils.h                     |  2 ++
 include/tst_safe_macros.h                  |  3 +++
 lib/tst_safe_macros.c                      | 13 +++++++++++++
 testcases/kernel/mem/swapping/swapping01.c |  4 ++++
 4 files changed, 22 insertions(+)

-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 1/2] tst_memutils.c: Add tst_print_meminfo function

[Wei Gao via ltp]

Signed-off-by: Wei Gao <wegao@suse.com>
---
 include/tst_memutils.h    |  2 ++
 include/tst_safe_macros.h |  3 +++
 lib/tst_safe_macros.c     | 13 +++++++++++++
 3 files changed, 18 insertions(+)

diff --git a/include/tst_memutils.h b/include/tst_memutils.h
index 19b593430..0dd941ced 100644
--- a/include/tst_memutils.h
+++ b/include/tst_memutils.h
@@ -58,4 +58,6 @@ void tst_enable_oom_protection(pid_t pid);
  */
 void tst_disable_oom_protection(pid_t pid);
 
+#define TST_PRINT_MEMINFO() safe_print_file(__FILE__, __LINE__, "/proc/meminfo")
+
 #endif /* TST_MEMUTILS_H__ */
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index c899c4f2c..f2ce8919b 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -21,6 +21,7 @@
 #include <dirent.h>
 #include <grp.h>
 
+#include "safe_stdio_fn.h"
 #include "safe_macros_fn.h"
 #include "tst_cmd.h"
 
@@ -671,4 +672,6 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
 #define SAFE_SYSINFO(info) \
 	safe_sysinfo(__FILE__, __LINE__, (info))
 
+void safe_print_file(const char *file, const int lineno, char *path);
+
 #endif /* SAFE_MACROS_H__ */
diff --git a/lib/tst_safe_macros.c b/lib/tst_safe_macros.c
index 7f28d43e5..024963bab 100644
--- a/lib/tst_safe_macros.c
+++ b/lib/tst_safe_macros.c
@@ -610,3 +610,16 @@ int safe_msync(const char *file, const int lineno, void *addr,
 
 	return rval;
 }
+
+void safe_print_file(const char *file, const int lineno, char *path)
+{
+	FILE *pfile;
+	char line[PATH_MAX];
+
+	pfile = safe_fopen(file, lineno, NULL, path, "r");
+
+	while (fgets(line, sizeof(line), pfile))
+		tst_resm_(file, lineno, TINFO, "%s", line);
+
+	safe_fclose(file, lineno, NULL, pfile);
+}
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 1/2] tst_memutils.c: Add tst_print_meminfo function

[Petr Vorel]

Hi Wei,

> +void safe_print_file(const char *file, const int lineno, char *path)
> +{
> +	FILE *pfile;
> +	char line[PATH_MAX];
> +
> +	pfile = safe_fopen(file, lineno, NULL, path, "r");
> +
I'd expect here TINFO message:

tst_res(TINFO, "=== %s ===", path);

> +	while (fgets(line, sizeof(line), pfile))
> +		tst_resm_(file, lineno, TINFO, "%s", line);
This will not work, that's legacy API.

Please next time pay attention with warnings:

tst_safe_macros.c: In function ‘safe_print_file’:
tst_safe_macros.c:622:17: warning: implicit declaration of function ‘tst_resm_’; did you mean ‘tst_res_’? [-Wimplicit-function-declaration]
  622 |                 tst_resm_(file, lineno, TINFO, "%s", line);
      |                 ^~~~~~~~~
      |                 tst_res_


Also, if you push to github, you see this is fatal for CI, where we compile with
-Werror=implicit-function-declaration.

Please next time push to github first, so that you save reviewer time.

Also, when you run the test, the output contains extra new line:

tst_kconfig.c:87: TINFO: Parsing kernel config '/proc/config.gz'
tst_test.c:1690: TINFO: LTP version: 20230929-201-ge32735252
tst_test.c:1574: TINFO: Timeout per run is 0h 10m 30s
tst_safe_macros.c:619: TINFO: === /proc/meminfo ===
swapping01.c:88: TINFO: MemTotal:        7700224 kB

swapping01.c:88: TINFO: MemFree:         4058996 kB

swapping01.c:88: TINFO: MemAvailable:    4172736 kB
...

We could remove it, but actually printing it via fprintf is actually better:

tst_kconfig.c:87: TINFO: Parsing kernel config '/proc/config.gz'
tst_test.c:1690: TINFO: LTP version: 20230929-201-ge32735252
tst_test.c:1574: TINFO: Timeout per run is 0h 10m 30s
tst_safe_macros.c:619: TINFO: === /proc/meminfo ===
MemTotal:        1467180 kB
MemFree:          820040 kB
MemAvailable:    1143320 kB
...

Anyway, fixed and merged this patch.

Kind regards,
Petr

> +
> +	safe_fclose(file, lineno, NULL, pfile);
> +}

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 2/2] swapping01.c: Reporting /proc/meminfo during test

[Wei Gao via ltp]

Get clear overview memory status during test is good for debug, such as
get report before the test and also at the moment of the failure.

Signed-off-by: Wei Gao <wegao@suse.com>
---
 testcases/kernel/mem/swapping/swapping01.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
index fc225e4a6..025f44363 100644
--- a/testcases/kernel/mem/swapping/swapping01.c
+++ b/testcases/kernel/mem/swapping/swapping01.c
@@ -83,7 +83,9 @@ static void test_swapping(void)
 
 	switch (pid = SAFE_FORK()) {
 	case 0:
+		TST_PRINT_MEMINFO();
 		do_alloc(0);
+		TST_PRINT_MEMINFO();
 		do_alloc(1);
 		exit(0);
 	default:
@@ -138,6 +140,7 @@ static void check_swapping(void)
 		swap_free_now = SAFE_READ_MEMINFO("SwapFree:");
 		sleep(1);
 		long diff = labs(swap_free_now - SAFE_READ_MEMINFO("SwapFree:"));
+
 		if (diff < 10)
 			break;
 
@@ -146,6 +149,7 @@ static void check_swapping(void)
 
 	swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
 	if (swapped > mem_over_max) {
+		TST_PRINT_MEMINFO();
 		kill(pid, SIGCONT);
 		tst_brk(TFAIL, "heavy swapping detected: "
 				"%ld MB swapped.", swapped / 1024);
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 2/2] swapping01.c: Reporting /proc/meminfo during test

[Petr Vorel]

Hi Wei,

> Get clear overview memory status during test is good for debug, such as
> get report before the test and also at the moment of the failure.

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/mem/swapping/swapping01.c | 4 ++++
>  1 file changed, 4 insertions(+)

> diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
> index fc225e4a6..025f44363 100644
> --- a/testcases/kernel/mem/swapping/swapping01.c
> +++ b/testcases/kernel/mem/swapping/swapping01.c
> @@ -83,7 +83,9 @@ static void test_swapping(void)

>  	switch (pid = SAFE_FORK()) {
>  	case 0:
> +		TST_PRINT_MEMINFO();
>  		do_alloc(0);
> +		TST_PRINT_MEMINFO();
Developer asked you to print /proc/meminfo "before the test starts and at the
moment of the failure" Wouldn't be just this second TST_PRINT_MEMINFO() enough?

Kind regards,
Petr

>  		do_alloc(1);
>  		exit(0);
>  	default:
> @@ -138,6 +140,7 @@ static void check_swapping(void)
>  		swap_free_now = SAFE_READ_MEMINFO("SwapFree:");
>  		sleep(1);
>  		long diff = labs(swap_free_now - SAFE_READ_MEMINFO("SwapFree:"));
> +
>  		if (diff < 10)
>  			break;

> @@ -146,6 +149,7 @@ static void check_swapping(void)

>  	swapped = SAFE_READ_PROC_STATUS(pid, "VmSwap:");
>  	if (swapped > mem_over_max) {
> +		TST_PRINT_MEMINFO();
>  		kill(pid, SIGCONT);
>  		tst_brk(TFAIL, "heavy swapping detected: "
>  				"%ld MB swapped.", swapped / 1024);

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 2/2] swapping01.c: Reporting /proc/meminfo during test

[Petr Vorel]

> Hi Wei,

> > Get clear overview memory status during test is good for debug, such as
> > get report before the test and also at the moment of the failure.

> > Signed-off-by: Wei Gao <wegao@suse.com>
> > ---
> >  testcases/kernel/mem/swapping/swapping01.c | 4 ++++
> >  1 file changed, 4 insertions(+)

> > diff --git a/testcases/kernel/mem/swapping/swapping01.c b/testcases/kernel/mem/swapping/swapping01.c
> > index fc225e4a6..025f44363 100644
> > --- a/testcases/kernel/mem/swapping/swapping01.c
> > +++ b/testcases/kernel/mem/swapping/swapping01.c
> > @@ -83,7 +83,9 @@ static void test_swapping(void)

> >  	switch (pid = SAFE_FORK()) {
> >  	case 0:
> > +		TST_PRINT_MEMINFO();
> >  		do_alloc(0);
> > +		TST_PRINT_MEMINFO();
> Developer asked you to print /proc/meminfo "before the test starts and at the
> moment of the failure" Wouldn't be just this second TST_PRINT_MEMINFO() enough?

OK, I merged this. Thank you.

Kind regards,
Petr

> Kind regards,
> Petr

> >  		do_alloc(1);
> >  		exit(0);

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp