diff mbox series

[net-next,v3,1/2] net-timestamp: filter out report when setting SOF_TIMESTAMPING_SOFTWARE

Message ID 20240830153751.86895-2-kerneljasonxing@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net-timestamp: introduce a flag to filter out rx software report | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 31 this patch: 31
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: corbet@lwn.net linux-doc@vger.kernel.org
netdev/build_clang success Errors and warnings before: 84 this patch: 84
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3243 this patch: 3243
netdev/checkpatch warning WARNING: line length of 93 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 36 this patch: 36
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-31--15-00 (tests: 714)

Commit Message

Jason Xing Aug. 30, 2024, 3:37 p.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

introduce a new flag SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER in the
receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
out rx 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.

In this way, we have two kinds of combination:
1. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
will surely allow users to get the rx timestamp report.
2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
while the skb is timestamped, it will stop reporting the rx 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 <willemdebruijn.kernel@gmail.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
1. Willem suggested this alternative way to solve the issue, so I
added his Suggested-by tag here. Thanks!
---
 Documentation/networking/timestamping.rst | 12 ++++++++++++
 include/uapi/linux/net_tstamp.h           |  3 ++-
 net/core/sock.c                           |  4 ++++
 net/ethtool/common.c                      |  1 +
 net/ipv4/tcp.c                            |  7 +++++--
 net/socket.c                              |  5 ++++-
 6 files changed, 28 insertions(+), 4 deletions(-)

Comments

Willem de Bruijn Aug. 31, 2024, 2:49 p.m. UTC | #1 
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> introduce a new flag SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER in the
> receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
> out rx 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.
> 
> In this way, we have two kinds of combination:
> 1. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
> will surely allow users to get the rx timestamp report.
> 2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
> while the skb is timestamped, it will stop reporting the rx timestamp.

The one point this commit does not explain is 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.
 
> 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.

Good find on skb_is_err_queue rather than open-coding PACKET_OUTGOING.

> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
> 1. Willem suggested this alternative way to solve the issue, so I
> added his Suggested-by tag here. Thanks!
> ---
>  Documentation/networking/timestamping.rst | 12 ++++++++++++
>  include/uapi/linux/net_tstamp.h           |  3 ++-
>  net/core/sock.c                           |  4 ++++
>  net/ethtool/common.c                      |  1 +
>  net/ipv4/tcp.c                            |  7 +++++--
>  net/socket.c                              |  5 ++++-
>  6 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 5e93cd71f99f..ef2a334d373e 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -266,6 +266,18 @@ SOF_TIMESTAMPING_OPT_TX_SWHW:
>    two separate messages will be looped to the socket's error queue,
>    each containing just one timestamp.
>  
> +SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER:
> +  Used in the receive software timestamp. Enabling the flag along with
> +  SOF_TIMESTAMPING_SOFTWARE will not report the rx timestamp to the
> +  userspace so that it can filter out the case where one process starts
> +  first which turns on netstamp_needed_key through setting generation
> +  flags like SOF_TIMESTAMPING_RX_SOFTWARE, then another one only passing
> +  SOF_TIMESTAMPING_SOFTWARE report flag could also get the rx timestamp.
> +
> +  SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER prevents the application from
> +  being influenced by others and let the application finally choose
> +  whether to report the timestamp in the receive path or not.
> +

nit, so no need to revise, but for next time:

Documentation should describe the current state, not a history of
changes (log), so wording like "finally" does not belong.
Jason Xing Aug. 31, 2024, 3 p.m. UTC | #2 
On Sat, Aug 31, 2024 at 10:50 PM 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_SOFTWARE_FILTER in the
> > receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
> > out rx 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.
> >
> > In this way, we have two kinds of combination:
> > 1. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
> > will surely allow users to get the rx timestamp report.
> > 2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
> > while the skb is timestamped, it will stop reporting the rx timestamp.
>
> The one point this commit does not explain is 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.

Yes, you're right.

>
> > 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.
>
> Good find on skb_is_err_queue rather than open-coding PACKET_OUTGOING.
>
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
>
> Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks for your review:)

