[PATCH 0/2] timers test fix and duplicate defines cleanup

[PATCH 0/2] timers test fix and duplicate defines cleanup

[Shuah Khan]

The first patch in this two patch fixes warn_unused_result compile
time warning in posix_timers test.

The second patch removes local NSEC_PER_SEC and USEC_PER_SEC defines.
NSEC_PER_SEC and USEC_PER_SEC are defines in several timers tests.
These defines are inconsistent with variations of ULL, LL, etc. without
any explanation why it is necessary.

These defines can be picked up from include/vdso/time64.h header
file. In the interest of making it easier to maintain, remove the
local defines. Include include/vdso/time64.h instead. This change
will also make the defines consistent.

Shuah Khan (2):
  selftests:timers: posix_timers: Fix warn_unused_result in
    __fatal_error()
  selftests: timers: Remove local NSEC_PER_SEC and USEC_PER_SEC defines

 tools/testing/selftests/timers/Makefile       |  2 +-
 tools/testing/selftests/timers/adjtick.c      |  4 +--
 .../selftests/timers/alarmtimer-suspend.c     |  2 +-
 .../selftests/timers/inconsistency-check.c    |  2 +-
 tools/testing/selftests/timers/leap-a-day.c   |  2 +-
 tools/testing/selftests/timers/mqueue-lat.c   |  2 +-
 tools/testing/selftests/timers/nanosleep.c    |  3 +--
 tools/testing/selftests/timers/posix_timers.c | 27 ++++++++++---------
 tools/testing/selftests/timers/raw_skew.c     |  2 +-
 tools/testing/selftests/timers/set-2038.c     |  3 +--
 .../testing/selftests/timers/set-timer-lat.c  |  3 +--
 .../testing/selftests/timers/valid-adjtimex.c |  4 +--
 12 files changed, 26 insertions(+), 30 deletions(-)

-- 
2.40.1

[PATCH 1/2] selftests:timers: posix_timers: Fix warn_unused_result in __fatal_error()

[Shuah Khan]

__fatal_error routine doesn't check strerror_r() return value,
which results in the following compile time warning:

posix_timers.c: In function ‘__fatal_error’:
posix_timers.c:31:9: warning: ignoring return value of ‘strerror_r’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
   31 |         strerror_r(errno, buf, sizeof(buf));

Fix this by adding a check for return value and error handling appropriate
for the GNU-specific strerror_r() in use in __fatal_error(). Check if
return string is null and handle accordingly.

From Linux strerror_r() manual page:

"The GNU-specific strerror_r() returns a pointer to a string containing
the error message. This may be either a pointer to a string that the
function stores in buf, or a pointer to some (immutable) static string
(in which case buf is unused). If the function stores a string in buf,
then at most buflen bytes are stored (the string may be truncated if
buflen is too small and errnum is unknown). The string always includes
a terminating null byte."

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 tools/testing/selftests/timers/posix_timers.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c
index 16bd49492efa..ddb1cebc844e 100644
--- a/tools/testing/selftests/timers/posix_timers.c
+++ b/tools/testing/selftests/timers/posix_timers.c
@@ -26,13 +26,17 @@
 static void __fatal_error(const char *test, const char *name, const char *what)
 {
 	char buf[64];
+	char *ret_str = NULL;
 
-	strerror_r(errno, buf, sizeof(buf));
+	ret_str = strerror_r(errno, buf, sizeof(buf));
 
-	if (name && strlen(name))
-		ksft_exit_fail_msg("%s %s %s %s\n", test, name, what, buf);
+	if (name && strlen(name) && ret_str)
+		ksft_exit_fail_msg("%s %s %s %s\n", test, name, what, ret_str);
+	else if (ret_str)
+		ksft_exit_fail_msg("%s %s %s\n", test, what, ret_str);
 	else
-		ksft_exit_fail_msg("%s %s %s\n", test, what, buf);
+		ksft_exit_fail_msg("%s %s\n", test, what);
+
 }
 
 #define fatal_error(name, what)	__fatal_error(__func__, name, what)
