mbox series

[net-next,v5,0/2] net-timestamp: introduce a flag to filter out rx software and hardware report

Message ID 20240906095640.77533-1-kerneljasonxing@gmail.com
Headers show
Series net-timestamp: introduce a flag to filter out rx software and hardware report | expand

Message

Jason Xing Sept. 6, 2024, 9:56 a.m. UTC
From: Jason Xing <kernelxing@tencent.com>

When one socket is set SOF_TIMESTAMPING_RX_SOFTWARE which means the
whole system turns on the netstamp_needed_key button, other sockets
that only have SOF_TIMESTAMPING_SOFTWARE will be affected and then
print the rx timestamp information even without setting
SOF_TIMESTAMPING_RX_SOFTWARE generation flag.

How to solve it without breaking users?
We introduce a new flag named SOF_TIMESTAMPING_OPT_RX_FILTER. Using
it together with SOF_TIMESTAMPING_SOFTWARE can stop reporting the
rx software timestamp.

Similarly, we also filter out the hardware case where one process
enables the rx hardware generation flag, then another process only
passing SOF_TIMESTAMPING_RAW_HARDWARE gets the timestamp. So we can set
both SOF_TIMESTAMPING_RAW_HARDWARE and SOF_TIMESTAMPING_OPT_RX_FILTER
to stop reporting rx hardware timestamp after this patch applied.

v5
Link: https://lore.kernel.org/all/20240905071738.3725-1-kerneljasonxing@gmail.com/
1. squash the hardware case patch into this one (Willem)
2. update corresponding commit message and doc (Willem)
3. remove the limitation in sock_set_timestamping() and restore the
simplification branches. (Willem)
4. add missing type and another test in selftests

v4
Link: https://lore.kernel.org/all/20240830153751.86895-1-kerneljasonxing@gmail.com/
1. revise the doc and commit message (Willem)
2. add patch [2/4] to make the doc right (Willem)
3. add patch [3/4] to cover the hardware use (Willem)
4. add testcase for hardware use.
Note: the reason why I split into 4 patches is try to make each commit
clean, atomic, easy to review.

v3
Link: https://lore.kernel.org/all/20240828160145.68805-1-kerneljasonxing@gmail.com/
1. introduce a new flag to avoid application breakage, suggested by
Willem.
2. add it into the selftests.

v2
Link: https://lore.kernel.org/all/20240825152440.93054-1-kerneljasonxing@gmail.com/
Discussed with Willem
1. update the documentation accordingly
2. add more comments in each patch
3. remove the previous test statements in __sock_recv_timestamp()

Jason Xing (2):
  net-timestamp: introduce SOF_TIMESTAMPING_OPT_RX_FILTER flag
  net-timestamp: add selftests for SOF_TIMESTAMPING_OPT_RX_FILTER

 Documentation/networking/timestamping.rst | 27 +++++++++++++++++++++++
 include/uapi/linux/net_tstamp.h           |  3 ++-
 net/ethtool/common.c                      |  1 +
 net/ipv4/tcp.c                            |  9 ++++++--
 net/socket.c                              | 10 +++++++--
 tools/testing/selftests/net/rxtimestamp.c | 18 +++++++++++++++
 6 files changed, 63 insertions(+), 5 deletions(-)

Comments

Willem de Bruijn Sept. 6, 2024, 11:24 p.m. UTC | #1
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.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@google.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.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.

Otherwise implementation looks fine, only the tiniest nit.

> @@ -946,11 +946,17 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>  
>  	memset(&tss, 0, sizeof(tss));
>  	tsflags = READ_ONCE(sk->sk_tsflags);
> -	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> +	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +	     (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> +	     skb_is_err_queue(skb) ||
> +	     !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&

Nit: these statements should all align on the inner brace, so indent
by one character.

>  	    ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
>  		empty = 0;
>  	if (shhwtstamps &&
> -	    (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> +	    (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE &&
> +	    (tsflags & SOF_TIMESTAMPING_RX_HARDWARE ||
> +	    skb_is_err_queue(skb) ||
> +	    !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
>  	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
>  		if_index = 0;
>  		if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
> -- 
> 2.37.3
>
Jason Xing Sept. 7, 2024, 1:23 a.m. UTC | #2
On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.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@google.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.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.

I wonder if it makes sense to you :) ?

>
> Otherwise implementation looks fine, only the tiniest nit.
>
> > @@ -946,11 +946,17 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> >
> >       memset(&tss, 0, sizeof(tss));
> >       tsflags = READ_ONCE(sk->sk_tsflags);
> > -     if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> > +     if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +          (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > +          skb_is_err_queue(skb) ||
> > +          !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
>
> Nit: these statements should all align on the inner brace, so indent
> by one character.

I'm not that sure about the format, please help me to review here:

@@ -946,11 +946,17 @@ void __sock_recv_timestamp(struct msghdr *msg,
struct sock *sk,

        memset(&tss, 0, sizeof(tss));
        tsflags = READ_ONCE(sk->sk_tsflags);
-       if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
+       if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
+            (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
+             skb_is_err_queue(skb) ||
+             !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
            ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
                empty = 0;
        if (shhwtstamps &&
-           (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
+           (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE &&
+            (tsflags & SOF_TIMESTAMPING_RX_HARDWARE ||
+             skb_is_err_queue(skb) ||
+             !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
            !skb_is_swtx_tstamp(skb, false_tstamp)) {
                if_index = 0;
                if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)

>
> >           ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
> >               empty = 0;
> >       if (shhwtstamps &&
> > -         (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > +         (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE &&
> > +         (tsflags & SOF_TIMESTAMPING_RX_HARDWARE ||

same here and the following two statements? Should I also indent by
one char by the way?

> > +         skb_is_err_queue(skb) ||
> > +         !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
> >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> >               if_index = 0;
> >               if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
> > --
> > 2.37.3
> >
>
>
Jason Xing Sept. 7, 2024, 3:11 a.m. UTC | #3
On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.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@google.com>
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.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
Willem de Bruijn Sept. 8, 2024, 7:41 p.m. UTC | #4
Jason Xing wrote:
> On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@tencent.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@google.com>
> > > > > Signed-off-by: Jason Xing <kernelxing@tencent.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.
Jason Xing Sept. 8, 2024, 11:29 p.m. UTC | #5
Hello Willem,

On Mon, Sep 9, 2024 at 3:41 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > Jason Xing wrote:
> > > > > > From: Jason Xing <kernelxing@tencent.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@google.com>
> > > > > > Signed-off-by: Jason Xing <kernelxing@tencent.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