>
> > ---
> > 1. Willem suggested this alternative way to solve the issue, so I
> > added his Suggested-by tag here. Thanks!
> > ---
> >  Documentation/networking/timestamping.rst | 12 ++++++++++++
> >  include/uapi/linux/net_tstamp.h           |  3 ++-
> >  net/core/sock.c                           |  4 ++++
> >  net/ethtool/common.c                      |  1 +
> >  net/ipv4/tcp.c                            |  7 +++++--
> >  net/socket.c                              |  5 ++++-
> >  6 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > index 5e93cd71f99f..ef2a334d373e 100644
> > --- a/Documentation/networking/timestamping.rst
> > +++ b/Documentation/networking/timestamping.rst
> > @@ -266,6 +266,18 @@ SOF_TIMESTAMPING_OPT_TX_SWHW:
> >    two separate messages will be looped to the socket's error queue,
> >    each containing just one timestamp.
> >
> > +SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER:
> > +  Used in the receive software timestamp. Enabling the flag along with
> > +  SOF_TIMESTAMPING_SOFTWARE will not report the rx timestamp to the
> > +  userspace so that it can filter out the case where one process starts
> > +  first which turns on netstamp_needed_key through setting generation
> > +  flags like SOF_TIMESTAMPING_RX_SOFTWARE, then another one only passing
> > +  SOF_TIMESTAMPING_SOFTWARE report flag could also get the rx timestamp.
> > +
> > +  SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER prevents the application from
> > +  being influenced by others and let the application finally choose
> > +  whether to report the timestamp in the receive path or not.
> > +
>
> nit, so no need to revise, but for next time:
>
> Documentation should describe the current state, not a history of
> changes (log), so wording like "finally" does not belong.

I learned it, and will bear it in my mind:)

Thanks,
Jason
Jakub Kicinski Sept. 3, 2024, 7:19 p.m. UTC | #3 
On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> +	if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> +	    val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> +		return -EINVAL;


> -		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> +		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +		    (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> +		     !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
>  			has_timestamping = true;
>  		else
>  			tss->ts[0] = (struct timespec64) {0};
>  	}

>  	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_SOFTWARE_FILTER))) &&

Willem, do you prefer to keep the:

	tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)

conditions?IIUC we prevent both from being set at once. So 

	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)

is sufficient (and, subjectively, more intuitive).

Question #2 -- why are we only doing this for SW stamps?
HW stamps for TCP are also all or nothing.
Willem de Bruijn Sept. 3, 2024, 10:13 p.m. UTC | #4 
Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > +	if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > +	    val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > +		return -EINVAL;
> 
> 
> > -		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > +		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +		    (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > +		     !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> >  			has_timestamping = true;
> >  		else
> >  			tss->ts[0] = (struct timespec64) {0};
> >  	}
> 
> >  	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_SOFTWARE_FILTER))) &&
> 
> Willem, do you prefer to keep the:
> 
> 	tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> 	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> 
> conditions?IIUC we prevent both from being set at once. So 
> 
> 	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> 
> is sufficient (and, subjectively, more intuitive).

Good point. Yes, let's definitely simplify.

> Question #2 -- why are we only doing this for SW stamps?
> HW stamps for TCP are also all or nothing.

Fair. Else we'll inevitably add a
SOF_TIMESTAMPING_OPT_RX_HARDWARE_FILTER at some point.

There probably is no real use to filter one, but not the other.

So SOF_TIMESTAMPING_OPT_RX_FILTER then, and also apply
to the branch below:

        if (shhwtstamps &&
            (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
            !skb_is_swtx_tstamp(skb, false_tstamp)) {

and same for tcp_recv_timestamp.
Jason Xing Sept. 3, 2024, 11:29 p.m. UTC | #5 
On Wed, Sep 4, 2024 at 6:13 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jakub Kicinski wrote:
> > On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > > +   if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > > +       val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > +           return -EINVAL;
> >
> >
> > > -           if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > +           if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > +               (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > +                !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> > >                     has_timestamping = true;
> > >             else
> > >                     tss->ts[0] = (struct timespec64) {0};
> > >     }
> >
> > >     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_SOFTWARE_FILTER))) &&
> >
> > Willem, do you prefer to keep the:
> >
> >       tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> >
> > conditions?IIUC we prevent both from being set at once. So
> >
> >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> >
> > is sufficient (and, subjectively, more intuitive).
>
> Good point. Yes, let's definitely simplify.

Will do it.

