Message-ID: <CAL+tcoBWHqVzjesJqmmgUrX5cvKtLp_L9PZz+d+-b0FBXpatVg@mail.gmail.com>
Date: Mon, 26 Aug 2024 21:40:34 +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, dsahern@...nel.org, willemb@...gle.com, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 1/2] tcp: make SOF_TIMESTAMPING_RX_SOFTWARE
 feature per socket

Hello Willem,

On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > Normally, if we want to record and print the rx timestamp after
> > tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
> > and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
> > through running rxtimestamp binary in selftests (see testcase 7).
> >
> > However, there is one particular case that fails the selftests with
> > "./rxtimestamp: Expected swtstamp to not be set." error printing in
> > testcase 6.
> >
> > How does it happen? When we keep running a thread starting a socket
> > and set SOF_TIMESTAMPING_RX_HARDWARE option first, then running
> > ./rxtimestamp, it will fail. The reason is the former thread
> > switching on netstamp_needed_key that makes the feature global,
> > every skb going through netif_receive_skb_list_internal() function
> > will get a current timestamp in net_timestamp_check(). So the skb
> > will have timestamp regardless of whether its socket option has
> > SOF_TIMESTAMPING_RX_SOFTWARE or not.
> >
> > After this patch, we can pass the selftest and control each socket
> > as we want when using rx timestamp feature.
> >
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> >  net/ipv4/tcp.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 8514257f4ecd..49e73d66c57d 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -2235,6 +2235,7 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       struct scm_timestamping_internal *tss)
> >  {
> >       int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
> > +     u32 tsflags = READ_ONCE(sk->sk_tsflags);
> >       bool has_timestamping = false;
> >
> >       if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) {
> > @@ -2274,14 +2275,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       }
> >               }
> >
> > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > +             /* skb may contain timestamp because another socket
> > +              * turned on netstamp_needed_key which allows generate
> > +              * the timestamp. So we need to check the current socket.
> > +              */
> > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> >                       has_timestamping = true;
> >               else
> >                       tss->ts[0] = (struct timespec64) {0};
> >       }
>
> The current behavior is as described in
> Documentation/networking/timestamping.rst:
>
> "The socket option configures timestamp generation for individual
> sk_buffs (1.3.1), timestamp reporting to the socket's error
> queue (1.3.2)"
>
> SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
> SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.

Thanks for your review.

Yes, it's true.

>
> This patch changes that clearly defined behavior.

Why? I don't get it. Please see those testcase in
tools/testing/selftests/net/rxtimestamp.c.

>
> On Tx the separation between generation and reporting has value, as it
> allows setting the generation on a per packet basis with SCM_TSTAMP_*.

I didn't break the logic on the tx path. tcp_recv_timestamp() is only
related to the rx path.

Regarding the tx path, I carefully take care of this logic in
patch[2/2], so now the series only handles the issue happening in the
rx path.

>
> On Rx it is more subtle, but the two are still tested at different
> points in the path, and can be updated by setsockopt in between a
> packet arrival and a recvmsg().
>
> The interaction between sockets on software timestamping is a
> longstanding issue. I don't think there is any urgency to change this

Oh, now I see.

> now. This proposed change makes the API less consistent, and may
> also affect applications that depend on the current behavior.
>

Maybe. But, it's not the original design which we expect, right?

I can make sure this series can fix the issue. This series is trying
to ask users to use/set both flags to receive an expected timestamp.
The purpose of using setsockopt is to control the socket itself
insteading of interfering with others.

About the breakage issue, let me assume, if the user only sets the
SOF_TIMESTAMPING_SOFTWARE flag, he cannot expect the socket will
receive a timestamp, right? So what might happen if we fix the issue?
I think the logics in the rxtimestamp.c selftest are very clear :)

Besides, test case 6 will fail under this circumstance.

Thanks,
Jason

Powered by blists - more mailing lists