[LTP] [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[LTP] [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Li Wang]

This shmctl01 test detect the time as the number of seconds twice
(before and after) the shmget() instruction, then it verifies
whether the 'struct shmid_ds ds' gets data correctly. But here it
shows 'ds->ctime' out of the seconds range (1604298586, 1604298586),

The reason is that shmget()/msgsnd() always use ktime_get_real_second
to get real seconds, but time() on aarch64 via gettimeofday() or (it
depends on different kernel versions) clock_gettime() in use-level's
VDSO to return tv_sec.

time()
  __cvdso_gettimeofday
   ...
     do_gettimeofday
       ktime_get_real_ts64
        timespc64_add_ns

Situation can be simplify as difference between ktime_get_real_second
and ktime_get_real_ts64. As we can see ktime_get_real_second return
tk->xtime_sec directly, however

timespc64_add_ns more easily add 1 more second via "a->tv_sec +=..."
on a virtual machine, that's why we got occasional errors like:

shmctl01.c:183: TFAIL: SHM_STAT: shm_ctime=1604298585, expected <1604298586,1604298586>
...
msgsnd01.c:59: TFAIL: msg_stime = 1605730573 out of [1605730574, 1605730574]

Here we propose to use '__NR_time' to invoke syscall directly that makes
test all get real seconds via ktime_get_real_second.

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Chunyu Hu <chuhu@redhat.com>
Cc: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c | 5 +++--
 testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c | 5 +++--
 testcases/kernel/syscalls/ipc/shmctl/shmctl01.c | 5 +++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
index 5c1e317e9..6fdc47dc3 100644
--- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
+++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
@@ -10,6 +10,7 @@
 #include "tst_test.h"
 #include "tst_safe_sysv_ipc.h"
 #include "libnewipc.h"
+#include "tst_timer.h"
 
 static key_t msgkey;
 static int queue_id = -1, pid;
@@ -25,13 +26,13 @@ static void verify_msgrcv(void)
 
 	SAFE_MSGSND(queue_id, &snd_buf, MSGSIZE, 0);
 
-	time(&before_rcv);
+	tst_syscall(__NR_time, &before_rcv);
 	TEST(msgrcv(queue_id, &rcv_buf, MSGSIZE, 1, 0));
 	if (TST_RET == -1) {
 		tst_res(TFAIL | TTERRNO, "msgrcv failed");
 		return;
 	}
-	time(&after_rcv);
+	tst_syscall(__NR_time, &after_rcv);
 
 	if (strcmp(rcv_buf.mtext, snd_buf.mtext) == 0)
 		tst_res(TPASS, "message received(%s) = message sent(%s)",
diff --git a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
index 5f5da52d2..9101f2668 100644
--- a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
+++ b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
@@ -16,6 +16,7 @@
 #include "tst_test.h"
 #include "tst_safe_sysv_ipc.h"
 #include "libnewipc.h"
+#include "tst_timer.h"
 
 static key_t msgkey;
 static int queue_id = -1, pid;
@@ -29,13 +30,13 @@ static void verify_msgsnd(void)
 	struct msqid_ds qs_buf;
 	time_t before_snd, after_snd;
 
-	time(&before_snd);
+	tst_syscall(__NR_time, &before_snd);
 	TEST(msgsnd(queue_id, &snd_buf, MSGSIZE, 0));
 	if (TST_RET == -1) {
 		tst_res(TFAIL | TTERRNO, "msgsnd() failed");
 		return;
 	}
-	time(&after_snd);
+	tst_syscall(__NR_time, &after_snd);
 
 	SAFE_MSGCTL(queue_id, IPC_STAT, &qs_buf);
 
diff --git a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
index 3a39a4d74..f5b8eaef9 100644
--- a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
+++ b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
@@ -19,6 +19,7 @@
 #include "tst_test.h"
 #include "tst_safe_sysv_ipc.h"
 #include "libnewipc.h"
+#include "tst_timer.h"
 
 #define NCHILD 20
 
@@ -240,9 +241,9 @@ static int get_shm_idx_from_id(int shm_id)
 
 static void setup(void)
 {
-	ctime_min = time(NULL);
+	ctime_min = tst_syscall(__NR_time, NULL);
 	shm_id = SAFE_SHMGET(IPC_PRIVATE, SHM_SIZE, IPC_CREAT | SHM_RW);
-	ctime_max = time(NULL);
+	ctime_max = tst_syscall(__NR_time, NULL);
 
 	shm_idx = get_shm_idx_from_id(shm_id);
 
-- 
2.21.1

[LTP] [PATCH 2/2] syscalls: shift to time() if __NR_time not support

[Li Wang]

On some platforms(aarch64) __NR_time is not supported, if that happens,
go back to invoke time() and relax 1-second in low bound for comparing.

This also to fix:
  TFAIL: msg_stime = 1605730573 out of [1605730574, 1605730574]

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Chunyu Hu <chuhu@redhat.com>
Cc: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c | 14 ++++++++++++--
 testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c | 14 ++++++++++++--
 testcases/kernel/syscalls/ipc/shmctl/shmctl01.c | 14 ++++++++++++--
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
index 6fdc47dc3..9dc778ca7 100644
--- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
+++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
@@ -26,13 +26,23 @@ static void verify_msgrcv(void)
 
 	SAFE_MSGSND(queue_id, &snd_buf, MSGSIZE, 0);
 
-	tst_syscall(__NR_time, &before_rcv);
+	before_rcv = syscall(__NR_time, NULL);
+	if (before_rcv == -1 && errno == ENOSYS) {
+		tst_res(TINFO, "__NR_time not supported");
+		before_rcv = time(NULL) - 1;
+	}
+
 	TEST(msgrcv(queue_id, &rcv_buf, MSGSIZE, 1, 0));
 	if (TST_RET == -1) {
 		tst_res(TFAIL | TTERRNO, "msgrcv failed");
 		return;
 	}
-	tst_syscall(__NR_time, &after_rcv);
+
+	after_rcv = syscall(__NR_time, NULL);
+	if (after_rcv == -1 && errno == ENOSYS) {
+		tst_res(TINFO, "__NR_time not supported");
+		after_rcv = time(NULL);
+	}
 
 	if (strcmp(rcv_buf.mtext, snd_buf.mtext) == 0)
 		tst_res(TPASS, "message received(%s) = message sent(%s)",
diff --git a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
index 9101f2668..27464e79f 100644
--- a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
+++ b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
@@ -30,13 +30,23 @@ static void verify_msgsnd(void)
 	struct msqid_ds qs_buf;
 	time_t before_snd, after_snd;
 
-	tst_syscall(__NR_time, &before_snd);
+	before_snd = syscall(__NR_time, NULL);
+	if (before_snd == -1 && errno == ENOSYS) {
+		tst_res(TINFO, "__NR_time not supported");
+		before_snd = time(NULL) - 1;
+	}
+
 	TEST(msgsnd(queue_id, &snd_buf, MSGSIZE, 0));
 	if (TST_RET == -1) {
 		tst_res(TFAIL | TTERRNO, "msgsnd() failed");
 		return;
 	}
-	tst_syscall(__NR_time, &after_snd);
+
+	after_snd = syscall(__NR_time, NULL);
+	if (after_snd == -1 && errno == ENOSYS) {
+		tst_res(TINFO, "__NR_time not supported");
+		after_snd = time(NULL);
+	}
 
 	SAFE_MSGCTL(queue_id, IPC_STAT, &qs_buf);
 
diff --git a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
index f5b8eaef9..356513726 100644
--- a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
+++ b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
@@ -241,9 +241,19 @@ static int get_shm_idx_from_id(int shm_id)
 
 static void setup(void)
 {
-	ctime_min = tst_syscall(__NR_time, NULL);
+	ctime_min = syscall(__NR_time, NULL);
+	if (ctime_min == -1 && errno == ENOSYS) {
+		tst_res(TINFO, "__NR_time not supported");
+		ctime_min = time(NULL) - 1;
+	}
+
 	shm_id = SAFE_SHMGET(IPC_PRIVATE, SHM_SIZE, IPC_CREAT | SHM_RW);
-	ctime_max = tst_syscall(__NR_time, NULL);
+
+	ctime_max = syscall(__NR_time, NULL);
+	if (ctime_max == -1 && errno == ENOSYS) {
+		tst_res(TINFO, "__NR_time not supported");
+		ctime_max = time(NULL);
+	}
 
 	shm_idx = get_shm_idx_from_id(shm_id);
 
-- 
2.21.1

[LTP] [PATCH 2/2] syscalls: shift to time() if __NR_time not support

[Yang Xu]

Hi Li

I have seen this patchset, Can we use a function to check whether kernel 
supports time syscall (like time_supported_by_kernel()) in setup
and then we use time() - !return value in verify funtion?
> On some platforms(aarch64) __NR_time is not supported, if that happens,
> go back to invoke time() and relax 1-second in low bound for comparing.
>
> This also to fix:
>    TFAIL: msg_stime = 1605730573 out of [1605730574, 1605730574]
>
> Signed-off-by: Li Wang<liwang@redhat.com>
> Cc: Chunyu Hu<chuhu@redhat.com>
> Cc: Cyril Hrubis<chrubis@suse.cz>
> ---
>   testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c | 14 ++++++++++++--
>   testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c | 14 ++++++++++++--
>   testcases/kernel/syscalls/ipc/shmctl/shmctl01.c | 14 ++++++++++++--
>   3 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
> index 6fdc47dc3..9dc778ca7 100644
> --- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
> +++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
> @@ -26,13 +26,23 @@ static void verify_msgrcv(void)
>
>   	SAFE_MSGSND(queue_id,&snd_buf, MSGSIZE, 0);
>
> -	tst_syscall(__NR_time,&before_rcv);
> +	before_rcv = syscall(__NR_time, NULL);
> +	if (before_rcv == -1&&  errno == ENOSYS) {
> +		tst_res(TINFO, "__NR_time not supported");
> +		before_rcv = time(NULL) - 1;
> +	}
> +
>   	TEST(msgrcv(queue_id,&rcv_buf, MSGSIZE, 1, 0));
>   	if (TST_RET == -1) {
>   		tst_res(TFAIL | TTERRNO, "msgrcv failed");
>   		return;
>   	}
> -	tst_syscall(__NR_time,&after_rcv);
> +
> +	after_rcv = syscall(__NR_time, NULL);
> +	if (after_rcv == -1&&  errno == ENOSYS) {
> +		tst_res(TINFO, "__NR_time not supported");
> +		after_rcv = time(NULL);
> +	}
>
>   	if (strcmp(rcv_buf.mtext, snd_buf.mtext) == 0)
>   		tst_res(TPASS, "message received(%s) = message sent(%s)",
> diff --git a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
> index 9101f2668..27464e79f 100644
> --- a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
> +++ b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
> @@ -30,13 +30,23 @@ static void verify_msgsnd(void)
>   	struct msqid_ds qs_buf;
>   	time_t before_snd, after_snd;
>
> -	tst_syscall(__NR_time,&before_snd);
> +	before_snd = syscall(__NR_time, NULL);
> +	if (before_snd == -1&&  errno == ENOSYS) {
> +		tst_res(TINFO, "__NR_time not supported");
> +		before_snd = time(NULL) - 1;
> +	}
> +
>   	TEST(msgsnd(queue_id,&snd_buf, MSGSIZE, 0));
>   	if (TST_RET == -1) {
>   		tst_res(TFAIL | TTERRNO, "msgsnd() failed");
>   		return;
>   	}
> -	tst_syscall(__NR_time,&after_snd);
> +
> +	after_snd = syscall(__NR_time, NULL);
> +	if (after_snd == -1&&  errno == ENOSYS) {
> +		tst_res(TINFO, "__NR_time not supported");
> +		after_snd = time(NULL);
> +	}
>
>   	SAFE_MSGCTL(queue_id, IPC_STAT,&qs_buf);
>
> diff --git a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
> index f5b8eaef9..356513726 100644
> --- a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
> +++ b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
> @@ -241,9 +241,19 @@ static int get_shm_idx_from_id(int shm_id)
>
>   static void setup(void)
>   {
> -	ctime_min = tst_syscall(__NR_time, NULL);
> +	ctime_min = syscall(__NR_time, NULL);
> +	if (ctime_min == -1&&  errno == ENOSYS) {
> +		tst_res(TINFO, "__NR_time not supported");
> +		ctime_min = time(NULL) - 1;
> +	}
> +
>   	shm_id = SAFE_SHMGET(IPC_PRIVATE, SHM_SIZE, IPC_CREAT | SHM_RW);
> -	ctime_max = tst_syscall(__NR_time, NULL);
> +
> +	ctime_max = syscall(__NR_time, NULL);
> +	if (ctime_max == -1&&  errno == ENOSYS) {
> +		tst_res(TINFO, "__NR_time not supported");
> +		ctime_max = time(NULL);
> +	}
>
>   	shm_idx = get_shm_idx_from_id(shm_id);
>

[LTP] [PATCH 2/2] syscalls: shift to time() if __NR_time not support

[Li Wang]

Hi Xu,

Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote:


> I have seen this patchset, Can we use a function to check whether kernel
>

Yes, we can, I was even thinking to define a global MACRO can check
any syscall not only __NR_time.
(maybe we can achieve it for other tests)
But for this kind of case, I'd not suggest using that MACRO/function
to check __NR_time, because the test will perform twice at the moment
for the __NR_time syscall if it supporting(first time for support checking,
second time for real invoking).

Considering this is a time comparing test, that makes our seconds
more inaccurate to compare.

supports time syscall (like time_supported_by_kernel()) in setup
> and then we use time() - !return value in verify funtion?
>

Though we check the syscall in setup(), shouldn't we also export a variable
to record the result we checked? That does not make things be simple too.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201124/f2bb2de4/attachment.htm>

[LTP] [PATCH 2/2] syscalls: shift to time() if __NR_time not support

[Yang Xu]

Hi Li
> Hi Xu,
>
> Yang Xu <xuyang2018.jy@cn.fujitsu.com
> <mailto:xuyang2018.jy@cn.fujitsu.com>> wrote:
>
>     I have seen this patchset, Can we use a function to check whether
>     kernel
>
>
> Yes, we can, I was even thinking to define a global MACRO can check
> any syscall not only __NR_time.
> (maybe we can achieve it for other tests)
> But for this kind of case, I'd not suggest using that MACRO/function
> to check __NR_time, because the test will perform twice at the moment
> for the __NR_time syscall if it supporting(first time for support checking,
> second time for real invoking).
>
> Considering this is a time comparing test, that makes our seconds
> more inaccurate to compare.
>
>     supports time syscall (like time_supported_by_kernel()) in setup
>     and then we use time() - !return value in verify funtion?
>
>
> Though we check the syscall in setup(), shouldn't we also export a variable
> to record the result we checked? That does not make things be simple too.
Yes. On some platform(aarch64), it is simple because it doesn't need to 
call tst_syscall two times and  time() two times and report non-support 
info many times especailly when using -i parameters.

IMO, it is a taste perference(I usually detect kernel whether support in 
setup). Your patchset is also ok. So

Acked-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>

>
> --
> Regards,
> Li Wang

[LTP] [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Cyril Hrubis]

Hi!
[CC Thomas Gleixner]

Thomas can you please have a look? It looks like we can get the SysV IPC
ctime to be one second off compared to what we get from realtime clock.

Do we care to get this fixed in kernel or should we fix the tests?

> This shmctl01 test detect the time as the number of seconds twice
> (before and after) the shmget() instruction, then it verifies
> whether the 'struct shmid_ds ds' gets data correctly. But here it
> shows 'ds->ctime' out of the seconds range (1604298586, 1604298586),
> 
> The reason is that shmget()/msgsnd() always use ktime_get_real_second
> to get real seconds, but time() on aarch64 via gettimeofday() or (it
> depends on different kernel versions) clock_gettime() in use-level's
> VDSO to return tv_sec.
> 
> time()
>   __cvdso_gettimeofday
>    ...
>      do_gettimeofday
>        ktime_get_real_ts64
>         timespc64_add_ns
> 
> Situation can be simplify as difference between ktime_get_real_second
> and ktime_get_real_ts64. As we can see ktime_get_real_second return
> tk->xtime_sec directly, however
> 
> timespc64_add_ns more easily add 1 more second via "a->tv_sec +=..."
> on a virtual machine, that's why we got occasional errors like:
> 
> shmctl01.c:183: TFAIL: SHM_STAT: shm_ctime=1604298585, expected <1604298586,1604298586>
> ...
> msgsnd01.c:59: TFAIL: msg_stime = 1605730573 out of [1605730574, 1605730574]
> 
> Here we propose to use '__NR_time' to invoke syscall directly that makes
> test all get real seconds via ktime_get_real_second.
> 
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Chunyu Hu <chuhu@redhat.com>
> Cc: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c | 5 +++--
>  testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c | 5 +++--
>  testcases/kernel/syscalls/ipc/shmctl/shmctl01.c | 5 +++--
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
> index 5c1e317e9..6fdc47dc3 100644
> --- a/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
> +++ b/testcases/kernel/syscalls/ipc/msgrcv/msgrcv01.c
> @@ -10,6 +10,7 @@
>  #include "tst_test.h"
>  #include "tst_safe_sysv_ipc.h"
>  #include "libnewipc.h"
> +#include "tst_timer.h"
>  
>  static key_t msgkey;
>  static int queue_id = -1, pid;
> @@ -25,13 +26,13 @@ static void verify_msgrcv(void)
>  
>  	SAFE_MSGSND(queue_id, &snd_buf, MSGSIZE, 0);
>  
> -	time(&before_rcv);
> +	tst_syscall(__NR_time, &before_rcv);
>  	TEST(msgrcv(queue_id, &rcv_buf, MSGSIZE, 1, 0));
>  	if (TST_RET == -1) {
>  		tst_res(TFAIL | TTERRNO, "msgrcv failed");
>  		return;
>  	}
> -	time(&after_rcv);
> +	tst_syscall(__NR_time, &after_rcv);
>  
>  	if (strcmp(rcv_buf.mtext, snd_buf.mtext) == 0)
>  		tst_res(TPASS, "message received(%s) = message sent(%s)",
> diff --git a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
> index 5f5da52d2..9101f2668 100644
> --- a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
> +++ b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd01.c
> @@ -16,6 +16,7 @@
>  #include "tst_test.h"
>  #include "tst_safe_sysv_ipc.h"
>  #include "libnewipc.h"
> +#include "tst_timer.h"
>  
>  static key_t msgkey;
>  static int queue_id = -1, pid;
> @@ -29,13 +30,13 @@ static void verify_msgsnd(void)
>  	struct msqid_ds qs_buf;
>  	time_t before_snd, after_snd;
>  
> -	time(&before_snd);
> +	tst_syscall(__NR_time, &before_snd);
>  	TEST(msgsnd(queue_id, &snd_buf, MSGSIZE, 0));
>  	if (TST_RET == -1) {
>  		tst_res(TFAIL | TTERRNO, "msgsnd() failed");
>  		return;
>  	}
> -	time(&after_snd);
> +	tst_syscall(__NR_time, &after_snd);
>  
>  	SAFE_MSGCTL(queue_id, IPC_STAT, &qs_buf);
>  
> diff --git a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
> index 3a39a4d74..f5b8eaef9 100644
> --- a/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
> +++ b/testcases/kernel/syscalls/ipc/shmctl/shmctl01.c
> @@ -19,6 +19,7 @@
>  #include "tst_test.h"
>  #include "tst_safe_sysv_ipc.h"
>  #include "libnewipc.h"
> +#include "tst_timer.h"
>  
>  #define NCHILD 20
>  
> @@ -240,9 +241,9 @@ static int get_shm_idx_from_id(int shm_id)
>  
>  static void setup(void)
>  {
> -	ctime_min = time(NULL);
> +	ctime_min = tst_syscall(__NR_time, NULL);
>  	shm_id = SAFE_SHMGET(IPC_PRIVATE, SHM_SIZE, IPC_CREAT | SHM_RW);
> -	ctime_max = time(NULL);
> +	ctime_max = tst_syscall(__NR_time, NULL);
>  
>  	shm_idx = get_shm_idx_from_id(shm_id);
>  
> -- 
> 2.21.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Thomas Gleixner]

Cyril,

On Tue, Nov 24 2020 at 16:38, Cyril Hrubis wrote:
> Thomas can you please have a look? It looks like we can get the SysV IPC
> ctime to be one second off compared to what we get from realtime clock.
>
> Do we care to get this fixed in kernel or should we fix the tests?

See below.

>> This shmctl01 test detect the time as the number of seconds twice
>> (before and after) the shmget() instruction, then it verifies
>> whether the 'struct shmid_ds ds' gets data correctly. But here it
>> shows 'ds->ctime' out of the seconds range (1604298586, 1604298586),
>> 
>> The reason is that shmget()/msgsnd() always use ktime_get_real_second
>> to get real seconds, but time() on aarch64 via gettimeofday() or (it
>> depends on different kernel versions) clock_gettime() in use-level's
>> VDSO to return tv_sec.
>> 
>> time()
>>   __cvdso_gettimeofday
>>    ...
>>      do_gettimeofday
>>        ktime_get_real_ts64
>>         timespc64_add_ns
>> 
>> Situation can be simplify as difference between ktime_get_real_second
>> and ktime_get_real_ts64. As we can see ktime_get_real_second return
>> tk->xtime_sec directly, however
>> 
>> timespc64_add_ns more easily add 1 more second via "a->tv_sec +=..."
>> on a virtual machine, that's why we got occasional errors like:
>> 
>> shmctl01.c:183: TFAIL: SHM_STAT: shm_ctime=1604298585, expected <1604298586,1604298586>
>> ...
>> msgsnd01.c:59: TFAIL: msg_stime = 1605730573 out of [1605730574, 1605730574]
>> 
>> Here we propose to use '__NR_time' to invoke syscall directly that makes
>> test all get real seconds via ktime_get_real_second.

This is a general problem and not really just for this particular test
case.

Due to the internal implementation of ktime_get_real_seconds(), which is
a 2038 safe replacement for the former get_seconds() function, this
accumulation issue can be observed. (time(2) via syscall and newer
versions of VDSO use the same mechanism).

     clock_gettime(CLOCK_REALTIME, &ts);
     sec = time();
     assert(sec >= ts.tv_sec);

That assert can trigger for two reasons:

 1) Clock was set between the clock_gettime() and time().

 2) The clock has advanced far enough that:

    timekeeper.tv_nsec + (clock_now_ns() - last_update_ns) > NSEC_PER_SEC

#1 is just a property of clock REALTIME. There is nothing we can do
   about that.

#2 is due to the optimized get_seconds()/time() access which avoids to
   read the clock. This can happen on bare metal as well, but is far
   more likely to be exposed on virt.

The same problem exists for CLOCK_XXX vs. CLOCK_XXX_COARSE

     clock_gettime(CLOCK_XXX, &ts);
     clock_gettime(CLOCK_XXX_COARSE, &tc);
     assert(tc.tv_sec >= ts.tv_sec);

The _COARSE variants return their associated timekeeper.tv_sec,tv_nsec
pair without reading the clock. Same as #2 above just extended to clock
MONOTONIC.

There is no way to fix this except giving up on the fast accessors and
make everything take the slow path and read the clock, which might make
a lot of people unhappy.

For clock REALTIME #1 is anyway an issue, so I think documenting this
proper is the right thing to do.

Thoughts?

Thanks,

        tglx

[LTP] [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Thomas Gleixner]

Cyril,

On Tue, Nov 24 2020 at 16:38, Cyril Hrubis wrote:
> Thomas can you please have a look? It looks like we can get the SysV IPC
> ctime to be one second off compared to what we get from realtime clock.
>
> Do we care to get this fixed in kernel or should we fix the tests?

See below.

>> This shmctl01 test detect the time as the number of seconds twice
>> (before and after) the shmget() instruction, then it verifies
>> whether the 'struct shmid_ds ds' gets data correctly. But here it
>> shows 'ds->ctime' out of the seconds range (1604298586, 1604298586),
>> 
>> The reason is that shmget()/msgsnd() always use ktime_get_real_second
>> to get real seconds, but time() on aarch64 via gettimeofday() or (it
>> depends on different kernel versions) clock_gettime() in use-level's
>> VDSO to return tv_sec.
>> 
>> time()
>>   __cvdso_gettimeofday
>>    ...
>>      do_gettimeofday
>>        ktime_get_real_ts64
>>         timespc64_add_ns
>> 
>> Situation can be simplify as difference between ktime_get_real_second
>> and ktime_get_real_ts64. As we can see ktime_get_real_second return
>> tk->xtime_sec directly, however
>> 
>> timespc64_add_ns more easily add 1 more second via "a->tv_sec +=..."
>> on a virtual machine, that's why we got occasional errors like:
>> 
>> shmctl01.c:183: TFAIL: SHM_STAT: shm_ctime=1604298585, expected <1604298586,1604298586>
>> ...
>> msgsnd01.c:59: TFAIL: msg_stime = 1605730573 out of [1605730574, 1605730574]
>> 
>> Here we propose to use '__NR_time' to invoke syscall directly that makes
>> test all get real seconds via ktime_get_real_second.

This is a general problem and not really just for this particular test
case.

Due to the internal implementation of ktime_get_real_seconds(), which is
a 2038 safe replacement for the former get_seconds() function, this
accumulation issue can be observed. (time(2) via syscall and newer
versions of VDSO use the same mechanism).

     clock_gettime(CLOCK_REALTIME, &ts);
     sec = time();
     assert(sec >= ts.tv_sec);

That assert can trigger for two reasons:

 1) Clock was set between the clock_gettime() and time().

 2) The clock has advanced far enough that:

    timekeeper.tv_nsec + (clock_now_ns() - last_update_ns) > NSEC_PER_SEC

#1 is just a property of clock REALTIME. There is nothing we can do
   about that.

#2 is due to the optimized get_seconds()/time() access which avoids to
   read the clock. This can happen on bare metal as well, but is far
   more likely to be exposed on virt.

The same problem exists for CLOCK_XXX vs. CLOCK_XXX_COARSE

     clock_gettime(CLOCK_XXX, &ts);
     clock_gettime(CLOCK_XXX_COARSE, &tc);
     assert(tc.tv_sec >= ts.tv_sec);

The _COARSE variants return their associated timekeeper.tv_sec,tv_nsec
pair without reading the clock. Same as #2 above just extended to clock
MONOTONIC.

There is no way to fix this except giving up on the fast accessors and
make everything take the slow path and read the clock, which might make
a lot of people unhappy.

For clock REALTIME #1 is anyway an issue, so I think documenting this
proper is the right thing to do.

Thoughts?

Thanks,

        tglx

Re: [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Cyril Hrubis]

Hi!
> This is a general problem and not really just for this particular test
> case.
> 
> Due to the internal implementation of ktime_get_real_seconds(), which is
> a 2038 safe replacement for the former get_seconds() function, this
> accumulation issue can be observed. (time(2) via syscall and newer
> versions of VDSO use the same mechanism).
> 
>      clock_gettime(CLOCK_REALTIME, &ts);
>      sec = time();
>      assert(sec >= ts.tv_sec);
> 
> That assert can trigger for two reasons:
> 
>  1) Clock was set between the clock_gettime() and time().
> 
>  2) The clock has advanced far enough that:
> 
>     timekeeper.tv_nsec + (clock_now_ns() - last_update_ns) > NSEC_PER_SEC
> 
> #1 is just a property of clock REALTIME. There is nothing we can do
>    about that.
> 
> #2 is due to the optimized get_seconds()/time() access which avoids to
>    read the clock. This can happen on bare metal as well, but is far
>    more likely to be exposed on virt.
> 
> The same problem exists for CLOCK_XXX vs. CLOCK_XXX_COARSE
> 
>      clock_gettime(CLOCK_XXX, &ts);
>      clock_gettime(CLOCK_XXX_COARSE, &tc);
>      assert(tc.tv_sec >= ts.tv_sec);
> 
> The _COARSE variants return their associated timekeeper.tv_sec,tv_nsec
> pair without reading the clock. Same as #2 above just extended to clock
> MONOTONIC.

Good hint, I guess that easiest fix would be to switch to coarse timers
for these tests.

> There is no way to fix this except giving up on the fast accessors and
> make everything take the slow path and read the clock, which might make
> a lot of people unhappy.

That's understandable and reasonable. Thanks a lot for the confirmation.

> For clock REALTIME #1 is anyway an issue, so I think documenting this
> proper is the right thing to do.
> 
> Thoughts?

I guess that ideally BUGS section for time(2) and clock_gettime(2)
should be updated with this explanation.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Cyril Hrubis]

Hi!
> This is a general problem and not really just for this particular test
> case.
> 
> Due to the internal implementation of ktime_get_real_seconds(), which is
> a 2038 safe replacement for the former get_seconds() function, this
> accumulation issue can be observed. (time(2) via syscall and newer
> versions of VDSO use the same mechanism).
> 
>      clock_gettime(CLOCK_REALTIME, &ts);
>      sec = time();
>      assert(sec >= ts.tv_sec);
> 
> That assert can trigger for two reasons:
> 
>  1) Clock was set between the clock_gettime() and time().
> 
>  2) The clock has advanced far enough that:
> 
>     timekeeper.tv_nsec + (clock_now_ns() - last_update_ns) > NSEC_PER_SEC
> 
> #1 is just a property of clock REALTIME. There is nothing we can do
>    about that.
> 
> #2 is due to the optimized get_seconds()/time() access which avoids to
>    read the clock. This can happen on bare metal as well, but is far
>    more likely to be exposed on virt.
> 
> The same problem exists for CLOCK_XXX vs. CLOCK_XXX_COARSE
> 
>      clock_gettime(CLOCK_XXX, &ts);
>      clock_gettime(CLOCK_XXX_COARSE, &tc);
>      assert(tc.tv_sec >= ts.tv_sec);
> 
> The _COARSE variants return their associated timekeeper.tv_sec,tv_nsec
> pair without reading the clock. Same as #2 above just extended to clock
> MONOTONIC.