-- 
2.40.1

Re: [PATCH 1/2] selftests:timers: posix_timers: Fix warn_unused_result in __fatal_error()

[Shuah Khan]

On 9/24/24 09:56, Shuah Khan wrote:
> __fatal_error routine doesn't check strerror_r() return value,
> which results in the following compile time warning:
> 
> posix_timers.c: In function ‘__fatal_error’:
> posix_timers.c:31:9: warning: ignoring return value of ‘strerror_r’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
>     31 |         strerror_r(errno, buf, sizeof(buf));
> 
> Fix this by adding a check for return value and error handling appropriate
> for the GNU-specific strerror_r() in use in __fatal_error(). Check if
> return string is null and handle accordingly.
> 
>  From Linux strerror_r() manual page:
> 
> "The GNU-specific strerror_r() returns a pointer to a string containing
> the error message. This may be either a pointer to a string that the
> function stores in buf, or a pointer to some (immutable) static string
> (in which case buf is unused). If the function stores a string in buf,
> then at most buflen bytes are stored (the string may be truncated if
> buflen is too small and errnum is unknown). The string always includes
> a terminating null byte."
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>   tools/testing/selftests/timers/posix_timers.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c
> index 16bd49492efa..ddb1cebc844e 100644
> --- a/tools/testing/selftests/timers/posix_timers.c
> +++ b/tools/testing/selftests/timers/posix_timers.c
> @@ -26,13 +26,17 @@
>   static void __fatal_error(const char *test, const char *name, const char *what)
>   {
>   	char buf[64];
> +	char *ret_str = NULL;
>   
> -	strerror_r(errno, buf, sizeof(buf));
> +	ret_str = strerror_r(errno, buf, sizeof(buf));
>   
> -	if (name && strlen(name))
> -		ksft_exit_fail_msg("%s %s %s %s\n", test, name, what, buf);
> +	if (name && strlen(name) && ret_str)
> +		ksft_exit_fail_msg("%s %s %s %s\n", test, name, what, ret_str);
> +	else if (ret_str)
> +		ksft_exit_fail_msg("%s %s %s\n", test, what, ret_str);
>   	else
> -		ksft_exit_fail_msg("%s %s %s\n", test, what, buf);
> +		ksft_exit_fail_msg("%s %s\n", test, what);
> +
>   }
>   
>   #define fatal_error(name, what)	__fatal_error(__func__, name, what)

Any comments on this patch John?

thanks,
-- Shuah

Re: [PATCH 1/2] selftests:timers: posix_timers: Fix warn_unused_result in __fatal_error()

[John Stultz]

On Tue, Sep 24, 2024 at 8:57 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> __fatal_error routine doesn't check strerror_r() return value,
> which results in the following compile time warning:
>
> posix_timers.c: In function ‘__fatal_error’:
> posix_timers.c:31:9: warning: ignoring return value of ‘strerror_r’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
>    31 |         strerror_r(errno, buf, sizeof(buf));
>
> Fix this by adding a check for return value and error handling appropriate
> for the GNU-specific strerror_r() in use in __fatal_error(). Check if
> return string is null and handle accordingly.
>
> From Linux strerror_r() manual page:
>
> "The GNU-specific strerror_r() returns a pointer to a string containing
> the error message. This may be either a pointer to a string that the
> function stores in buf, or a pointer to some (immutable) static string
> (in which case buf is unused). If the function stores a string in buf,
> then at most buflen bytes are stored (the string may be truncated if
> buflen is too small and errnum is unknown). The string always includes
> a terminating null byte."
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  tools/testing/selftests/timers/posix_timers.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c
> index 16bd49492efa..ddb1cebc844e 100644
> --- a/tools/testing/selftests/timers/posix_timers.c
> +++ b/tools/testing/selftests/timers/posix_timers.c
> @@ -26,13 +26,17 @@
>  static void __fatal_error(const char *test, const char *name, const char *what)
>  {
>         char buf[64];
> +       char *ret_str = NULL;
>
> -       strerror_r(errno, buf, sizeof(buf));
> +       ret_str = strerror_r(errno, buf, sizeof(buf));
>
> -       if (name && strlen(name))
> -               ksft_exit_fail_msg("%s %s %s %s\n", test, name, what, buf);
> +       if (name && strlen(name) && ret_str)
> +               ksft_exit_fail_msg("%s %s %s %s\n", test, name, what, ret_str);
> +       else if (ret_str)
> +               ksft_exit_fail_msg("%s %s %s\n", test, what, ret_str);
>         else
> -               ksft_exit_fail_msg("%s %s %s\n", test, what, buf);
> +               ksft_exit_fail_msg("%s %s\n", test, what);
> +
>  }

