Jason Xing wrote:
> From: Jason Xing <kernelxing@xxxxxxxxxxx>
>
> When I was trying to modify the tx timestamping feature, I found that
> running "./txtimestamp -4 -C -L 127.0.0.1" didn't reflect the fact
> properly.
Did not reflect what fact? Sorry, I don't entirely follow the issue
you raise.
> In this selftest file, we respectively test three tx generation flags.
> With the generation and report flag enabled, we expect that the timestamp
> must be returned to the userspace unless 1) generating the timestamp
> fails, 2) reporting the timestamp fails. So we should test if the
> timestamps can be read and parsed succuessfuly in txtimestamp.c, or
typo: successfully
> else there is a bug in the kernel.
>
> After adding the check so that running ./txtimestamp will reflect the
> result correctly like this if there is an error in kernel:
> protocol: TCP
> payload: 10
> server port: 9000
>
> family: INET
> test SND
> USR: 1725458477 s 667997 us (seq=0, len=0)
> Failed to parse timestamps
> USR: 1725458477 s 718128 us (seq=0, len=0)
> Failed to parse timestamps
> USR: 1725458477 s 768273 us (seq=0, len=0)
> Failed to parse timestamps
> USR: 1725458477 s 818416 us (seq=0, len=0)
> Failed to parse timestamps
> ...
>
> Signed-off-by: Jason Xing <kernelxing@xxxxxxxxxxx>
> ---
> I'm not sure if I should also check if the cur->tv_sec or cur->tv_nsec
> is zero in __print_timestamp(). Could it be valid when either of
> them is zero?
tv_nsec can be zero. tv_sec cannot.
> ---
> tools/testing/selftests/net/txtimestamp.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/net/txtimestamp.c b/tools/testing/selftests/net/txtimestamp.c
> index ec60a16c9307..b69aae840a67 100644
> --- a/tools/testing/selftests/net/txtimestamp.c
> +++ b/tools/testing/selftests/net/txtimestamp.c
> @@ -358,6 +358,10 @@ static void __recv_errmsg_cmsg(struct msghdr *msg, int payload_len)
>
> if (batch > 1)
> fprintf(stderr, "batched %d timestamps\n", batch);
> + else if (!batch) {
> + fprintf(stderr, "Failed to parse timestamps\n");
> + test_failed = true;
> + }
nit: if adding braces around one side of a branch, then add to both (all).
This is not so much a parsing failure as that no timestamps arrived.
More importantly, this function gets called also if
recvmsg(fd, .., MSG_ERRQUEUE) returned 0:
if (ret >= 0) {
__recv_errmsg_cmsg(&msg, ret);
That seems counterintuitive, as there is no data. But this was
introduced with cfg_loop_nodata (SOF_TIMESTAMPING_OPT_TSONLY). When
there may be packets looped, just 0B packets. In those cases we also
expect timestamps.
But, can __recv_errmsg_cmsg now also be called when there truly is
nothing on the error queue? It is a non-blocking read, after all.
Judging from
while (!recv_errmsg(fd)) {}
The caller can. But if there is nothing waiting it returns -1 with
EAGAIN:
ret = recvmsg(fd, &msg, MSG_ERRQUEUE);
if (ret == -1 && errno != EAGAIN)
error(1, errno, "recvmsg");
So long story short, subject to a few nits your patch sounds okay to
me (but it's not entirely trivial that that is so: sharing so that you
also double check, thanks).
> }
>
> static int recv_errmsg(int fd)
> --
> 2.37.3
>
