Message-ID: <66cf29b67d686_336087294f0@willemb.c.googlers.com.notmuch>
Date: Wed, 28 Aug 2024 09:44:22 -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

> > > 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) ||

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 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.

> 3) remove the last key SOCK_TIMESTAMPING_RX_SOFTWARE from enum
> sk_flags, if you want me to do so :)

Nope.

> If there is something weird here, please point it out so that I can
> make the right move.
> 
> Thanks,
> Jason

Powered by blists - more mailing lists