Acked-by: John Stultz <jstultz@google.com>

[PATCH 2/2] selftests: timers: Remove local NSEC_PER_SEC and USEC_PER_SEC defines

[Shuah Khan]

Remove local NSEC_PER_SEC and USEC_PER_SEC defines. Pick them up from
include/vdso/time64.h. This requires -I $(top_srcdir) to the timers
Makefile to include the include/vdso/time64.h.

posix_timers test names the defines NSECS_PER_SEC and USECS_PER_SEC.
Include the include/vdso/time64.h and change NSECS_PER_SEC and
USECS_PER_SEC references to NSEC_PER_SEC and USEC_PER_SEC respectively.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 tools/testing/selftests/timers/Makefile           |  2 +-
 tools/testing/selftests/timers/adjtick.c          |  4 +---
 .../testing/selftests/timers/alarmtimer-suspend.c |  2 +-
 .../selftests/timers/inconsistency-check.c        |  2 +-
 tools/testing/selftests/timers/leap-a-day.c       |  2 +-
 tools/testing/selftests/timers/mqueue-lat.c       |  2 +-
 tools/testing/selftests/timers/nanosleep.c        |  3 +--
 tools/testing/selftests/timers/posix_timers.c     | 15 +++++++--------
 tools/testing/selftests/timers/raw_skew.c         |  2 +-
 tools/testing/selftests/timers/set-2038.c         |  3 +--
 tools/testing/selftests/timers/set-timer-lat.c    |  3 +--
 tools/testing/selftests/timers/valid-adjtimex.c   |  4 +---
 12 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 0e73a16874c4..32203593c62e 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-CFLAGS += -O3 -Wl,-no-as-needed -Wall
+CFLAGS += -O3 -Wl,-no-as-needed -Wall -I $(top_srcdir)
 LDLIBS += -lrt -lpthread -lm
 
 # these are all "safe" tests that don't modify
diff --git a/tools/testing/selftests/timers/adjtick.c b/tools/testing/selftests/timers/adjtick.c
index 205b76a4abb4..cb9a30f54662 100644
--- a/tools/testing/selftests/timers/adjtick.c
+++ b/tools/testing/selftests/timers/adjtick.c
@@ -22,14 +22,12 @@
 #include <sys/time.h>
 #include <sys/timex.h>
 #include <time.h>
+#include <include/vdso/time64.h>
 
 #include "../kselftest.h"
 
 #define CLOCK_MONOTONIC_RAW	4
 
-#define NSEC_PER_SEC		1000000000LL
-#define USEC_PER_SEC		1000000
-
 #define MILLION			1000000
 
 long systick;
diff --git a/tools/testing/selftests/timers/alarmtimer-suspend.c b/tools/testing/selftests/timers/alarmtimer-suspend.c
index ad52e608b88e..62da2a3f949e 100644
--- a/tools/testing/selftests/timers/alarmtimer-suspend.c
+++ b/tools/testing/selftests/timers/alarmtimer-suspend.c
@@ -28,6 +28,7 @@
 #include <signal.h>
 #include <stdlib.h>
 #include <pthread.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
 #define CLOCK_REALTIME			0
@@ -45,7 +46,6 @@
 #define NR_CLOCKIDS			12
 
 
