On Thu, Sep 5, 2024 at 5:16 AM Willem de Bruijn
<willemdebruijn.kernel@xxxxxxxxx> wrote:
>
> 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.
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@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.
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