Good hint, I guess that easiest fix would be to switch to coarse timers
for these tests.

> There is no way to fix this except giving up on the fast accessors and
> make everything take the slow path and read the clock, which might make
> a lot of people unhappy.

That's understandable and reasonable. Thanks a lot for the confirmation.

> For clock REALTIME #1 is anyway an issue, so I think documenting this
> proper is the right thing to do.
> 
> Thoughts?

I guess that ideally BUGS section for time(2) and clock_gettime(2)
should be updated with this explanation.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Vincenzo Frascino]

Hi Thomas.

On 11/25/20 11:32 AM, Thomas Gleixner wrote:
[...]

>>> Here we propose to use '__NR_time' to invoke syscall directly that makes
>>> test all get real seconds via ktime_get_real_second.
> 
> This is a general problem and not really just for this particular test
> case.
> 
> Due to the internal implementation of ktime_get_real_seconds(), which is
> a 2038 safe replacement for the former get_seconds() function, this
> accumulation issue can be observed. (time(2) via syscall and newer
> versions of VDSO use the same mechanism).
> 
>      clock_gettime(CLOCK_REALTIME, &ts);
>      sec = time();
>      assert(sec >= ts.tv_sec);
> 
> That assert can trigger for two reasons:
> 
>  1) Clock was set between the clock_gettime() and time().
> 
>  2) The clock has advanced far enough that:
> 
>     timekeeper.tv_nsec + (clock_now_ns() - last_update_ns) > NSEC_PER_SEC
> 
> #1 is just a property of clock REALTIME. There is nothing we can do
>    about that.
> 
> #2 is due to the optimized get_seconds()/time() access which avoids to
>    read the clock. This can happen on bare metal as well, but is far
>    more likely to be exposed on virt.
> 
> The same problem exists for CLOCK_XXX vs. CLOCK_XXX_COARSE
> 
>      clock_gettime(CLOCK_XXX, &ts);
>      clock_gettime(CLOCK_XXX_COARSE, &tc);
>      assert(tc.tv_sec >= ts.tv_sec);
> 
> The _COARSE variants return their associated timekeeper.tv_sec,tv_nsec
> pair without reading the clock. Same as #2 above just extended to clock
> MONOTONIC.
> 
> There is no way to fix this except giving up on the fast accessors and
> make everything take the slow path and read the clock, which might make
> a lot of people unhappy.
> 
> For clock REALTIME #1 is anyway an issue, so I think documenting this
> proper is the right thing to do.
> 
> Thoughts?
>

