diff mbox series

[net-next,v2,1/2] tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

Message ID 20240828160145.68805-2-kerneljasonxing@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
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: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
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: 16 this patch: 16
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-29--15-00 (tests: 711)

Commit Message

Jason Xing Aug. 28, 2024, 4:01 p.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

Normally, if we want to record and print the rx timestamp after
tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
through running rxtimestamp binary in selftests (see testcase 7).

However, there is one particular case that fails the selftests with
"./rxtimestamp: Expected swtstamp to not be set." error printing in
testcase 6.

How does it happen? When we keep running a thread starting a socket
and set SOF_TIMESTAMPING_RX_SOFTWARE option first, then running
./rxtimestamp, it will fail. The reason is the former thread
switching on netstamp_needed_key that makes the feature global,
every skb going through netif_receive_skb_list_internal() function
will get a current timestamp in net_timestamp_check(). So the skb
will have timestamp regardless of whether its socket option has
SOF_TIMESTAMPING_RX_SOFTWARE or not.

After this patch, we can pass the selftest and control each socket
as we want when using rx timestamp feature.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/ipv4/tcp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Willem de Bruijn Aug. 29, 2024, 2:16 p.m. UTC | #1 
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Normally, if we want to record and print the rx timestamp after
> tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
> and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
> through running rxtimestamp binary in selftests (see testcase 7).
> 
> However, there is one particular case that fails the selftests with
> "./rxtimestamp: Expected swtstamp to not be set." error printing in
> testcase 6.
> 
> How does it happen? When we keep running a thread starting a socket
> and set SOF_TIMESTAMPING_RX_SOFTWARE option first, then running
> ./rxtimestamp, it will fail. The reason is the former thread
> switching on netstamp_needed_key that makes the feature global,
> every skb going through netif_receive_skb_list_internal() function
> will get a current timestamp in net_timestamp_check(). So the skb
> will have timestamp regardless of whether its socket option has
> SOF_TIMESTAMPING_RX_SOFTWARE or not.
> 
> After this patch, we can pass the selftest and control each socket
> as we want when using rx timestamp feature.
> 
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/ipv4/tcp.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 8514257f4ecd..5e88c765b9a1 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,20 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
>  			}
>  		}
>  
> -		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> +		/* We have to use the generation flag here to test if we
> +		 * allow the corresponding application to receive the rx
> +		 * timestamp. Only using report flag does not hold for
> +		 * receive timestamping case.
> +		 */

Nit: what does "does not hold" mean here? I don't think a casual reader
will be able to parse this comment and understand it.

Perhaps something along the lines of

"Test both reporting and generation flag, to filter out false
positives where the process asked only for tx software timestamps and
another process enabled receive software timestamp generation."

> +		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +		    tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
>  			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};
> -- 
> 2.37.3
>
Jason Xing Aug. 29, 2024, 3:34 p.m. UTC | #2 
On Thu, Aug 29, 2024 at 10:16 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Normally, if we want to record and print the rx timestamp after
> > tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
> > and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
> > through running rxtimestamp binary in selftests (see testcase 7).
> >
> > However, there is one particular case that fails the selftests with
> > "./rxtimestamp: Expected swtstamp to not be set." error printing in
> > testcase 6.
> >
> > How does it happen? When we keep running a thread starting a socket
> > and set SOF_TIMESTAMPING_RX_SOFTWARE option first, then running
> > ./rxtimestamp, it will fail. The reason is the former thread
> > switching on netstamp_needed_key that makes the feature global,
> > every skb going through netif_receive_skb_list_internal() function
> > will get a current timestamp in net_timestamp_check(). So the skb
> > will have timestamp regardless of whether its socket option has
> > SOF_TIMESTAMPING_RX_SOFTWARE or not.
> >
> > After this patch, we can pass the selftest and control each socket
> > as we want when using rx timestamp feature.
> >
> > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  net/ipv4/tcp.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 8514257f4ecd..5e88c765b9a1 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,20 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       }
> >               }
> >
> > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > +             /* We have to use the generation flag here to test if we
> > +              * allow the corresponding application to receive the rx
> > +              * timestamp. Only using report flag does not hold for
> > +              * receive timestamping case.
> > +              */
>
> Nit: what does "does not hold" mean here? I don't think a casual reader
> will be able to parse this comment and understand it.

“hold for” can be a fixed collocation, which means "be suitable for"?
I'm not that sure. I was trying to say "only using the report flag
cannot meet our needs" something like this.

>
> Perhaps something along the lines of
>
> "Test both reporting and generation flag, to filter out false
> positives where the process asked only for tx software timestamps and
> another process enabled receive software timestamp generation."

Thanks, it's much better than mine. I will use it.

Thanks,
Jason

>
> > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> >                       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};
> > --
> > 2.37.3
> >
>
>
diff mbox series

Patch

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 8514257f4ecd..5e88c765b9a1 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,20 @@  void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
 			}
 		}
 
-		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
+		/* We have to use the generation flag here to test if we
+		 * allow the corresponding application to receive the rx
+		 * timestamp. Only using report flag does not hold for
+		 * receive timestamping case.
+		 */
+		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
+		    tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
 			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};