Message-ID: <CAL+tcoC2TajcYLewWrhpF7KL75XyMiW_G0RV+58=fbMEH8rf=g@mail.gmail.com>
Date: Sat, 7 Sep 2024 11:11:44 +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

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.

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

Thanks,
Jason

Powered by blists - more mailing lists