Message-ID: <66ce07f174845_2a065029481@willemb.c.googlers.com.notmuch>
Date: Tue, 27 Aug 2024 13:08:01 -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

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

Powered by blists - more mailing lists