Message-ID: <66cf2fcbb0bd5_338e3529467@willemb.c.googlers.com.notmuch>
Date: Wed, 28 Aug 2024 10:10:19 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 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

Jason Xing wrote:
> On Wed, Aug 28, 2024 at 9:44 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > > > > I can see what you mean here: you don't like combining the reporting
> > > > > flag and generation flag, right? But If we don't check whether those
> > > > > two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> > > > > SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> > > > > protocols like udp will fail as we talked before.
> > > > >
> > > > > netstamp_needed_key cannot be implemented as per socket feature (at
> > > > > that time when the driver just pass the skb to the rx stack, we don't
> > > > > know which socket the skb belongs to). Since we cannot prevent this
> > > > > from happening during its generation period, I suppose we can delay
> > > > > the check and try to stop it when it has to report, I mean, in
> > > > > sock_recv_timestamp().
> > > > >
> > > > > Or am I missing something? What would you suggest?
> > > > >
> > > > > >
> > > > > >         /*
> > > > > >          * generate control messages if
> > > > > >          * - receive time stamping in software requested
> > > > > >          * - software time stamp available and wanted
> > > > > >          * - hardware time stamps available and wanted
> > > > > >          */
> > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > > > > >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > > > > >             (hwtstamps->hwtstamp &&
> > > > > >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > > > > >                 __sock_recv_timestamp(msg, sk, skb);
> > > > > >
> > > > > > I evidently already noticed this back in 2014, when I left a note in
> > > > > > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > > > > > sk_flags"):
> > > > > >
> > > > > >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> > > > > >     timestamp logic (netstamp_needed). That can be simplified and this
> > > > > >     last key removed, but will leave that for a separate patch.
> > > > > >
> > > > > > But I do not see __sock_recv_timestamp toggling the feature either
> > > > > > then or now, so I think this is vestigial and can be removed.
> > >
> > > After investigating more of it, as your previous commit said, the
> > > legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
> > > SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
> > > flag from enum sock_flags {}, right? Do you expect me to do that? If
> > > so, I would love to do it :)
> >
> > I did not say that. I said that the specific line here appears
> > vestigial.
> >
> > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> 
> Thanks for your patience.
> 
> I think I will remove the above two lines as one patch with your
> suggested-by tag in the series so that later we can easily review each
> of them.

Please don't. As I said right below this:

One thing at a time. Let's focus on the change you proposed to me.

The above is just a hunch on first read. I would have to spend more
time to convince myself that it is indeed correct and safe. At which
point I might as well send it myself too..

More importantly, let's not expand what is intended to be a stand
alone improvement with tangential changes. It just makes reviewing
harder and slows it down.
 
> >
> > One thing at a time. Let's focus on the change you proposed to me.
> >
> > > But I still don't get it when you say "__sock_recv_timestamp toggling
> > > the feature", could you say more, please? I'm not sure if it has
> > > something to do with the above line.
> >
> > SOF_TIMESTAMPING_RX_SOFTWARE is a request to enable software receive
> > timestamp *generation*. This is done by calling net_enable_timestamp.
> >
> > I did not immediately see a path from __sock_recv_timestamp to
> > net_enable_timestamp, so I don't see a point in entering that function
> > based on this flag.
> 
> I see. It does make sense. We don't need the generation flag to test
> if we need to report here.
> 
> I will remove them by quoting your saying.

See above.

> >
> > > > > > I can see the value of your extra filter. Given the above examples, it
> > > > > > won't be the first subtle variance from the API design, either.
> > > > >
> > > > > Really appreciate that you understand me :)
> > > > >
> > > > > >
> > > > > > So either way is fine with me: change it or leave it.
> > > > > >
> > > > > > But in both ways, yes: please update the documentation accordingly.
> > > > >
> > > > > Roger that, sir. I will do it.
> > > > >
> > > > > >
> > > > > > And if you do choose to change it, please be ready to revert on report
> > > > > > of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
> > > > > > because that always worked as they subtly relied on another daemon to
> > > > > > enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.
> > > > >
> > > > > Yes, I still chose to change it and try to make it in the correct
> > > > > direction. So if there are future reports, please let me know, I will
> > > > > surely keep a close eye on it.
> > > >
> > > > Sounds good, thanks.
> > >
> > > So let me organize my thoughts here.
> > >
> > > In the next move, I would do such things:
> > > 1) keep two patches in this series as they are.
> > > 2) add some descriptions about "this commit introduces subtle
> > > variance, if the application that only pass
> > > SOF_TIMESTAMPING_SOFTWARE..." something like this in the Documentation
> > > file.
> >
> > Make it crystal clear that the distinction between timestamp
> > generation and timestamp reporting, which this document goes out of
> > its way to explain, does not hold for receive timestamping.
> 
> Sure, I will add it into the documentation as well.
> 
> Thanks for your help.

Powered by blists - more mailing lists