-#define NSEC_PER_SEC 1000000000ULL
 #define UNREASONABLE_LAT (NSEC_PER_SEC * 5) /* hopefully we resume in 5 secs */
 
 #define SUSPEND_SECS 15
diff --git a/tools/testing/selftests/timers/inconsistency-check.c b/tools/testing/selftests/timers/inconsistency-check.c
index 36a49fba6c9b..75650cf0503f 100644
--- a/tools/testing/selftests/timers/inconsistency-check.c
+++ b/tools/testing/selftests/timers/inconsistency-check.c
@@ -28,10 +28,10 @@
 #include <sys/timex.h>
 #include <string.h>
 #include <signal.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
 #define CALLS_PER_LOOP 64
-#define NSEC_PER_SEC 1000000000ULL
 
 #define CLOCK_REALTIME			0
 #define CLOCK_MONOTONIC			1
diff --git a/tools/testing/selftests/timers/leap-a-day.c b/tools/testing/selftests/timers/leap-a-day.c
index 986abbdb1521..04004a7c0934 100644
--- a/tools/testing/selftests/timers/leap-a-day.c
+++ b/tools/testing/selftests/timers/leap-a-day.c
@@ -48,9 +48,9 @@
 #include <string.h>
 #include <signal.h>
 #include <unistd.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
-#define NSEC_PER_SEC 1000000000ULL
 #define CLOCK_TAI 11
 
 time_t next_leap;
diff --git a/tools/testing/selftests/timers/mqueue-lat.c b/tools/testing/selftests/timers/mqueue-lat.c
index f3179a605bba..63de2334a291 100644
--- a/tools/testing/selftests/timers/mqueue-lat.c
+++ b/tools/testing/selftests/timers/mqueue-lat.c
@@ -29,9 +29,9 @@
 #include <signal.h>
 #include <errno.h>
 #include <mqueue.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
-#define NSEC_PER_SEC 1000000000ULL
 
 #define TARGET_TIMEOUT		100000000	/* 100ms in nanoseconds */
 #define UNRESONABLE_LATENCY	40000000	/* 40ms in nanosecs */
diff --git a/tools/testing/selftests/timers/nanosleep.c b/tools/testing/selftests/timers/nanosleep.c
index df1d03516e7b..9a354e38a569 100644
--- a/tools/testing/selftests/timers/nanosleep.c
+++ b/tools/testing/selftests/timers/nanosleep.c
@@ -27,10 +27,9 @@
 #include <sys/timex.h>
 #include <string.h>
 #include <signal.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
-#define NSEC_PER_SEC 1000000000ULL
-
 #define CLOCK_REALTIME			0
 #define CLOCK_MONOTONIC			1
 #define CLOCK_PROCESS_CPUTIME_ID	2
diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c
index ddb1cebc844e..9814b3a1c77d 100644
--- a/tools/testing/selftests/timers/posix_timers.c
+++ b/tools/testing/selftests/timers/posix_timers.c
@@ -15,13 +15,12 @@
 #include <string.h>
 #include <unistd.h>
 #include <time.h>
+#include <include/vdso/time64.h>
 #include <pthread.h>
 
 #include "../kselftest.h"
 
 #define DELAY 2
