Message-ID: <CAL+tcoDVhYpQwZ7xX4Lv+0SWuQOKMpRiJxH=R9v+M8-Lp9HGzA@mail.gmail.com>
Date: Wed, 28 Aug 2024 08:43:23 +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 1:08 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> > > > > > Besides those two concepts you mentioned, could you explain if there
> > > > > > are side effects that the series has and what kind of bad consequences
> > > > > > that the series could bring?
> > > > >
> > > > > It doesn't do the same for hardware timestamping, creating
> > > > > inconsistency.
> > >
> > > Taking a closer look at the code, there are actually already two weird
> > > special cases here.
> > >
> > > SOF_TIMESTAMPING_RX_HARDWARE never has to be passed, as rx hardware
> > > timestamp generation is configured through SIOCSHWTSTAMP.
> >
> > Do you refer to the patch [1/2] I wrote? To be more specific, is it
> > about the above wrong commit message which I just modified?
> >
> > Things could happen when other unrelated threads set
> > SOF_TIMESTAMPING_RX_SOFTWARE instead of SOF_TIMESTAMPING_RX_HARDWARE.
> >
> > Sorry for the confusion.
>
> No, this is referring to the current state.
>
> > >
> > > SOF_TIMESTAMPING_RX_SOFTWARE already enables timestamp reporting from
> > > sock_recv_timestamp(), while reporting should not be conditional on
> > > this generation flag.
> >
> > I'm not sure if you're talking about patch [2/2] in the series. But I guess so.
>
> Nope, same thing. I mention a commit from 2014.

I thought you asked me to change these two last night. Actually you
were only stating the fact: two cases where we use both generation and
reporting flags already exist before.

Okay, I finally got it. It's fine to me from my point of view :)

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

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.

Thanks for your patience:)

> >
> > I'm not so sure about the unix case, I can see this call trace:
> > unix_dgram_recvmsg()->__unix_dgram_recvmsg()->__sock_recv_timestamp().
> >
> > The reason why I added the check in in __sock_recv_timestamp () in the
> > patch [2/2] is considering the above call trace.
> >
> > One thing I can be sure of is that removing the modification in
> > __sock_recv_timestamp in that patch doesn't affect the selftests.
> >
> > Please correct me if I'm wrong.
>
> I think we're talking alongside each other. I was pointing to code
> before your patch.
>
> > >
> > > > >
> > > > > Changing established interfaces always risks production issues. In
> > > > > this case, I'm not convinced that the benefit outweighs this risk.
> > > >
> > > > I got it.
> > > >
> > > > I'm thinking that I'm not the first one and the last one who know/find
> > > > this long standing "issue", could we at least documentented it
> > > > somewhere, like adding comments in the selftests or Documentation, to
> > > > avoid the similar confusion in the future? Or change the behaviour in
> > > > the rxtimestamp.c test? What do you think about it? Adding
> > > > documentation or comments is the simplest way:)
> > >
> > > 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.
3) remove the last key SOCK_TIMESTAMPING_RX_SOFTWARE from enum
sk_flags, if you want me to do so :)

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