>
> > Question #2 -- why are we only doing this for SW stamps?
> > HW stamps for TCP are also all or nothing.
>
> Fair. Else we'll inevitably add a
> SOF_TIMESTAMPING_OPT_RX_HARDWARE_FILTER at some point.
>
> There probably is no real use to filter one, but not the other.
>
> So SOF_TIMESTAMPING_OPT_RX_FILTER then, and also apply
> to the branch below:
>
>         if (shhwtstamps &&
>             (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>             !skb_is_swtx_tstamp(skb, false_tstamp)) {
>
> and same for tcp_recv_timestamp.

IIUC, I'm going to replace it with SOF_TIMESTAMPING_OPT_RX_FILTER and
update the test statements accordingly.

Thanks,
Jason
Jason Xing Sept. 4, 2024, 9:14 a.m. UTC | #6 
On Wed, Sep 4, 2024 at 6:13 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jakub Kicinski wrote:
> > On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > > +   if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > > +       val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > +           return -EINVAL;
> >
> >
> > > -           if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > +           if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > +               (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > +                !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> > >                     has_timestamping = true;
> > >             else
> > >                     tss->ts[0] = (struct timespec64) {0};
> > >     }
> >
> > >     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_SOFTWARE_FILTER))) &&
> >
> > Willem, do you prefer to keep the:
> >
> >       tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> >
> > conditions?IIUC we prevent both from being set at once. So
> >
> >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> >
> > is sufficient (and, subjectively, more intuitive).
>
> Good point. Yes, let's definitely simplify.
>
> > Question #2 -- why are we only doing this for SW stamps?
> > HW stamps for TCP are also all or nothing.
>
> Fair. Else we'll inevitably add a
> SOF_TIMESTAMPING_OPT_RX_HARDWARE_FILTER at some point.
>
> There probably is no real use to filter one, but not the other.
>
> So SOF_TIMESTAMPING_OPT_RX_FILTER then, and also apply
> to the branch below:
>
>         if (shhwtstamps &&
>             (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>             !skb_is_swtx_tstamp(skb, false_tstamp)) {
>
> and same for tcp_recv_timestamp.

When I'm looking at this part, I noticed that RAW_HARDWARE is actually
a tx report flag instead of rx, please also see the kdoc you wrote a
long time ago:

SOF_TIMESTAMPING_RAW_HARDWARE:
  Report hardware timestamps as generated by
  SOF_TIMESTAMPING_TX_HARDWARE when available.

If so, OPT_RX_FILTER doesn't fit for the name of tx timestamp.

I wonder if I can only revise the series with the code simplified as
Jakub suggested and then repost it? I think we need to choose a new
name for this tx hardware report case, like
SOF_TIMESTAMPING_OPT_TX_HARDWARE_FILTER?

Since it belongs to the tx path, can I put it into another series or a
new patch in the current series where I will explicitly explain why we
also need to introduce this new flag?

Thanks,
Jason
Willem de Bruijn Sept. 4, 2024, 8:25 p.m. UTC | #7 
Jason Xing wrote:
> On Wed, Sep 4, 2024 at 6:13 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jakub Kicinski wrote:
> > > On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > > > +   if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > > > +       val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > > +           return -EINVAL;
> > >
> > >
> > > > -           if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > +           if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > +               (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > > +                !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> > > >                     has_timestamping = true;
> > > >             else
> > > >                     tss->ts[0] = (struct timespec64) {0};
> > > >     }
> > >
> > > >     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_SOFTWARE_FILTER))) &&
> > >
> > > Willem, do you prefer to keep the:
> > >
> > >       tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > >
> > > conditions?IIUC we prevent both from being set at once. So
> > >
> > >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > >
> > > is sufficient (and, subjectively, more intuitive).
> >
> > Good point. Yes, let's definitely simplify.
> >
> > > Question #2 -- why are we only doing this for SW stamps?
> > > HW stamps for TCP are also all or nothing.
> >
> > Fair. Else we'll inevitably add a
> > SOF_TIMESTAMPING_OPT_RX_HARDWARE_FILTER at some point.
> >
> > There probably is no real use to filter one, but not the other.
> >
> > So SOF_TIMESTAMPING_OPT_RX_FILTER then, and also apply
> > to the branch below:
> >
> >         if (shhwtstamps &&
> >             (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> >             !skb_is_swtx_tstamp(skb, false_tstamp)) {
> >
> > and same for tcp_recv_timestamp.
> 
> When I'm looking at this part, I noticed that RAW_HARDWARE is actually
> a tx report flag instead of rx, please also see the kdoc you wrote a
> long time ago:
> 
> SOF_TIMESTAMPING_RAW_HARDWARE:
>   Report hardware timestamps as generated by
>   SOF_TIMESTAMPING_TX_HARDWARE when available.

Right, this is analogous to the software part that you modify:

        if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
            ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
                empty = 0;

The idea is to also add for hardware timestamps your suggested
condition that the socket also sets the timestamp generation flag
SOF_TIMESTAMPING_RX_HARDWARE or that the new OPT_RX_FILTER flag
is not set.


> If so, OPT_RX_FILTER doesn't fit for the name of tx timestamp.
> 
> I wonder if I can only revise the series with the code simplified as
> Jakub suggested and then repost it? I think we need to choose a new
> name for this tx hardware report case, like
> SOF_TIMESTAMPING_OPT_TX_HARDWARE_FILTER?
> 
> Since it belongs to the tx path, can I put it into another series or a
> new patch in the current series where I will explicitly explain why we
> also need to introduce this new flag?

I think the confusion here comes from that comment that
SOF_TIMESTAMPING_RAW_HARDWARE only reports
SOF_TIMESTAMPING_TX_HARDWARE generated timestamps. This statement is
incorrect and should be revised. It also reports
SOF_TIMESTAMPING_RX_HARDWARE.

Unless I'm missing something. But I think the author of that statement
is the one who made the mistake. Who is.. also me.
Jason Xing Sept. 4, 2024, 9:38 p.m. UTC | #8 
On Thu, Sep 5, 2024 at 4:25 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Wed, Sep 4, 2024 at 6:13 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jakub Kicinski wrote:
> > > > On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > > > > +   if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > > > > +       val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > > > +           return -EINVAL;
> > > >
> > > >
> > > > > -           if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > > +           if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > > +               (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > > > +                !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> > > > >                     has_timestamping = true;
> > > > >             else
> > > > >                     tss->ts[0] = (struct timespec64) {0};
> > > > >     }
> > > >
> > > > >     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_SOFTWARE_FILTER))) &&
> > > >
> > > > Willem, do you prefer to keep the:
> > > >
> > > >       tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > >
> > > > conditions?IIUC we prevent both from being set at once. So
> > > >
> > > >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > >
> > > > is sufficient (and, subjectively, more intuitive).
> > >
> > > Good point. Yes, let's definitely simplify.
> > >
> > > > Question #2 -- why are we only doing this for SW stamps?
> > > > HW stamps for TCP are also all or nothing.
> > >
> > > Fair. Else we'll inevitably add a
> > > SOF_TIMESTAMPING_OPT_RX_HARDWARE_FILTER at some point.
> > >
> > > There probably is no real use to filter one, but not the other.
> > >
> > > So SOF_TIMESTAMPING_OPT_RX_FILTER then, and also apply
> > > to the branch below:
> > >
> > >         if (shhwtstamps &&
> > >             (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > >             !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > >
> > > and same for tcp_recv_timestamp.
> >
> > When I'm looking at this part, I noticed that RAW_HARDWARE is actually
> > a tx report flag instead of rx, please also see the kdoc you wrote a
> > long time ago:
> >
> > SOF_TIMESTAMPING_RAW_HARDWARE:
> >   Report hardware timestamps as generated by
> >   SOF_TIMESTAMPING_TX_HARDWARE when available.
>
> Right, this is analogous to the software part that you modify:
>
>         if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
>             ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
>                 empty = 0;
>
> The idea is to also add for hardware timestamps your suggested
> condition that the socket also sets the timestamp generation flag
> SOF_TIMESTAMPING_RX_HARDWARE or that the new OPT_RX_FILTER flag
> is not set.
>
>
> > If so, OPT_RX_FILTER doesn't fit for the name of tx timestamp.
> >
> > I wonder if I can only revise the series with the code simplified as
> > Jakub suggested and then repost it? I think we need to choose a new
> > name for this tx hardware report case, like
> > SOF_TIMESTAMPING_OPT_TX_HARDWARE_FILTER?
> >
> > Since it belongs to the tx path, can I put it into another series or a
> > new patch in the current series where I will explicitly explain why we
> > also need to introduce this new flag?
>
> I think the confusion here comes from that comment that
> SOF_TIMESTAMPING_RAW_HARDWARE only reports
> SOF_TIMESTAMPING_TX_HARDWARE generated timestamps. This statement is
> incorrect and should be revised. It also reports
> SOF_TIMESTAMPING_RX_HARDWARE.
>
> Unless I'm missing something. But I think the author of that statement
> is the one who made the mistake. Who is.. also me.

That's all right :)

Then, the OPT_RX_FILTER is fine under this circumstance.

Let me also revise the documentation first in the series and put it
into a separate patch because the rule of writing the patch is to keep
one patch doing only one thing at one time.

Similar action will be done to the "rx hardware" part because I think
mixing the long description for both of them (software & hardware) can
be a little bit messy to readers.

Let me try a better organization of the series: keep each patch clear,
small, atomic and easy to review.

Thanks for your help!
diff mbox series

Patch

diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
index 5e93cd71f99f..ef2a334d373e 100644
--- a/Documentation/networking/timestamping.rst
+++ b/Documentation/networking/timestamping.rst
@@ -266,6 +266,18 @@  SOF_TIMESTAMPING_OPT_TX_SWHW:
   two separate messages will be looped to the socket's error queue,
   each containing just one timestamp.
 
+SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER:
+  Used in the receive software timestamp. Enabling the flag along with
+  SOF_TIMESTAMPING_SOFTWARE will not report the rx timestamp to the
+  userspace so that it can filter out the case where one process starts
+  first which turns on netstamp_needed_key through setting generation
+  flags like SOF_TIMESTAMPING_RX_SOFTWARE, then another one only passing
+  SOF_TIMESTAMPING_SOFTWARE report flag could also get the rx timestamp.
+
+  SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER prevents the application from
+  being influenced by others and let the application finally choose
+  whether to report the timestamp in the receive path or not.
+
 New applications are encouraged to pass SOF_TIMESTAMPING_OPT_ID to
 disambiguate timestamps and SOF_TIMESTAMPING_OPT_TSONLY to operate
 regardless of the setting of sysctl net.core.tstamp_allow_data.
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index a2c66b3d7f0f..0042e91fa213 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -32,8 +32,9 @@  enum {
 	SOF_TIMESTAMPING_OPT_TX_SWHW = (1<<14),
 	SOF_TIMESTAMPING_BIND_PHC = (1 << 15),
 	SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16),
+	SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER = (1 << 17),
 
-	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_ID_TCP,
+	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER,
 	SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
 				 SOF_TIMESTAMPING_LAST
 };
diff --git a/net/core/sock.c b/net/core/sock.c
index 468b1239606c..c4488f6a3ce8 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -908,6 +908,10 @@  int sock_set_timestamping(struct sock *sk, int optname,
 	    !(val & SOF_TIMESTAMPING_OPT_ID))
 		return -EINVAL;
 
+	if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
+	    val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
+		return -EINVAL;
+
 	if (val & SOF_TIMESTAMPING_OPT_ID &&
 	    !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)) {
 		if (sk_is_tcp(sk)) {
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 7257ae272296..6fde55a904b0 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -430,6 +430,7 @@  const char sof_timestamping_names[][ETH_GSTRING_LEN] = {
 	[const_ilog2(SOF_TIMESTAMPING_OPT_TX_SWHW)]  = "option-tx-swhw",
 	[const_ilog2(SOF_TIMESTAMPING_BIND_PHC)]     = "bind-phc",
 	[const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)]   = "option-id-tcp",
+	[const_ilog2(SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)] = "option-rx-software-filter",
 };
 static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT);
 
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 8514257f4ecd..863cc6b8a208 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2235,6 +2235,7 @@  void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
 			struct scm_timestamping_internal *tss)
 {
 	int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
+	u32 tsflags = READ_ONCE(sk->sk_tsflags);
 	bool has_timestamping = false;
 
 	if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) {
@@ -2274,14 +2275,16 @@  void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
 			}
 		}
 
-		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
+		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
+		    (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
+		     !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
 			has_timestamping = true;
 		else
 			tss->ts[0] = (struct timespec64) {0};
 	}
 
 	if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) {
-		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_RAW_HARDWARE)
+		if (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)
 			has_timestamping = true;
 		else
 			tss->ts[2] = (struct timespec64) {0};
diff --git a/net/socket.c b/net/socket.c
index fcbdd5bc47ac..5ede4146198c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -946,7 +946,10 @@  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_SOFTWARE_FILTER))) &&
 	    ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
 		empty = 0;
 	if (shhwtstamps &&