-#define USECS_PER_SEC 1000000
-#define NSECS_PER_SEC 1000000000
 
 static void __fatal_error(const char *test, const char *name, const char *what)
 {
@@ -86,9 +85,9 @@ static int check_diff(struct timeval start, struct timeval end)
 	long long diff;
 
 	diff = end.tv_usec - start.tv_usec;
-	diff += (end.tv_sec - start.tv_sec) * USECS_PER_SEC;
+	diff += (end.tv_sec - start.tv_sec) * USEC_PER_SEC;
 
-	if (llabs(diff - DELAY * USECS_PER_SEC) > USECS_PER_SEC / 2) {
+	if (llabs(diff - DELAY * USEC_PER_SEC) > USEC_PER_SEC / 2) {
 		printf("Diff too high: %lld..", diff);
 		return -1;
 	}
@@ -448,7 +447,7 @@ static inline int64_t calcdiff_ns(struct timespec t1, struct timespec t2)
 {
 	int64_t diff;
 
-	diff = NSECS_PER_SEC * (int64_t)((int) t1.tv_sec - (int) t2.tv_sec);
+	diff = NSEC_PER_SEC * (int64_t)((int) t1.tv_sec - (int) t2.tv_sec);
 	diff += ((int) t1.tv_nsec - (int) t2.tv_nsec);
 	return diff;
 }
@@ -479,7 +478,7 @@ static void check_sigev_none(int which, const char *name)
 	do {
 		if (clock_gettime(which, &now))
 			fatal_error(name, "clock_gettime()");
-	} while (calcdiff_ns(now, start) < NSECS_PER_SEC);
+	} while (calcdiff_ns(now, start) < NSEC_PER_SEC);
 
 	if (timer_gettime(timerid, &its))
 		fatal_error(name, "timer_gettime()");
@@ -536,7 +535,7 @@ static void check_gettime(int which, const char *name)
 			wraps++;
 		prev = its;
 
-	} while (calcdiff_ns(now, start) < NSECS_PER_SEC);
+	} while (calcdiff_ns(now, start) < NSEC_PER_SEC);
 
 	if (timer_delete(timerid))
 		fatal_error(name, "timer_delete()");
@@ -587,7 +586,7 @@ static void check_overrun(int which, const char *name)
 	do {
 		if (clock_gettime(which, &now))
 			fatal_error(name, "clock_gettime()");
-	} while (calcdiff_ns(now, start) < NSECS_PER_SEC);
+	} while (calcdiff_ns(now, start) < NSEC_PER_SEC);
 
 	/* Unblock it, which should deliver a signal */
 	if (sigprocmask(SIG_UNBLOCK, &set, NULL))
diff --git a/tools/testing/selftests/timers/raw_skew.c b/tools/testing/selftests/timers/raw_skew.c
index 030143eb09b4..ea50e4efc422 100644
--- a/tools/testing/selftests/timers/raw_skew.c
+++ b/tools/testing/selftests/timers/raw_skew.c
@@ -25,10 +25,10 @@
 #include <sys/time.h>
 #include <sys/timex.h>
 #include <time.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
 #define CLOCK_MONOTONIC_RAW		4
-#define NSEC_PER_SEC 1000000000LL
 
 #define shift_right(x, s) ({		\
 	__typeof__(x) __x = (x);	\
diff --git a/tools/testing/selftests/timers/set-2038.c b/tools/testing/selftests/timers/set-2038.c
index f7d978721b9e..ed244315e11c 100644
--- a/tools/testing/selftests/timers/set-2038.c
+++ b/tools/testing/selftests/timers/set-2038.c
@@ -27,10 +27,9 @@
 #include <unistd.h>
 #include <time.h>
 #include <sys/time.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
-#define NSEC_PER_SEC 1000000000LL
-
 #define KTIME_MAX	((long long)~((unsigned long long)1 << 63))
 #define KTIME_SEC_MAX	(KTIME_MAX / NSEC_PER_SEC)
 
diff --git a/tools/testing/selftests/timers/set-timer-lat.c b/tools/testing/selftests/timers/set-timer-lat.c
index 7ce240c89b21..5365e9ae61c3 100644
--- a/tools/testing/selftests/timers/set-timer-lat.c
+++ b/tools/testing/selftests/timers/set-timer-lat.c
@@ -28,6 +28,7 @@
 #include <signal.h>
 #include <stdlib.h>
 #include <pthread.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
 #define CLOCK_REALTIME			0
@@ -44,8 +45,6 @@
 #define CLOCK_TAI			11
 #define NR_CLOCKIDS			12
 
-
-#define NSEC_PER_SEC 1000000000ULL
 #define UNRESONABLE_LATENCY 40000000 /* 40ms in nanosecs */
 
 #define TIMER_SECS 1
