Message-ID: <CAL+tcoDGZ_uWwtDAOSUS4-e+VXGuvJ16emyGymQT5vTF+-A_DA@mail.gmail.com>
Date: Wed, 28 Aug 2024 22:02:46 +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

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.

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

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