On Thu, Sep 5, 2024 at 5:16 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
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.
I was trying to say if someone writes a bug in the timestamping feature, the selftest probably returns pass value instead of failure.
I will explicitly report the case I met.
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
Will update it.
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@tencent.com
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.
Thanks. Now I am learning :)
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).
I see.
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.
Right, It does make sense.
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).
Thanks for pointing out this one. I will rewrite this patch/patch series tomorrow with your questions resolved.
Thanks, Jason