diff --git a/tools/testing/selftests/timers/valid-adjtimex.c b/tools/testing/selftests/timers/valid-adjtimex.c
index d500884801d8..6b7801055ad1 100644
--- a/tools/testing/selftests/timers/valid-adjtimex.c
+++ b/tools/testing/selftests/timers/valid-adjtimex.c
@@ -29,11 +29,9 @@
 #include <string.h>
 #include <signal.h>
 #include <unistd.h>
+#include <include/vdso/time64.h>
 #include "../kselftest.h"
 
-#define NSEC_PER_SEC 1000000000LL
-#define USEC_PER_SEC 1000000LL
-
 #define ADJ_SETOFFSET 0x0100
 
 #include <sys/syscall.h>
-- 
2.40.1

Re: [PATCH 2/2] selftests: timers: Remove local NSEC_PER_SEC and USEC_PER_SEC defines

[John Stultz]

On Tue, Sep 24, 2024 at 8:57 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> Remove local NSEC_PER_SEC and USEC_PER_SEC defines. Pick them up from
> include/vdso/time64.h. This requires -I $(top_srcdir) to the timers
> Makefile to include the include/vdso/time64.h.
>
> posix_timers test names the defines NSECS_PER_SEC and USECS_PER_SEC.
> Include the include/vdso/time64.h and change NSECS_PER_SEC and
> USECS_PER_SEC references to NSEC_PER_SEC and USEC_PER_SEC respectively.

Nit: You got the last bit switched there. This patch changes local
NSEC_PER_SEC to the upstream defined NSECS_PER_SEC.

Overall no objection from me. I've always pushed to have the tests be
mostly self-contained so they can be built outside of the kernel
source, but at this point the current kselftest.h dependencies means
it already takes some work, so this isn't introducing an undue
hardship.

Other then the nit,
  Acked-by: John Stultz <jstultz@google.com>

> diff --git a/tools/testing/selftests/timers/adjtick.c b/tools/testing/selftests/timers/adjtick.c
> index 205b76a4abb4..cb9a30f54662 100644
> --- a/tools/testing/selftests/timers/adjtick.c
> +++ b/tools/testing/selftests/timers/adjtick.c
> @@ -22,14 +22,12 @@
>  #include <sys/time.h>
>  #include <sys/timex.h>
>  #include <time.h>
> +#include <include/vdso/time64.h>
>
>  #include "../kselftest.h"
>
>  #define CLOCK_MONOTONIC_RAW    4

I suspect CLOCK_MONOTONIC_RAW (and the other clockid definitions)
could be similarly removed here as well in a future patch?

thanks
-john

Re: [PATCH 2/2] selftests: timers: Remove local NSEC_PER_SEC and USEC_PER_SEC defines

[Shuah Khan]

On 9/24/24 17:59, John Stultz wrote:
> On Tue, Sep 24, 2024 at 8:57 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> Remove local NSEC_PER_SEC and USEC_PER_SEC defines. Pick them up from
>> include/vdso/time64.h. This requires -I $(top_srcdir) to the timers
>> Makefile to include the include/vdso/time64.h.
>>
>> posix_timers test names the defines NSECS_PER_SEC and USECS_PER_SEC.
>> Include the include/vdso/time64.h and change NSECS_PER_SEC and
>> USECS_PER_SEC references to NSEC_PER_SEC and USEC_PER_SEC respectively.
> 
> Nit: You got the last bit switched there. This patch changes local
> NSEC_PER_SEC to the upstream defined NSECS_PER_SEC.

I think what IO have is correct. posix_timers defines them as NSECS_PER_SEC
and USECS_PER_SEC and the header file doesn't have the extra S. It could
use rephrasing thought to make it clear.

Is it okay to fix this when I apply the patch or would you like me to send v2?

> 
> Overall no objection from me. I've always pushed to have the tests be
> mostly self-contained so they can be built outside of the kernel
> source, but at this point the current kselftest.h dependencies means
> it already takes some work, so this isn't introducing an undue
> hardship.
> 

