Message-ID: <CAL+tcoD4NQSYcdFOpOxXQnfPwjsck5bzZZXzAbOGMKFHvp_d6w@mail.gmail.com>
Date: Mon, 9 Sep 2024 07:29:14 +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, 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

Hello Willem,

On Mon, Sep 9, 2024 at 3:41 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> 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.

Willem, thanks so much for your effort!!! I learn a lot these days.

I'm going to completely replace it with your description.

Thanks,
Jason

Powered by blists - more mailing lists