Message-ID: <CAL+tcoBhdFS5wjE7v7G_Hf9SsTkqiaGqWTE8Zp-NyJA7F6pspw@mail.gmail.com>
Date: Thu, 5 Sep 2024 21:40:57 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, shuah@...nel.org, linux-kselftest@...r.kernel.org, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] selftests: return failure when timestamps can't
 be parsed

On Thu, Sep 5, 2024 at 7:04 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> On Thu, Sep 5, 2024 at 5:16 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <kernelxing@...cent.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.
> >
> > > 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@...cent.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.
> >
> > > ---
> > >  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};

If so, rxtimestamp test will fail. Let me correct myself here.

>
> which I intentionally wrote is used to show one stupid bug as an
> example. The txtimestamp test should spot it :)

Sorry, not in tcp_recv_timestamp, but like in __sock_recv_timestamp:

j@@ -946,7 +946,7 @@ void __sock_recv_timestamp(struct msghdr *msg,
struct sock *sk,

        memset(&tss, 0, sizeof(tss));
        tsflags = READ_ONCE(sk->sk_tsflags);
-       if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
+       if (!(tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
            ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
                empty = 0;
        if (shhwtstamps &&

This error/bug cannot be noticed by txtimestamp before this patch.

It's just an example which helps me clarify my thoughts.

>
> Thanks,
> Jason

Powered by blists - more mailing lists