Yes. At this point it would be hard to build it outside. DO you think
these defines can be part of uapi?
  
> Other then the nit,
>    Acked-by: John Stultz <jstultz@google.com>
> 
>> diff --git a/tools/testing/selftests/timers/adjtick.c b/tools/testing/selftests/timers/adjtick.c
>> index 205b76a4abb4..cb9a30f54662 100644
>> --- a/tools/testing/selftests/timers/adjtick.c
>> +++ b/tools/testing/selftests/timers/adjtick.c
>> @@ -22,14 +22,12 @@
>>   #include <sys/time.h>
>>   #include <sys/timex.h>
>>   #include <time.h>
>> +#include <include/vdso/time64.h>
>>
>>   #include "../kselftest.h"
>>
>>   #define CLOCK_MONOTONIC_RAW    4
> 
> I suspect CLOCK_MONOTONIC_RAW (and the other clockid definitions)
> could be similarly removed here as well in a future patch?

Yes. It could be cleaned up. I will send a patch in for this.

thanks,
-- Shuah

Re: [PATCH 2/2] selftests: timers: Remove local NSEC_PER_SEC and USEC_PER_SEC defines

[John Stultz]

On Wed, Sep 25, 2024 at 8:20 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 9/24/24 17:59, John Stultz wrote:
> > On Tue, Sep 24, 2024 at 8:57 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
> >>
> >> Remove local NSEC_PER_SEC and USEC_PER_SEC defines. Pick them up from
> >> include/vdso/time64.h. This requires -I $(top_srcdir) to the timers
> >> Makefile to include the include/vdso/time64.h.
> >>
> >> posix_timers test names the defines NSECS_PER_SEC and USECS_PER_SEC.
> >> Include the include/vdso/time64.h and change NSECS_PER_SEC and
> >> USECS_PER_SEC references to NSEC_PER_SEC and USEC_PER_SEC respectively.
> >
> > Nit: You got the last bit switched there. This patch changes local
> > NSEC_PER_SEC to the upstream defined NSECS_PER_SEC.
>
> I think what IO have is correct. posix_timers defines them as NSECS_PER_SEC
> and USECS_PER_SEC and the header file doesn't have the extra S. It could
> use rephrasing thought to make it clear.

?
But the patch is removing the local non-plural NSEC_PER_SEC usage in
posix_timers?

-#define NSEC_PER_SEC           1000000000LL
-#define USEC_PER_SEC           1000000
-

As the headers do have the values with the extra S.

So I'm confused (sort of my natural state :), but this is a minor nit,
so apologies and just ignore me if I'm really getting it backwards
here.


> Is it okay to fix this when I apply the patch or would you like me to send v2?
>

I don't need a v2

> >
> > Overall no objection from me. I've always pushed to have the tests be
> > mostly self-contained so they can be built outside of the kernel
> > source, but at this point the current kselftest.h dependencies means
> > it already takes some work, so this isn't introducing an undue
> > hardship.
> >
>
> Yes. At this point it would be hard to build it outside. DO you think
> these defines can be part of uapi?

Maybe, though they are so common I fret it would cause folks trouble
with redefinition collisions.

Thanks for doing this cleanup!
-john

Re: [PATCH 0/2] timers test fix and duplicate defines cleanup

[Thomas Gleixner]

On Tue, Sep 24 2024 at 09:56, Shuah Khan wrote:

> The first patch in this two patch fixes warn_unused_result compile
> time warning in posix_timers test.
>
> The second patch removes local NSEC_PER_SEC and USEC_PER_SEC defines.
> NSEC_PER_SEC and USEC_PER_SEC are defines in several timers tests.
> These defines are inconsistent with variations of ULL, LL, etc. without
> any explanation why it is necessary.
>
> These defines can be picked up from include/vdso/time64.h header
> file. In the interest of making it easier to maintain, remove the
> local defines. Include include/vdso/time64.h instead. This change
> will also make the defines consistent.

Acked-by: Thomas Gleixner <tglx@linutronix.de>