I completely agree with your analysis and the fact that we should document this
information.

My proposal would be to use either the vDSO document present in the kernel [1]
or the man pages for time(2) and clock_gettime(2). Probably the second would be
more accessible to user space developers.

[1] Documentation/ABI/stable/vdso

> Thanks,
> 
>         tglx
> 

-- 
Regards,
Vincenzo

[LTP] [PATCH 1/2] syscalls: avoid time() using __cvdso_gettimeofday in use-level's VDSO

[Vincenzo Frascino]

Hi Thomas.

On 11/25/20 11:32 AM, Thomas Gleixner wrote:
[...]

>>> Here we propose to use '__NR_time' to invoke syscall directly that makes
>>> test all get real seconds via ktime_get_real_second.
> 
> This is a general problem and not really just for this particular test
> case.
> 
> Due to the internal implementation of ktime_get_real_seconds(), which is
> a 2038 safe replacement for the former get_seconds() function, this
> accumulation issue can be observed. (time(2) via syscall and newer
> versions of VDSO use the same mechanism).
> 
>      clock_gettime(CLOCK_REALTIME, &ts);
>      sec = time();
>      assert(sec >= ts.tv_sec);
> 
> That assert can trigger for two reasons:
> 
>  1) Clock was set between the clock_gettime() and time().
> 
>  2) The clock has advanced far enough that:
> 
>     timekeeper.tv_nsec + (clock_now_ns() - last_update_ns) > NSEC_PER_SEC
> 
> #1 is just a property of clock REALTIME. There is nothing we can do
>    about that.
> 
> #2 is due to the optimized get_seconds()/time() access which avoids to
>    read the clock. This can happen on bare metal as well, but is far
>    more likely to be exposed on virt.
> 
> The same problem exists for CLOCK_XXX vs. CLOCK_XXX_COARSE
> 
>      clock_gettime(CLOCK_XXX, &ts);
>      clock_gettime(CLOCK_XXX_COARSE, &tc);
>      assert(tc.tv_sec >= ts.tv_sec);
> 
> The _COARSE variants return their associated timekeeper.tv_sec,tv_nsec
> pair without reading the clock. Same as #2 above just extended to clock
> MONOTONIC.
> 
> There is no way to fix this except giving up on the fast accessors and
> make everything take the slow path and read the clock, which might make
> a lot of people unhappy.
> 
> For clock REALTIME #1 is anyway an issue, so I think documenting this
> proper is the right thing to do.
> 
> Thoughts?
>

I completely agree with your analysis and the fact that we should document this
information.

My proposal would be to use either the vDSO document present in the kernel [1]
or the man pages for time(2) and clock_gettime(2). Probably the second would be
more accessible to user space developers.

[1] Documentation/ABI/stable/vdso

> Thanks,
> 
>         tglx
> 

-- 
Regards,
Vincenzo