[PATCH 0/3] selftests: cachestat: fix build and run on older kernels

[PATCH 0/3] selftests: cachestat: fix build and run on older kernels

[Andre Przywara]

I ran all kernel selftests on some test machine, and stumbled upon
cachestat failing (among others).
Those patches fix the cachestat test compilation and run on older
kernels.

Also I found that the but-last test (on a normal file) fails when run on
a tmpfs mounted directory, as it happens on an initramfs-only system, or
when the current directory happens to be /dev/shm or /tmp:
# Create/open tmpfilecachestat
# Cachestat call returned 0
# Using cachestat: Cached: 4, Dirty: 4, Writeback: 0, Evicted: 0, Recently Evicted: 0
# Cachestat call (after fsync) returned 0
# Using cachestat: Cached: 4, Dirty: 4, Writeback: 0, Evicted: 0, Recently Evicted: 0
# Number of dirty should be zero after fsync.
not ok 6 cachestat fails with normal file

That same test binary succeeds on the same machine right afterwards if
the current directory is changed to an ext4 filesystem.

I don't really know if this is expected, and whether we should try to
figure out if the test file lives on a tmpfs filesystem, or whether the
test itself is not strict enough, and requires more "flushing"
(drop_caches?) to cover tmpfs directories as well.

Any ideas how to fix this would be appreciated.

Cheers,
Andre

Andre Przywara (3):
  selftests: cachestat: properly link in librt
  selftests: cachestat: use proper syscall number macro
  selftests: cachestat: test for cachestat availability

 tools/testing/selftests/cachestat/Makefile    |  2 +-
 .../selftests/cachestat/test_cachestat.c      | 29 +++++++++++++++----
 2 files changed, 25 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/3] selftests: cachestat: properly link in librt

[Andre Przywara]

Libraries should be listed last on the compiler's command line, so that
the linker can look for and find still unresolved symbols. The librt
library, required for the shm_* functions, was announced using CFLAGS,
which puts the library *before* the source files, and fails compilation
on my system:
======================
gcc -isystem /src/linux-selftests/usr/include -Wall -lrt test_cachestat.c
  -o /src/linux-selftests/kselftest/cachestat/test_cachestat
/usr/bin/ld: /tmp/cceQWO3u.o: in function `test_cachestat_shmem':
test_cachestat.c:(.text+0x890): undefined reference to `shm_open'
/usr/bin/ld: test_cachestat.c:(.text+0x99c): undefined reference to `shm_unlink'
collect2: error: ld returned 1 exit status
make[4]: *** [../lib.mk:181: /src/linux-selftests/kselftest/cachestat/test_cachestat] Error 1
======================

Announce the library using the LDLIBS variable, which ensures the proper
ordering on the command line.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/cachestat/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cachestat/Makefile b/tools/testing/selftests/cachestat/Makefile
index fca73aaa7d141..778b54ebb0364 100644
--- a/tools/testing/selftests/cachestat/Makefile
+++ b/tools/testing/selftests/cachestat/Makefile
@@ -3,6 +3,6 @@ TEST_GEN_PROGS := test_cachestat
 
 CFLAGS += $(KHDR_INCLUDES)
 CFLAGS += -Wall
-CFLAGS += -lrt
+LDLIBS += -lrt
 
 include ../lib.mk
-- 
2.25.1

Re: [PATCH 1/3] selftests: cachestat: properly link in librt

[Shuah Khan]

On 8/15/23 09:56, Andre Przywara wrote:
> Libraries should be listed last on the compiler's command line, so that
> the linker can look for and find still unresolved symbols. The librt
> library, required for the shm_* functions, was announced using CFLAGS,
> which puts the library *before* the source files, and fails compilation
> on my system:
> ======================
> gcc -isystem /src/linux-selftests/usr/include -Wall -lrt test_cachestat.c
>    -o /src/linux-selftests/kselftest/cachestat/test_cachestat
> /usr/bin/ld: /tmp/cceQWO3u.o: in function `test_cachestat_shmem':
> test_cachestat.c:(.text+0x890): undefined reference to `shm_open'
> /usr/bin/ld: test_cachestat.c:(.text+0x99c): undefined reference to `shm_unlink'
> collect2: error: ld returned 1 exit status
> make[4]: *** [../lib.mk:181: /src/linux-selftests/kselftest/cachestat/test_cachestat] Error 1
> ======================
> 
> Announce the library using the LDLIBS variable, which ensures the proper
> ordering on the command line.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   tools/testing/selftests/cachestat/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/cachestat/Makefile b/tools/testing/selftests/cachestat/Makefile
> index fca73aaa7d141..778b54ebb0364 100644
> --- a/tools/testing/selftests/cachestat/Makefile
> +++ b/tools/testing/selftests/cachestat/Makefile
> @@ -3,6 +3,6 @@ TEST_GEN_PROGS := test_cachestat
>   
>   CFLAGS += $(KHDR_INCLUDES)
>   CFLAGS += -Wall
> -CFLAGS += -lrt
> +LDLIBS += -lrt
>   
>   include ../lib.mk

