Re: [PATCH net-next] selftests: return failure when timestamps can't be parsed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 






[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux