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.
>
> > 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.
Today I re-read this paragraph. I think we were just past each other.
I would like to check that if the reporting timestamp with someone's
patch applied someday wouldn't work, the txtimestamp should return
failure to warn the submitter. That is to say, we succeed to generate
the skb with timestamp but failed to report it like this:
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 8a5680b4e786..65f7947322cd 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2274,7 +2274,7 @@ void tcp_recv_timestamp(struct msghdr *msg,
const struct sock *sk,
}
}
- if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
+ if (!(READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE))
has_timestamping = true;
else
tss->ts[0] = (struct timespec64) {0};
which I intentionally wrote is used to show one stupid bug as an
example. The txtimestamp test should spot it :)
Thanks,
Jason