Thank you. Applied to linux-kselftest next for Linux 6.6-rc1

thanks,
-- Shuah

[PATCH 2/3] selftests: cachestat: use proper syscall number macro

[Andre Przywara]

At the moment the cachestat syscall number is hard coded into the test
source code.
Remove that and replace it with the proper __NR_cachestat macro.
That ensures compatibility should other architectures pick a different
number.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/testing/selftests/cachestat/test_cachestat.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
index 54d09b820ed4b..a5a4ac8dcb76c 100644
--- a/tools/testing/selftests/cachestat/test_cachestat.c
+++ b/tools/testing/selftests/cachestat/test_cachestat.c
@@ -19,7 +19,6 @@ static const char * const dev_files[] = {
 	"/dev/zero", "/dev/null", "/dev/urandom",
 	"/proc/version", "/proc"
 };
-static const int cachestat_nr = 451;
 
 void print_cachestat(struct cachestat *cs)
 {
@@ -126,7 +125,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
 		}
 	}
 
-	syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
+	syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
 
 	ksft_print_msg("Cachestat call returned %ld\n", syscall_ret);
 
@@ -152,7 +151,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
 			ksft_print_msg("fsync fails.\n");
 			ret = false;
 		} else {
-			syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
+			syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
 
 			ksft_print_msg("Cachestat call (after fsync) returned %ld\n",
 				syscall_ret);
@@ -213,7 +212,7 @@ bool test_cachestat_shmem(void)
 		goto close_fd;
 	}
 
