Message-ID: <66ddfdece6d32_2fb98729447@willemb.c.googlers.com.notmuch>
Date: Sun, 08 Sep 2024 15:41:32 -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, 
 shuah@...nel.org, 
 willemb@...gle.com, 
 linux-kselftest@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v5 1/2] net-timestamp: introduce
 SOF_TIMESTAMPING_OPT_RX_FILTER flag

Jason Xing wrote:
> On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Jason Xing wrote:
> > > On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn
> > > <willemdebruijn.kernel@...il.com> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@...cent.com>
> > > > >
> > > > > introduce a new flag SOF_TIMESTAMPING_OPT_RX_FILTER in the receive
> > > > > path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
> > > > > out rx software timestamp report, especially after a process turns on
> > > > > netstamp_needed_key which can time stamp every incoming skb.
> > > > >
> > > > > Previously, we found out if an application starts first which turns on
> > > > > netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> > > > > could also get rx timestamp. Now we handle this case by introducing this
> > > > > new flag without breaking users.
> > > > >
> > > > > Quoting Willem to explain why we need the flag:
> > > > > "why a process would want to request software timestamp reporting, but
> > > > > not receive software timestamp generation. The only use I see is when
> > > > > the application does request
> > > > > SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE."
> > > > >
> > > > > Similarly, this new flag could also be used for hardware case where we
> > > > > can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive
> > > > > hardware receive timestamp.
> > > > >
> > > > > Another thing about errqueue in this patch I have a few words to say:
> > > > > In this case, we need to handle the egress path carefully, or else
> > > > > reporting the tx timestamp will fail. Egress path and ingress path will
> > > > > finally call sock_recv_timestamp(). We have to distinguish them.
> > > > > Errqueue is a good indicator to reflect the flow direction.
> > > > >
> > > > > Suggested-by: Willem de Bruijn <willemb@...gle.com>
> > > > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > >
> > > > High level: where is the harm in receiving unsolicited timestamps?
> > > > A process can easily ignore them. I do wonder if the only use case is
> > > > an overly strict testcase. Was reminded of this as I tried to write
> > > > a more concise paragraph for the documentation.
> > >
> > > You raised a good question.
> > >
> > > I think It's more of a design consideration instead of a bugfix
> > > actually. So it is not solving a bug which makes the apps wrong but
> > > gives users a hint that we can explicitly and accurately do what we
> > > want and we expect.
> > >
> > > Let's assume: if we remove all the report flags design, what will
> > > happen? It can work of course. I don't believe that people choose to
> > > enable the generation flag but are not willing to report it. Of
> > > course, It's another thing. I'm just saying.
> >
> > Let's not debate the existing API. Its design predates both of our
> > contributions.
> 
> Yep.
> 
> >
> > > I wonder if it makes sense to you :) ?
> >
> > I don't see a strong use case. I know we're on v5 while I reopen that
> > point, sorry.
> 
> That's all right. No worries.
> 
> >
> > It seems simpler to me to not read spurious fields that are not
> > requested, rather than to explicitly request them to be set to zero.
> >
> > Adding more flags is not free. An extra option adds mental load for
> > casual users of this alread complex API. This may certainly sound
> > hypocritical coming from me, as I added my fair share. But I hope
> > their functionality outweighs that cost (well.. in at least one case
> > it was an ugly fix for a bad first attempt.. OPT_ID).
> 
> I understand what you meant here. But I'm lost...
> 
> For some users, they might use the tsflags in apps to test whether
> they need to receive/report the rx timestamp or not, and someday they
> will notice there are unexpected timestamps that come out. As we know,
> it's more of a design consideration about whether the users can
> control it by setsockopt...
> 
> In addition to the design itself, above is the only use case I know.

Ok. I'm on the fence, but not a hard no. Evidently you see value, so
others may too.

A pendantic use case is if the caller expects other cmsg, but not
these. Then the amount of cmsg space used will depend on a third
process enabling receive timestamps. Again, can be worked around. But
admittedly is surprising behavior.

> >
> > I got to this point trying to condense the proposed documentation.
> > We can add this if you feel strongly.
> 
> If the new flag is not good for future development, we can stop it and
> then _only_ document the special case, which we both agreed about a
> week ago.
> 
> Personally, I don't want to let it go easily. But It's just me. You
> are the maintainer, so you have to make the decision. I'm totally fine
> with either way. Thanks.
> 
> I was only trying to make the feature better. At least, we both have
> tried a lot.
> 
> >
> > But then my main feedback is that the doc has to be shorter and to
> 
> It's truly very long, to be honest. I thought I needed to list the
> possible combination use cases.
> 
> > the point. Why would a user user this? No background on how we got
> > here, what they might already do accidentally.
> 
> It looks like I should remove those use cases? And then clarify the
> reason is per socket control?
> 
> I have no idea if I should continue on this.

My attempt to document, feel free to revise:

SOF_TIMESTAMPING_OPT_RX_FILTER:

Filter out spurious receive timestamps: report a receive timestamp
only if the matching timestamp generation flag is enabled.

Receive timestamps are generated early in the ingress path, before a
packet's destination socket is known. If any socket enables receive
timestamps, packets for all socket will receive timestamped packets.
Including those that request timestamp reporting with
SOF_TIMESTAMPING_SOFTWARE and/or SOF_TIMESTAMPING_RAW_HARDWARE, but
do not request receive timestamp generation. This can happen when
requesting transmit timestamps only.

Receiving spurious timestamps is generally benign. A process can
ignore the unexpected non-zero value. But it makes behavior subtly
dependent on other sockets. This flag isolates the socket for more
deterministic behavior.

Powered by blists - more mailing lists