-	syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
+	syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
 
 	if (syscall_ret) {
 		ksft_print_msg("Cachestat returned non-zero.\n");
-- 
2.25.1

Re: [PATCH 2/3] selftests: cachestat: use proper syscall number macro

[Nhat Pham]

On Tue, Aug 15, 2023 at 8:56 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> At the moment the cachestat syscall number is hard coded into the test
> source code.
> Remove that and replace it with the proper __NR_cachestat macro.
> That ensures compatibility should other architectures pick a different
> number.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  tools/testing/selftests/cachestat/test_cachestat.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
> index 54d09b820ed4b..a5a4ac8dcb76c 100644
> --- a/tools/testing/selftests/cachestat/test_cachestat.c
> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> @@ -19,7 +19,6 @@ static const char * const dev_files[] = {
>         "/dev/zero", "/dev/null", "/dev/urandom",
>         "/proc/version", "/proc"
>  };
> -static const int cachestat_nr = 451;
>
>  void print_cachestat(struct cachestat *cs)
>  {
> @@ -126,7 +125,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
>                 }
>         }
>
> -       syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
> +       syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>
>         ksft_print_msg("Cachestat call returned %ld\n", syscall_ret);
>
> @@ -152,7 +151,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
>                         ksft_print_msg("fsync fails.\n");
>                         ret = false;
>                 } else {
> -                       syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
> +                       syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>
>                         ksft_print_msg("Cachestat call (after fsync) returned %ld\n",
>                                 syscall_ret);
> @@ -213,7 +212,7 @@ bool test_cachestat_shmem(void)
>                 goto close_fd;
>         }
>
> -       syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
> +       syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>
>         if (syscall_ret) {
>                 ksft_print_msg("Cachestat returned non-zero.\n");
> --
> 2.25.1
>

Oops something I forgot to fix. Thanks, Andre!
Reviewed-by: Nhat Pham <nphamcs@gmail.com>

Re: [PATCH 2/3] selftests: cachestat: use proper syscall number macro

[Shuah Khan]

On 8/15/23 09:56, Andre Przywara wrote:
> At the moment the cachestat syscall number is hard coded into the test
> source code.
> Remove that and replace it with the proper __NR_cachestat macro.
> That ensures compatibility should other architectures pick a different
> number.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   tools/testing/selftests/cachestat/test_cachestat.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
> index 54d09b820ed4b..a5a4ac8dcb76c 100644
> --- a/tools/testing/selftests/cachestat/test_cachestat.c
> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> @@ -19,7 +19,6 @@ static const char * const dev_files[] = {
>   	"/dev/zero", "/dev/null", "/dev/urandom",
>   	"/proc/version", "/proc"
>   };
> -static const int cachestat_nr = 451;
>   
>   void print_cachestat(struct cachestat *cs)
>   {
> @@ -126,7 +125,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
>   		}
>   	}
>   
> -	syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
> +	syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>   
>   	ksft_print_msg("Cachestat call returned %ld\n", syscall_ret);
>   
> @@ -152,7 +151,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
>   			ksft_print_msg("fsync fails.\n");
>   			ret = false;
>   		} else {
> -			syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
> +			syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>   
>   			ksft_print_msg("Cachestat call (after fsync) returned %ld\n",
>   				syscall_ret);
> @@ -213,7 +212,7 @@ bool test_cachestat_shmem(void)
>   		goto close_fd;
>   	}
>   
> -	syscall_ret = syscall(cachestat_nr, fd, &cs_range, &cs, 0);
> +	syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>   
>   	if (syscall_ret) {
>   		ksft_print_msg("Cachestat returned non-zero.\n");


Thank you. Applied to linux-kselftest next for Linux 6.6-rc1

thanks,
-- Shuah

[PATCH 3/3] selftests: cachestat: test for cachestat availability

[Andre Przywara]

As cachestat is a new syscall, it won't be available on older kernels,
for instance those running on a build machine. In this case, a run
reports all tests as "not ok" at the moment.

Test for the cachestat syscall availability first, before doing further
tests, and bail out early with a TAP SKIP comment.

This also uses the opportunity to add the proper TAP headers, and add
one check for the syscall error handling (illegal file descriptor).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../selftests/cachestat/test_cachestat.c      | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
index a5a4ac8dcb76c..77620e7ecf562 100644
--- a/tools/testing/selftests/cachestat/test_cachestat.c
+++ b/tools/testing/selftests/cachestat/test_cachestat.c
@@ -15,6 +15,8 @@
 
 #include "../kselftest.h"
 
+#define NR_TESTS	8
+
 static const char * const dev_files[] = {
 	"/dev/zero", "/dev/null", "/dev/urandom",
 	"/proc/version", "/proc"
@@ -235,7 +237,25 @@ bool test_cachestat_shmem(void)
 
 int main(void)
 {
-	int ret = 0;
+	int ret;
+
+	ksft_print_header();
+
+	ret = syscall(__NR_cachestat, -1, NULL, NULL, 0);
+	if (ret == -1 && errno == ENOSYS) {
+		printf("1..0 # Skipped: cachestat syscall not available\n");
+		return KSFT_SKIP;
+	}
+
+	ksft_set_plan(NR_TESTS);
+
+	if (ret == -1 && errno == EBADF) {
+		ksft_test_result_pass("bad file descriptor recognized\n");
+		ret = 0;
+	} else {
+		ksft_test_result_fail("bad file descriptor ignored\n");
+		ret = 1;
+	}
 
 	for (int i = 0; i < 5; i++) {
 		const char *dev_filename = dev_files[i];
-- 
2.25.1

Re: [PATCH 3/3] selftests: cachestat: test for cachestat availability

[Nhat Pham]

On Tue, Aug 15, 2023 at 8:56 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> As cachestat is a new syscall, it won't be available on older kernels,
> for instance those running on a build machine. In this case, a run
> reports all tests as "not ok" at the moment.
Interesting - I was under the assumption that if you backported the
selftests for cachestat, you would also backport the syscall's implementation
and wiring.

But yeah, I guess if you build with !CONFIG_CACHESTAT_SYSCALL,
these tests would fail.
>
> Test for the cachestat syscall availability first, before doing further
> tests, and bail out early with a TAP SKIP comment.
>
> This also uses the opportunity to add the proper TAP headers, and add
> one check for the syscall error handling (illegal file descriptor).
Thanks for the addition!
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../selftests/cachestat/test_cachestat.c      | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
> index a5a4ac8dcb76c..77620e7ecf562 100644
> --- a/tools/testing/selftests/cachestat/test_cachestat.c
> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> @@ -15,6 +15,8 @@
>
>  #include "../kselftest.h"
>
> +#define NR_TESTS       8
> +
>  static const char * const dev_files[] = {
>         "/dev/zero", "/dev/null", "/dev/urandom",
>         "/proc/version", "/proc"
> @@ -235,7 +237,25 @@ bool test_cachestat_shmem(void)
>
>  int main(void)
>  {
> -       int ret = 0;
> +       int ret;
> +
> +       ksft_print_header();
> +
> +       ret = syscall(__NR_cachestat, -1, NULL, NULL, 0);
> +       if (ret == -1 && errno == ENOSYS) {
nit: if (ret && errno == ENOSYS) sounds cleaner, but up to you.
> +               printf("1..0 # Skipped: cachestat syscall not available\n");
nit: perhaps ksft_print_msg()?
> +               return KSFT_SKIP;
> +       }
> +
> +       ksft_set_plan(NR_TESTS);
> +
> +       if (ret == -1 && errno == EBADF) {
> +               ksft_test_result_pass("bad file descriptor recognized\n");
> +               ret = 0;
> +       } else {
> +               ksft_test_result_fail("bad file descriptor ignored\n");
> +               ret = 1;
> +       }
Nice!
>
>         for (int i = 0; i < 5; i++) {
>                 const char *dev_filename = dev_files[i];
> --
> 2.25.1
>
Nitpicking aside:
Acked-by: Nhat Pham <nphamcs@gmail.com>

Re: [PATCH 3/3] selftests: cachestat: test for cachestat availability

[Andre Przywara]

On Tue, 15 Aug 2023 16:25:54 -0700
Nhat Pham <nphamcs@gmail.com> wrote:

Hi Nhat,

many thanks for having a look!

> On Tue, Aug 15, 2023 at 8:56 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > As cachestat is a new syscall, it won't be available on older kernels,
> > for instance those running on a build machine. In this case, a run
> > reports all tests as "not ok" at the moment.  
> Interesting - I was under the assumption that if you backported the
> selftests for cachestat, you would also backport the syscall's implementation
> and wiring.

Well, just running the tests on the kernel you just built is only one
use case. I build the tests from latest git HEAD, then copy them to a
target system with some kernel running. Or I just build the tests and run
them for regression testing on my build system with a distro kernel, which
is Ubuntu's 5.15 flavour, in my case.
The documentation explicitly mentions that selftests should work on older
kernels (copying the normal userland compatibility requirements), check
the second paragraph of Documentation/dev-tools/kselftest.rst.

> But yeah, I guess if you build with !CONFIG_CACHESTAT_SYSCALL,
> these tests would fail.
> >
> > Test for the cachestat syscall availability first, before doing further
> > tests, and bail out early with a TAP SKIP comment.
> >
> > This also uses the opportunity to add the proper TAP headers, and add
> > one check for the syscall error handling (illegal file descriptor).  
> Thanks for the addition!
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  .../selftests/cachestat/test_cachestat.c      | 22 ++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
> > index a5a4ac8dcb76c..77620e7ecf562 100644
> > --- a/tools/testing/selftests/cachestat/test_cachestat.c
> > +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> > @@ -15,6 +15,8 @@
> >
> >  #include "../kselftest.h"
> >
> > +#define NR_TESTS       8
> > +
> >  static const char * const dev_files[] = {
> >         "/dev/zero", "/dev/null", "/dev/urandom",
> >         "/proc/version", "/proc"
> > @@ -235,7 +237,25 @@ bool test_cachestat_shmem(void)
> >
> >  int main(void)
> >  {
> > -       int ret = 0;
> > +       int ret;
> > +
> > +       ksft_print_header();
> > +
> > +       ret = syscall(__NR_cachestat, -1, NULL, NULL, 0);
> > +       if (ret == -1 && errno == ENOSYS) {  
> nit: if (ret && errno == ENOSYS) sounds cleaner, but up to you.

Do you ever return a positive value other than 0? I think technically
errno is only valid when the return value is -1, so in the error case,
which I wanted to test here explicitly.
Some syscall selftests (I checked landlock the other day) do very elaborate
testing of the error path, trying to carefully cover all corner cases:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/landlock/base_test.c#n24

So this test was inspired by that, but I didn't want to go that far here
;-)

> > +               printf("1..0 # Skipped: cachestat syscall not available\n");  
> nit: perhaps ksft_print_msg()?

Ah, yes, of course!

> > +               return KSFT_SKIP;
> > +       }
> > +
> > +       ksft_set_plan(NR_TESTS);
> > +
> > +       if (ret == -1 && errno == EBADF) {
> > +               ksft_test_result_pass("bad file descriptor recognized\n");
> > +               ret = 0;
> > +       } else {
> > +               ksft_test_result_fail("bad file descriptor ignored\n");
> > +               ret = 1;
> > +       }  
> Nice!
> >
> >         for (int i = 0; i < 5; i++) {
> >                 const char *dev_filename = dev_files[i];
> > --
> > 2.25.1
> >  
> Nitpicking aside:
> Acked-by: Nhat Pham <nphamcs@gmail.com>

Thanks, I will send a v2 later, using ksft_print_msg(). But first I will
try if I can detect a tmpfs instance without boiling the ocean.

Cheers,
Andre

Re: [PATCH 3/3] selftests: cachestat: test for cachestat availability

[Shuah Khan]

On 8/15/23 09:56, Andre Przywara wrote:
> As cachestat is a new syscall, it won't be available on older kernels,
> for instance those running on a build machine. In this case, a run
> reports all tests as "not ok" at the moment.
> 
> Test for the cachestat syscall availability first, before doing further
> tests, and bail out early with a TAP SKIP comment.
> 
> This also uses the opportunity to add the proper TAP headers, and add
> one check for the syscall error handling (illegal file descriptor).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   .../selftests/cachestat/test_cachestat.c      | 22 ++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
> index a5a4ac8dcb76c..77620e7ecf562 100644
> --- a/tools/testing/selftests/cachestat/test_cachestat.c
> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> @@ -15,6 +15,8 @@
>   
>   #include "../kselftest.h"
>   
> +#define NR_TESTS	8
> +
>   static const char * const dev_files[] = {
>   	"/dev/zero", "/dev/null", "/dev/urandom",
>   	"/proc/version", "/proc"
> @@ -235,7 +237,25 @@ bool test_cachestat_shmem(void)
>   
>   int main(void)
>   {
> -	int ret = 0;
> +	int ret;
> +
> +	ksft_print_header();
> +
> +	ret = syscall(__NR_cachestat, -1, NULL, NULL, 0);
> +	if (ret == -1 && errno == ENOSYS) {
> +		printf("1..0 # Skipped: cachestat syscall not available\n");
> +		return KSFT_SKIP;
What happens when other errors besides ENOSYS? The test shouldn't
continue.

> +	}
> +
> +	ksft_set_plan(NR_TESTS);
> +
> +	if (ret == -1 && errno == EBADF) {
> +		ksft_test_result_pass("bad file descriptor recognized\n");
> +		ret = 0;
> +	} else {
> +		ksft_test_result_fail("bad file descriptor ignored\n");
> +		ret = 1;
> +	}
>   
>   	for (int i = 0; i < 5; i++) {
>   		const char *dev_filename = dev_files[i];

thanks,
-- Shuah

Re: [PATCH 3/3] selftests: cachestat: test for cachestat availability

[Andre Przywara]

On Wed, 16 Aug 2023 11:11:49 -0600
Shuah Khan <skhan@linuxfoundation.org> wrote:

Hi,

> On 8/15/23 09:56, Andre Przywara wrote:
> > As cachestat is a new syscall, it won't be available on older kernels,
> > for instance those running on a build machine. In this case, a run
> > reports all tests as "not ok" at the moment.
> > 
> > Test for the cachestat syscall availability first, before doing further
> > tests, and bail out early with a TAP SKIP comment.
> > 
> > This also uses the opportunity to add the proper TAP headers, and add
> > one check for the syscall error handling (illegal file descriptor).
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >   .../selftests/cachestat/test_cachestat.c      | 22 ++++++++++++++++++-
> >   1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
> > index a5a4ac8dcb76c..77620e7ecf562 100644
> > --- a/tools/testing/selftests/cachestat/test_cachestat.c
> > +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> > @@ -15,6 +15,8 @@
> >   
> >   #include "../kselftest.h"
> >   
> > +#define NR_TESTS	8
> > +
> >   static const char * const dev_files[] = {
> >   	"/dev/zero", "/dev/null", "/dev/urandom",
> >   	"/proc/version", "/proc"
> > @@ -235,7 +237,25 @@ bool test_cachestat_shmem(void)
> >   
> >   int main(void)
> >   {
> > -	int ret = 0;
> > +	int ret;
> > +
> > +	ksft_print_header();
> > +
> > +	ret = syscall(__NR_cachestat, -1, NULL, NULL, 0);
> > +	if (ret == -1 && errno == ENOSYS) {
> > +		printf("1..0 # Skipped: cachestat syscall not available\n");
> > +		return KSFT_SKIP;  
> What happens when other errors besides ENOSYS? The test shouldn't
> continue.

-1 is an illegal file descriptor, and this is checked below (still using
the same ret and errno), but reported using the normal framework.
This check above is done early, before we even announce the plan, so that
we can skip *all* of the tests, since they don't make any sense when the
syscall is not available at all.

Does that make sense?

Cheers,
Andre

> 
> > +	}
> > +
> > +	ksft_set_plan(NR_TESTS);
> > +
> > +	if (ret == -1 && errno == EBADF) {
> > +		ksft_test_result_pass("bad file descriptor recognized\n");
> > +		ret = 0;
> > +	} else {
> > +		ksft_test_result_fail("bad file descriptor ignored\n");
> > +		ret = 1;
> > +	}
> >   
> >   	for (int i = 0; i < 5; i++) {
> >   		const char *dev_filename = dev_files[i];  
> 
> thanks,
> -- Shuah

Re: [PATCH 3/3] selftests: cachestat: test for cachestat availability

[Shuah Khan]

On 8/17/23 08:47, Andre Przywara wrote:
> On Wed, 16 Aug 2023 11:11:49 -0600
> Shuah Khan <skhan@linuxfoundation.org> wrote:
> 
> Hi,
> 
>> On 8/15/23 09:56, Andre Przywara wrote:
>>> As cachestat is a new syscall, it won't be available on older kernels,
>>> for instance those running on a build machine. In this case, a run
>>> reports all tests as "not ok" at the moment.
>>>
>>> Test for the cachestat syscall availability first, before doing further
>>> tests, and bail out early with a TAP SKIP comment.
>>>
>>> This also uses the opportunity to add the proper TAP headers, and add
>>> one check for the syscall error handling (illegal file descriptor).
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>    .../selftests/cachestat/test_cachestat.c      | 22 ++++++++++++++++++-
>>>    1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
>>> index a5a4ac8dcb76c..77620e7ecf562 100644
>>> --- a/tools/testing/selftests/cachestat/test_cachestat.c
>>> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
>>> @@ -15,6 +15,8 @@
>>>    
>>>    #include "../kselftest.h"
>>>    
>>> +#define NR_TESTS	8
>>> +
>>>    static const char * const dev_files[] = {
>>>    	"/dev/zero", "/dev/null", "/dev/urandom",
>>>    	"/proc/version", "/proc"
>>> @@ -235,7 +237,25 @@ bool test_cachestat_shmem(void)
>>>    
>>>    int main(void)
>>>    {
>>> -	int ret = 0;
>>> +	int ret;
>>> +
>>> +	ksft_print_header();
>>> +
>>> +	ret = syscall(__NR_cachestat, -1, NULL, NULL, 0);
>>> +	if (ret == -1 && errno == ENOSYS) {
>>> +		printf("1..0 # Skipped: cachestat syscall not available\n");
>>> +		return KSFT_SKIP;
>> What happens when other errors besides ENOSYS? The test shouldn't
>> continue.
> 
> -1 is an illegal file descriptor, and this is checked below (still using
> the same ret and errno), but reported using the normal framework.
> This check above is done early, before we even announce the plan, so that
> we can skip *all* of the tests, since they don't make any sense when the
> syscall is not available at all.
> 
> Does that make sense?
> 

Yup. I will apply this for Linux 6.6-rc1. You will get patchbot notification
shortly.

thanks,
-- Shuah

Re: [PATCH 0/3] selftests: cachestat: fix build and run on older kernels

[Nhat Pham]

On Tue, Aug 15, 2023 at 8:56 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> I ran all kernel selftests on some test machine, and stumbled upon
> cachestat failing (among others).
> Those patches fix the cachestat test compilation and run on older
> kernels.
>
> Also I found that the but-last test (on a normal file) fails when run on
> a tmpfs mounted directory, as it happens on an initramfs-only system, or
> when the current directory happens to be /dev/shm or /tmp:
> # Create/open tmpfilecachestat
> # Cachestat call returned 0
> # Using cachestat: Cached: 4, Dirty: 4, Writeback: 0, Evicted: 0, Recently Evicted: 0
> # Cachestat call (after fsync) returned 0
> # Using cachestat: Cached: 4, Dirty: 4, Writeback: 0, Evicted: 0, Recently Evicted: 0
> # Number of dirty should be zero after fsync.
> not ok 6 cachestat fails with normal file
>
> That same test binary succeeds on the same machine right afterwards if
> the current directory is changed to an ext4 filesystem.

Ah, if I recall correctly, these kinds of fs have no-op fsync, correct?
Something along the line of:
https://github.com/torvalds/linux/blob/91aa6c4/mm/shmem.c#L4111
The fsync logic would fail indeed. Thanks for pointing that out!

>
> I don't really know if this is expected, and whether we should try to
> figure out if the test file lives on a tmpfs filesystem, or whether the

This would be nice. I think there's a userspace method to check
this, right? There's a TMPFS_MAGIC here - not sure if relevant:

https://man7.org/linux/man-pages/man2/statfs.2.html

> test itself is not strict enough, and requires more "flushing"
> (drop_caches?) to cover tmpfs directories as well.
>
> Any ideas how to fix this would be appreciated.
>
> Cheers,
> Andre
>
> Andre Przywara (3):
>   selftests: cachestat: properly link in librt
>   selftests: cachestat: use proper syscall number macro
>   selftests: cachestat: test for cachestat availability
>
>  tools/testing/selftests/cachestat/Makefile    |  2 +-
>  .../selftests/cachestat/test_cachestat.c      | 29 +++++++++++++++----
>  2 files changed, 25 insertions(+), 6 deletions(-)
>
> --
> 2.25.1
>