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

Commit Message

Jason Xing Aug. 25, 2024, 3:24 p.m. UTC

From: Jason Xing <[e-mail]>

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

Signed-off-by: Jason Xing <[e-mail]>
---
 net/ipv4/tcp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Willem de Bruijn Aug. 26, 2024, 1:24 p.m. UTC | #1

Jason Xing wrote:
> From: Jason Xing <[e-mail]>
> 
> 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_HARDWARE 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.
> 
> Signed-off-by: Jason Xing <[e-mail]>
> ---
>  net/ipv4/tcp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
>  			}
>  		}
>  
> -		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> +		/* skb may contain timestamp because another socket
> +		 * turned on netstamp_needed_key which allows generate
> +		 * the timestamp. So we need to check the current socket.
> +		 */
> +		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +		    tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
>  			has_timestamping = true;
>  		else
>  			tss->ts[0] = (struct timespec64) {0};
>  	}

The current behavior is as described in
Documentation/networking/timestamping.rst:

"The socket option configures timestamp generation for individual
sk_buffs (1.3.1), timestamp reporting to the socket's error
queue (1.3.2)"

SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.

This patch changes that clearly defined behavior.

On Tx the separation between generation and reporting has value, as it
allows setting the generation on a per packet basis with SCM_TSTAMP_*.

On Rx it is more subtle, but the two are still tested at different
points in the path, and can be updated by setsockopt in between a
packet arrival and a recvmsg().

The interaction between sockets on software timestamping is a
longstanding issue. I don't think there is any urgency to change this
now. This proposed change makes the API less consistent, and may
also affect applications that depend on the current behavior.

Jason Xing Aug. 26, 2024, 1:40 p.m. UTC | #2

Hello Willem,

On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
<[e-mail]> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <[e-mail]>
> >
> > 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_HARDWARE 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.
> >
> > Signed-off-by: Jason Xing <[e-mail]>
> > ---
> >  net/ipv4/tcp.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       }
> >               }
> >
> > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > +             /* skb may contain timestamp because another socket
> > +              * turned on netstamp_needed_key which allows generate
> > +              * the timestamp. So we need to check the current socket.
> > +              */
> > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> >                       has_timestamping = true;
> >               else
> >                       tss->ts[0] = (struct timespec64) {0};
> >       }
>
> The current behavior is as described in
> Documentation/networking/timestamping.rst:
>
> "The socket option configures timestamp generation for individual
> sk_buffs (1.3.1), timestamp reporting to the socket's error
> queue (1.3.2)"
>
> SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
> SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.

Thanks for your review.

Yes, it's true.

>
> This patch changes that clearly defined behavior.

Why? I don't get it. Please see those testcase in
tools/testing/selftests/net/rxtimestamp.c.

>
> On Tx the separation between generation and reporting has value, as it
> allows setting the generation on a per packet basis with SCM_TSTAMP_*.

I didn't break the logic on the tx path. tcp_recv_timestamp() is only
related to the rx path.

Regarding the tx path, I carefully take care of this logic in
patch[2/2], so now the series only handles the issue happening in the
rx path.

>
> On Rx it is more subtle, but the two are still tested at different
> points in the path, and can be updated by setsockopt in between a
> packet arrival and a recvmsg().
>
> The interaction between sockets on software timestamping is a
> longstanding issue. I don't think there is any urgency to change this

Oh, now I see.

> now. This proposed change makes the API less consistent, and may
> also affect applications that depend on the current behavior.
>

Maybe. But, it's not the original design which we expect, right?

I can make sure this series can fix the issue. This series is trying
to ask users to use/set both flags to receive an expected timestamp.
The purpose of using setsockopt is to control the socket itself
insteading of interfering with others.

About the breakage issue, let me assume, if the user only sets the
SOF_TIMESTAMPING_SOFTWARE flag, he cannot expect the socket will
receive a timestamp, right? So what might happen if we fix the issue?
I think the logics in the rxtimestamp.c selftest are very clear :)

Besides, test case 6 will fail under this circumstance.

Thanks,
Jason

Willem de Bruijn Aug. 26, 2024, 4:03 p.m. UTC | #3

Jason Xing wrote:
> Hello Willem,
> 
> On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
> <[e-mail]> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <[e-mail]>
> > >
> > > 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_HARDWARE 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.
> > >
> > > Signed-off-by: Jason Xing <[e-mail]>
> > > ---
> > >  net/ipv4/tcp.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> > >                       }
> > >               }
> > >
> > > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > +             /* skb may contain timestamp because another socket
> > > +              * turned on netstamp_needed_key which allows generate
> > > +              * the timestamp. So we need to check the current socket.
> > > +              */
> > > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> > >                       has_timestamping = true;
> > >               else
> > >                       tss->ts[0] = (struct timespec64) {0};
> > >       }
> >
> > The current behavior is as described in
> > Documentation/networking/timestamping.rst:
> >
> > "The socket option configures timestamp generation for individual
> > sk_buffs (1.3.1), timestamp reporting to the socket's error
> > queue (1.3.2)"
> >
> > SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
> > SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.
> 
> Thanks for your review.
> 
> Yes, it's true.
> 
> >
> > This patch changes that clearly defined behavior.
> 
> Why?

Because it repurposes generation flag SOF_TIMESTAMPING_RX_SOFTWARE in
timestamp reporting.

If a single flag configures both generation and reporting, why bother
with two flags at all.

> I don't get it. Please see those testcase in
> tools/testing/selftests/net/rxtimestamp.c.
> 
> >
> > On Tx the separation between generation and reporting has value, as it
> > allows setting the generation on a per packet basis with SCM_TSTAMP_*.
> 
> I didn't break the logic on the tx path. tcp_recv_timestamp() is only
> related to the rx path.
> 
> Regarding the tx path, I carefully take care of this logic in
> patch[2/2], so now the series only handles the issue happening in the
> rx path.
> 
> >
> > On Rx it is more subtle, but the two are still tested at different
> > points in the path, and can be updated by setsockopt in between a
> > packet arrival and a recvmsg().
> >
> > The interaction between sockets on software timestamping is a
> > longstanding issue. I don't think there is any urgency to change this
> 
> Oh, now I see.
> 
> > now. This proposed change makes the API less consistent, and may
> > also affect applications that depend on the current behavior.
> >
> 
> Maybe. But, it's not the original design which we expect, right?

It is. Your argument is against the current API design. This is not
a bug where behavior diverges from the intended interface. The doc is
clear on this.

The API makes a distinction between generation and reporting bits. The
shared generation early in the Rx path is a long standing known issue.

I'm not saying that the API is perfect. But it is clear in its use of
the bits. Muddling the distinction between reporting and generation
bits in one of the four cases makes the API less consistent and harder
to understand.

If you think the API as is is wrong, then at a minimum that would
require an update to timestamping.rst. But I think that medicine may
be worse than the ailment.

> I can make sure this series can fix the issue. This series is trying
> to ask users to use/set both flags to receive an expected timestamp.
> The purpose of using setsockopt is to control the socket itself
> insteading of interfering with others.
> 
> About the breakage issue, let me assume, if the user only sets the
> SOF_TIMESTAMPING_SOFTWARE flag, he cannot expect the socket will
> receive a timestamp, right? So what might happen if we fix the issue?
> I think the logics in the rxtimestamp.c selftest are very clear :)
> 
> Besides, test case 6 will fail under this circumstance

Sorry about that. My team added that test, and we expanded it over
time. Crucially, the test was added well after the SO_TIMESTAMPING
API, so it was never intended to be prescriptive.

Commit 0558c3960407 ("selftests/net: plug rxtimestamp test into
kselftest framework") actually mentions this issue:

    Also ignore failures of test case #6 by default. This case verifies
    that a receive timestamp is not reported if timestamp reporting is
    enabled for a socket, but generation is disabled. Receive timestamp
    generation has to be enabled globally, as no associated socket is
    known yet. A background process that enables rx timestamp generation
    therefore causes a false positive. Ntpd is one example that does.

    Add a "--strict" option to cause failure in the event that any test
    case fails, including test #6. This is useful for environments that
    are known to not have such background processes.

Jason Xing Aug. 26, 2024, 4:41 p.m. UTC | #4

On Tue, Aug 27, 2024 at 12:03 AM Willem de Bruijn
<[e-mail]> wrote:
>
> Jason Xing wrote:
> > Hello Willem,
> >
> > On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
> > <[e-mail]> wrote:
> > >
> > > Jason Xing wrote:
> > > > From: Jason Xing <[e-mail]>
> > > >
> > > > 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_HARDWARE 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.
> > > >
> > > > Signed-off-by: Jason Xing <[e-mail]>
> > > > ---
> > > >  net/ipv4/tcp.c | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> > > >                       }
> > > >               }
> > > >
> > > > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > +             /* skb may contain timestamp because another socket
> > > > +              * turned on netstamp_needed_key which allows generate
> > > > +              * the timestamp. So we need to check the current socket.
> > > > +              */
> > > > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> > > >                       has_timestamping = true;
> > > >               else
> > > >                       tss->ts[0] = (struct timespec64) {0};
> > > >       }
> > >
> > > The current behavior is as described in
> > > Documentation/networking/timestamping.rst:
> > >
> > > "The socket option configures timestamp generation for individual
> > > sk_buffs (1.3.1), timestamp reporting to the socket's error
> > > queue (1.3.2)"
> > >
> > > SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
> > > SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.
> >
> > Thanks for your review.
> >
> > Yes, it's true.
> >
> > >
> > > This patch changes that clearly defined behavior.
> >
> > Why?
>
> Because it repurposes generation flag SOF_TIMESTAMPING_RX_SOFTWARE in
> timestamp reporting.
>
> If a single flag configures both generation and reporting, why bother
> with two flags at all.

Thanks for your full and detailed explanation :)

I probably understand what you're saying. You think we should strictly
distinguish these two concepts "generation" and "reporting".

In my opinion, they are just concepts. We can make it clear by writing
some sentences in the Documentation.

>
> > I don't get it. Please see those testcase in
> > tools/testing/selftests/net/rxtimestamp.c.
> >
> > >
> > > On Tx the separation between generation and reporting has value, as it
> > > allows setting the generation on a per packet basis with SCM_TSTAMP_*.
> >
> > I didn't break the logic on the tx path. tcp_recv_timestamp() is only
> > related to the rx path.
> >
> > Regarding the tx path, I carefully take care of this logic in
> > patch[2/2], so now the series only handles the issue happening in the
> > rx path.
> >
> > >
> > > On Rx it is more subtle, but the two are still tested at different
> > > points in the path, and can be updated by setsockopt in between a
> > > packet arrival and a recvmsg().
> > >
> > > The interaction between sockets on software timestamping is a
> > > longstanding issue. I don't think there is any urgency to change this
> >
> > Oh, now I see.
> >
> > > now. This proposed change makes the API less consistent, and may
> > > also affect applications that depend on the current behavior.
> > >
> >
> > Maybe. But, it's not the original design which we expect, right?
>
> It is. Your argument is against the current API design. This is not
> a bug where behavior diverges from the intended interface. The doc is
> clear on this.
>
> The API makes a distinction between generation and reporting bits. The
> shared generation early in the Rx path is a long standing known issue.
>
> I'm not saying that the API is perfect. But it is clear in its use of
> the bits. Muddling the distinction between reporting and generation
> bits in one of the four cases makes the API less consistent and harder
> to understand.
>
> If you think the API as is is wrong, then at a minimum that would
> require an update to timestamping.rst. But I think that medicine may
> be worse than the ailment.

At least, I think it is against the use of setsockopt, that's the key
reason: making people confused and thinking the setsockopt is not a
per-socket fine-grained design. Don't you think it's a little bit
strange?

Besides those two concepts you mentioned, could you explain if there
are side effects that the series has and what kind of bad consequences
that the series could bring?

I tried to make it more logical and also don't want to break the
existing use behaviour of applications.

I believe that what I wrote doesn't have an impact on other cases and
perfects what should be perfected. No offense. If the series does no
harm and we keep it in the right direction, are there other reasons
stopping it getting approved, I wonder.

Thanks,
Jason

>
> > I can make sure this series can fix the issue. This series is trying
> > to ask users to use/set both flags to receive an expected timestamp.
> > The purpose of using setsockopt is to control the socket itself
> > insteading of interfering with others.
> >
> > About the breakage issue, let me assume, if the user only sets the
> > SOF_TIMESTAMPING_SOFTWARE flag, he cannot expect the socket will
> > receive a timestamp, right? So what might happen if we fix the issue?
> > I think the logics in the rxtimestamp.c selftest are very clear :)
> >
> > Besides, test case 6 will fail under this circumstance
>
> Sorry about that. My team added that test, and we expanded it over
> time. Crucially, the test was added well after the SO_TIMESTAMPING
> API, so it was never intended to be prescriptive.
>
> Commit 0558c3960407 ("selftests/net: plug rxtimestamp test into
> kselftest framework") actually mentions this issue:
>
>     Also ignore failures of test case #6 by default. This case verifies
>     that a receive timestamp is not reported if timestamp reporting is
>     enabled for a socket, but generation is disabled. Receive timestamp
>     generation has to be enabled globally, as no associated socket is
>     known yet. A background process that enables rx timestamp generation
>     therefore causes a false positive. Ntpd is one example that does.
>
>     Add a "--strict" option to cause failure in the event that any test
>     case fails, including test #6. This is useful for environments that
>     are known to not have such background processes.

Willem de Bruijn Aug. 26, 2024, 6:43 p.m. UTC | #5

Jason Xing wrote:
> On Tue, Aug 27, 2024 at 12:03 AM Willem de Bruijn
> <[e-mail]> wrote:
> >
> > Jason Xing wrote:
> > > Hello Willem,
> > >
> > > On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
> > > <[e-mail]> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > From: Jason Xing <[e-mail]>
> > > > >
> > > > > 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_HARDWARE 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.
> > > > >
> > > > > Signed-off-by: Jason Xing <[e-mail]>
> > > > > ---
> > > > >  net/ipv4/tcp.c | 10 ++++++++--
> > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > > index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> > > > >                       }
> > > > >               }
> > > > >
> > > > > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > > +             /* skb may contain timestamp because another socket
> > > > > +              * turned on netstamp_needed_key which allows generate
> > > > > +              * the timestamp. So we need to check the current socket.
> > > > > +              */
> > > > > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> > > > >                       has_timestamping = true;
> > > > >               else
> > > > >                       tss->ts[0] = (struct timespec64) {0};
> > > > >       }
> > > >
> > > > The current behavior is as described in
> > > > Documentation/networking/timestamping.rst:
> > > >
> > > > "The socket option configures timestamp generation for individual
> > > > sk_buffs (1.3.1), timestamp reporting to the socket's error
> > > > queue (1.3.2)"
> > > >
> > > > SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
> > > > SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.
> > >
> > > Thanks for your review.
> > >
> > > Yes, it's true.
> > >
> > > >
> > > > This patch changes that clearly defined behavior.
> > >
> > > Why?
> >
> > Because it repurposes generation flag SOF_TIMESTAMPING_RX_SOFTWARE in
> > timestamp reporting.
> >
> > If a single flag configures both generation and reporting, why bother
> > with two flags at all.
> 
> Thanks for your full and detailed explanation :)
> 
> I probably understand what you're saying. You think we should strictly
> distinguish these two concepts "generation" and "reporting".
> 
> In my opinion, they are just concepts. We can make it clear by writing
> some sentences in the Documentation.
> 
> >
> > > I don't get it. Please see those testcase in
> > > tools/testing/selftests/net/rxtimestamp.c.
> > >
> > > >
> > > > On Tx the separation between generation and reporting has value, as it
> > > > allows setting the generation on a per packet basis with SCM_TSTAMP_*.
> > >
> > > I didn't break the logic on the tx path. tcp_recv_timestamp() is only
> > > related to the rx path.
> > >
> > > Regarding the tx path, I carefully take care of this logic in
> > > patch[2/2], so now the series only handles the issue happening in the
> > > rx path.
> > >
> > > >
> > > > On Rx it is more subtle, but the two are still tested at different
> > > > points in the path, and can be updated by setsockopt in between a
> > > > packet arrival and a recvmsg().
> > > >
> > > > The interaction between sockets on software timestamping is a
> > > > longstanding issue. I don't think there is any urgency to change this
> > >
> > > Oh, now I see.
> > >
> > > > now. This proposed change makes the API less consistent, and may
> > > > also affect applications that depend on the current behavior.
> > > >
> > >
> > > Maybe. But, it's not the original design which we expect, right?
> >
> > It is. Your argument is against the current API design. This is not
> > a bug where behavior diverges from the intended interface. The doc is
> > clear on this.
> >
> > The API makes a distinction between generation and reporting bits. The
> > shared generation early in the Rx path is a long standing known issue.
> >
> > I'm not saying that the API is perfect. But it is clear in its use of
> > the bits. Muddling the distinction between reporting and generation
> > bits in one of the four cases makes the API less consistent and harder
> > to understand.
> >
> > If you think the API as is is wrong, then at a minimum that would
> > require an update to timestamping.rst. But I think that medicine may
> > be worse than the ailment.
> 
> At least, I think it is against the use of setsockopt, that's the key
> reason: making people confused and thinking the setsockopt is not a
> per-socket fine-grained design. Don't you think it's a little bit
> strange?

That is moot. This design was made many years ago and is now expected.

I also don't see it as a huge issue.

The effect you point out in rxtimestamp.c was known and reported in
the test commit itself.

Perhaps a more interesting argument would be
SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE. But
spurious software rx timestamps are trivially ignored.

> Besides those two concepts you mentioned, could you explain if there
> are side effects that the series has and what kind of bad consequences
> that the series could bring?

It doesn't do the same for hardware timestamping, creating
inconsistency.

Changing established interfaces always risks production issues. In
this case, I'm not convinced that the benefit outweighs this risk.

> I tried to make it more logical and also don't want to break the
> existing use behaviour of applications.
> 
> I believe that what I wrote doesn't have an impact on other cases and
> perfects what should be perfected. No offense. If the series does no
> harm and we keep it in the right direction, are there other reasons
> stopping it getting approved, I wonder.
> 
> Thanks,
> Jason
> 
> >
> > > I can make sure this series can fix the issue. This series is trying
> > > to ask users to use/set both flags to receive an expected timestamp.
> > > The purpose of using setsockopt is to control the socket itself
> > > insteading of interfering with others.
> > >
> > > About the breakage issue, let me assume, if the user only sets the
> > > SOF_TIMESTAMPING_SOFTWARE flag, he cannot expect the socket will
> > > receive a timestamp, right? So what might happen if we fix the issue?
> > > I think the logics in the rxtimestamp.c selftest are very clear :)
> > >
> > > Besides, test case 6 will fail under this circumstance
> >
> > Sorry about that. My team added that test, and we expanded it over
> > time. Crucially, the test was added well after the SO_TIMESTAMPING
> > API, so it was never intended to be prescriptive.
> >
> > Commit 0558c3960407 ("selftests/net: plug rxtimestamp test into
> > kselftest framework") actually mentions this issue:
> >
> >     Also ignore failures of test case #6 by default. This case verifies
> >     that a receive timestamp is not reported if timestamp reporting is
> >     enabled for a socket, but generation is disabled. Receive timestamp
> >     generation has to be enabled globally, as no associated socket is
> >     known yet. A background process that enables rx timestamp generation
> >     therefore causes a false positive. Ntpd is one example that does.
> >
> >     Add a "--strict" option to cause failure in the event that any test
> >     case fails, including test #6. This is useful for environments that
> >     are known to not have such background processes.

Jason Xing Aug. 26, 2024, 11:42 p.m. UTC | #6

On Tue, Aug 27, 2024 at 2:43 AM Willem de Bruijn
<[e-mail]> wrote:
>
> Jason Xing wrote:
> > On Tue, Aug 27, 2024 at 12:03 AM Willem de Bruijn
> > <[e-mail]> wrote:
> > >
> > > Jason Xing wrote:
> > > > Hello Willem,
> > > >
> > > > On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
> > > > <[e-mail]> wrote:
> > > > >
> > > > > Jason Xing wrote:
> > > > > > From: Jason Xing <[e-mail]>
> > > > > >
> > > > > > 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_HARDWARE 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.
> > > > > >
> > > > > > Signed-off-by: Jason Xing <[e-mail]>
> > > > > > ---
> > > > > >  net/ipv4/tcp.c | 10 ++++++++--
> > > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > > > index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> > > > > >                       }
> > > > > >               }
> > > > > >
> > > > > > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > > > +             /* skb may contain timestamp because another socket
> > > > > > +              * turned on netstamp_needed_key which allows generate
> > > > > > +              * the timestamp. So we need to check the current socket.
> > > > > > +              */
> > > > > > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > > > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> > > > > >                       has_timestamping = true;
> > > > > >               else
> > > > > >                       tss->ts[0] = (struct timespec64) {0};
> > > > > >       }
> > > > >
> > > > > The current behavior is as described in
> > > > > Documentation/networking/timestamping.rst:
> > > > >
> > > > > "The socket option configures timestamp generation for individual
> > > > > sk_buffs (1.3.1), timestamp reporting to the socket's error
> > > > > queue (1.3.2)"
> > > > >
> > > > > SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
> > > > > SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.
> > > >
> > > > Thanks for your review.
> > > >
> > > > Yes, it's true.
> > > >
> > > > >
> > > > > This patch changes that clearly defined behavior.
> > > >
> > > > Why?
> > >
> > > Because it repurposes generation flag SOF_TIMESTAMPING_RX_SOFTWARE in
> > > timestamp reporting.
> > >
> > > If a single flag configures both generation and reporting, why bother
> > > with two flags at all.
> >
> > Thanks for your full and detailed explanation :)
> >
> > I probably understand what you're saying. You think we should strictly
> > distinguish these two concepts "generation" and "reporting".
> >
> > In my opinion, they are just concepts. We can make it clear by writing
> > some sentences in the Documentation.
> >
> > >
> > > > I don't get it. Please see those testcase in
> > > > tools/testing/selftests/net/rxtimestamp.c.
> > > >
> > > > >
> > > > > On Tx the separation between generation and reporting has value, as it
> > > > > allows setting the generation on a per packet basis with SCM_TSTAMP_*.
> > > >
> > > > I didn't break the logic on the tx path. tcp_recv_timestamp() is only
> > > > related to the rx path.
> > > >
> > > > Regarding the tx path, I carefully take care of this logic in
> > > > patch[2/2], so now the series only handles the issue happening in the
> > > > rx path.
> > > >
> > > > >
> > > > > On Rx it is more subtle, but the two are still tested at different
> > > > > points in the path, and can be updated by setsockopt in between a
> > > > > packet arrival and a recvmsg().
> > > > >
> > > > > The interaction between sockets on software timestamping is a
> > > > > longstanding issue. I don't think there is any urgency to change this
> > > >
> > > > Oh, now I see.
> > > >
> > > > > now. This proposed change makes the API less consistent, and may
> > > > > also affect applications that depend on the current behavior.
> > > > >
> > > >
> > > > Maybe. But, it's not the original design which we expect, right?
> > >
> > > It is. Your argument is against the current API design. This is not
> > > a bug where behavior diverges from the intended interface. The doc is
> > > clear on this.
> > >
> > > The API makes a distinction between generation and reporting bits. The
> > > shared generation early in the Rx path is a long standing known issue.
> > >
> > > I'm not saying that the API is perfect. But it is clear in its use of
> > > the bits. Muddling the distinction between reporting and generation
> > > bits in one of the four cases makes the API less consistent and harder
> > > to understand.
> > >
> > > If you think the API as is is wrong, then at a minimum that would
> > > require an update to timestamping.rst. But I think that medicine may
> > > be worse than the ailment.
> >
> > At least, I think it is against the use of setsockopt, that's the key
> > reason: making people confused and thinking the setsockopt is not a
> > per-socket fine-grained design. Don't you think it's a little bit
> > strange?
>
> That is moot. This design was made many years ago and is now expected.
>
> I also don't see it as a huge issue.

Sure, it's not a big problem.

>
> The effect you point out in rxtimestamp.c was known and reported in
> the test commit itself.
>
> Perhaps a more interesting argument would be
> SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE. But
> spurious software rx timestamps are trivially ignored.
>
> > Besides those two concepts you mentioned, could you explain if there
> > are side effects that the series has and what kind of bad consequences
> > that the series could bring?
>
> It doesn't do the same for hardware timestamping, creating
> inconsistency.
>
> Changing established interfaces always risks production issues. In
> this case, I'm not convinced that the benefit outweighs this risk.

I got it.

I'm thinking that I'm not the first one and the last one who know/find
this long standing "issue", could we at least documentented it
somewhere, like adding comments in the selftests or Documentation, to
avoid the similar confusion in the future? Or change the behaviour in
the rxtimestamp.c test? What do you think about it? Adding
documentation or comments is the simplest way:)

Thanks,
Jason

>
> > I tried to make it more logical and also don't want to break the
> > existing use behaviour of applications.
> >
> > I believe that what I wrote doesn't have an impact on other cases and
> > perfects what should be perfected. No offense. If the series does no
> > harm and we keep it in the right direction, are there other reasons
> > stopping it getting approved, I wonder.
> >
> > Thanks,
> > Jason
> >
> > >
> > > > I can make sure this series can fix the issue. This series is trying
> > > > to ask users to use/set both flags to receive an expected timestamp.
> > > > The purpose of using setsockopt is to control the socket itself
> > > > insteading of interfering with others.
> > > >
> > > > About the breakage issue, let me assume, if the user only sets the
> > > > SOF_TIMESTAMPING_SOFTWARE flag, he cannot expect the socket will
> > > > receive a timestamp, right? So what might happen if we fix the issue?
> > > > I think the logics in the rxtimestamp.c selftest are very clear :)
> > > >
> > > > Besides, test case 6 will fail under this circumstance
> > >
> > > Sorry about that. My team added that test, and we expanded it over
> > > time. Crucially, the test was added well after the SO_TIMESTAMPING
> > > API, so it was never intended to be prescriptive.
> > >
> > > Commit 0558c3960407 ("selftests/net: plug rxtimestamp test into
> > > kselftest framework") actually mentions this issue:
> > >
> > >     Also ignore failures of test case #6 by default. This case verifies
> > >     that a receive timestamp is not reported if timestamp reporting is
> > >     enabled for a socket, but generation is disabled. Receive timestamp
> > >     generation has to be enabled globally, as no associated socket is
> > >     known yet. A background process that enables rx timestamp generation
> > >     therefore causes a false positive. Ntpd is one example that does.
> > >
> > >     Add a "--strict" option to cause failure in the event that any test
> > >     case fails, including test #6. This is useful for environments that
> > >     are known to not have such background processes.
>
>

Willem de Bruijn Aug. 27, 2024, 1:20 p.m. UTC | #7

Jason Xing wrote:
> On Tue, Aug 27, 2024 at 2:43 AM Willem de Bruijn
> <[e-mail]> wrote:
> >
> > Jason Xing wrote:
> > > On Tue, Aug 27, 2024 at 12:03 AM Willem de Bruijn
> > > <[e-mail]> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > Hello Willem,
> > > > >
> > > > > On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > Jason Xing wrote:
> > > > > > > From: Jason Xing <[e-mail]>
> > > > > > >
> > > > > > > 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_HARDWARE 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.
> > > > > > >
> > > > > > > Signed-off-by: Jason Xing <[e-mail]>
> > > > > > > ---
> > > > > > >  net/ipv4/tcp.c | 10 ++++++++--
> > > > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > > > > index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> > > > > > >                       }
> > > > > > >               }
> > > > > > >
> > > > > > > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > > > > +             /* skb may contain timestamp because another socket
> > > > > > > +              * turned on netstamp_needed_key which allows generate
> > > > > > > +              * the timestamp. So we need to check the current socket.
> > > > > > > +              */
> > > > > > > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > > > > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> > > > > > >                       has_timestamping = true;
> > > > > > >               else
> > > > > > >                       tss->ts[0] = (struct timespec64) {0};
> > > > > > >       }
> > > > > >
> > > > > > The current behavior is as described in
> > > > > > Documentation/networking/timestamping.rst:
> > > > > >
> > > > > > "The socket option configures timestamp generation for individual
> > > > > > sk_buffs (1.3.1), timestamp reporting to the socket's error
> > > > > > queue (1.3.2)"
> > > > > >
> > > > > > SOF_TIMESTAMPING_RX_SOFTWARE is a timestamp generation option.
> > > > > > SOF_TIMESTAMPING_SOFTWARE is a timestamp reporting option.
> > > > >
> > > > > Thanks for your review.
> > > > >
> > > > > Yes, it's true.
> > > > >
> > > > > >
> > > > > > This patch changes that clearly defined behavior.
> > > > >
> > > > > Why?
> > > >
> > > > Because it repurposes generation flag SOF_TIMESTAMPING_RX_SOFTWARE in
> > > > timestamp reporting.
> > > >
> > > > If a single flag configures both generation and reporting, why bother
> > > > with two flags at all.
> > >
> > > Thanks for your full and detailed explanation :)
> > >
> > > I probably understand what you're saying. You think we should strictly
> > > distinguish these two concepts "generation" and "reporting".
> > >
> > > In my opinion, they are just concepts. We can make it clear by writing
> > > some sentences in the Documentation.
> > >
> > > >
> > > > > I don't get it. Please see those testcase in
> > > > > tools/testing/selftests/net/rxtimestamp.c.
> > > > >
> > > > > >
> > > > > > On Tx the separation between generation and reporting has value, as it
> > > > > > allows setting the generation on a per packet basis with SCM_TSTAMP_*.
> > > > >
> > > > > I didn't break the logic on the tx path. tcp_recv_timestamp() is only
> > > > > related to the rx path.
> > > > >
> > > > > Regarding the tx path, I carefully take care of this logic in
> > > > > patch[2/2], so now the series only handles the issue happening in the
> > > > > rx path.
> > > > >
> > > > > >
> > > > > > On Rx it is more subtle, but the two are still tested at different
> > > > > > points in the path, and can be updated by setsockopt in between a
> > > > > > packet arrival and a recvmsg().
> > > > > >
> > > > > > The interaction between sockets on software timestamping is a
> > > > > > longstanding issue. I don't think there is any urgency to change this
> > > > >
> > > > > Oh, now I see.
> > > > >
> > > > > > now. This proposed change makes the API less consistent, and may
> > > > > > also affect applications that depend on the current behavior.
> > > > > >
> > > > >
> > > > > Maybe. But, it's not the original design which we expect, right?
> > > >
> > > > It is. Your argument is against the current API design. This is not
> > > > a bug where behavior diverges from the intended interface. The doc is
> > > > clear on this.
> > > >
> > > > The API makes a distinction between generation and reporting bits. The
> > > > shared generation early in the Rx path is a long standing known issue.
> > > >
> > > > I'm not saying that the API is perfect. But it is clear in its use of
> > > > the bits. Muddling the distinction between reporting and generation
> > > > bits in one of the four cases makes the API less consistent and harder
> > > > to understand.
> > > >
> > > > If you think the API as is is wrong, then at a minimum that would
> > > > require an update to timestamping.rst. But I think that medicine may
> > > > be worse than the ailment.
> > >
> > > At least, I think it is against the use of setsockopt, that's the key
> > > reason: making people confused and thinking the setsockopt is not a
> > > per-socket fine-grained design. Don't you think it's a little bit
> > > strange?
> >
> > That is moot. This design was made many years ago and is now expected.
> >
> > I also don't see it as a huge issue.
> 
> Sure, it's not a big problem.
> 
> >
> > The effect you point out in rxtimestamp.c was known and reported in
> > the test commit itself.
> >
> > Perhaps a more interesting argument would be
> > SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE. But
> > spurious software rx timestamps are trivially ignored.
> >
> > > Besides those two concepts you mentioned, could you explain if there
> > > are side effects that the series has and what kind of bad consequences
> > > that the series could bring?
> >
> > It doesn't do the same for hardware timestamping, creating
> > inconsistency.

Taking a closer look at the code, there are actually already two weird
special cases here.

SOF_TIMESTAMPING_RX_HARDWARE never has to be passed, as rx hardware
timestamp generation is configured through SIOCSHWTSTAMP.

SOF_TIMESTAMPING_RX_SOFTWARE already enables timestamp reporting from
sock_recv_timestamp(), while reporting should not be conditional on
this generation flag.

        /*
         * generate control messages if
         * - receive time stamping in software requested
         * - software time stamp available and wanted
         * - hardware time stamps available and wanted
         */
        if (sock_flag(sk, SOCK_RCVTSTAMP) ||
            (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
            (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
            (hwtstamps->hwtstamp &&
             (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
                __sock_recv_timestamp(msg, sk, skb);

I evidently already noticed this back in 2014, when I left a note in
commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
sk_flags"):

    SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
    timestamp logic (netstamp_needed). That can be simplified and this
    last key removed, but will leave that for a separate patch.

But I do not see __sock_recv_timestamp toggling the feature either
then or now, so I think this is vestigial and can be removed.

> >
> > Changing established interfaces always risks production issues. In
> > this case, I'm not convinced that the benefit outweighs this risk.
> 
> I got it.
> 
> I'm thinking that I'm not the first one and the last one who know/find
> this long standing "issue", could we at least documentented it
> somewhere, like adding comments in the selftests or Documentation, to
> avoid the similar confusion in the future? Or change the behaviour in
> the rxtimestamp.c test? What do you think about it? Adding
> documentation or comments is the simplest way:)

I can see the value of your extra filter. Given the above examples, it
won't be the first subtle variance from the API design, either.

So either way is fine with me: change it or leave it.

But in both ways, yes: please update the documentation accordingly.

And if you do choose to change it, please be ready to revert on report
of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
because that always worked as they subtly relied on another daemon to
enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.

> Thanks,
> Jason
> 
> >
> > > I tried to make it more logical and also don't want to break the
> > > existing use behaviour of applications.
> > >
> > > I believe that what I wrote doesn't have an impact on other cases and
> > > perfects what should be perfected. No offense. If the series does no
> > > harm and we keep it in the right direction, are there other reasons
> > > stopping it getting approved, I wonder.
> > >
> > > Thanks,
> > > Jason
> > >
> > > >
> > > > > I can make sure this series can fix the issue. This series is trying
> > > > > to ask users to use/set both flags to receive an expected timestamp.
> > > > > The purpose of using setsockopt is to control the socket itself
> > > > > insteading of interfering with others.
> > > > >
> > > > > About the breakage issue, let me assume, if the user only sets the
> > > > > SOF_TIMESTAMPING_SOFTWARE flag, he cannot expect the socket will
> > > > > receive a timestamp, right? So what might happen if we fix the issue?
> > > > > I think the logics in the rxtimestamp.c selftest are very clear :)
> > > > >
> > > > > Besides, test case 6 will fail under this circumstance
> > > >
> > > > Sorry about that. My team added that test, and we expanded it over
> > > > time. Crucially, the test was added well after the SO_TIMESTAMPING
> > > > API, so it was never intended to be prescriptive.
> > > >
> > > > Commit 0558c3960407 ("selftests/net: plug rxtimestamp test into
> > > > kselftest framework") actually mentions this issue:
> > > >
> > > >     Also ignore failures of test case #6 by default. This case verifies
> > > >     that a receive timestamp is not reported if timestamp reporting is
> > > >     enabled for a socket, but generation is disabled. Receive timestamp
> > > >     generation has to be enabled globally, as no associated socket is
> > > >     known yet. A background process that enables rx timestamp generation
> > > >     therefore causes a false positive. Ntpd is one example that does.
> > > >
> > > >     Add a "--strict" option to cause failure in the event that any test
> > > >     case fails, including test #6. This is useful for environments that
> > > >     are known to not have such background processes.
> >
> >

Jakub Kicinski Aug. 27, 2024, 2:46 p.m. UTC | #8

On Sun, 25 Aug 2024 23:24:39 +0800 Jason Xing wrote:
> However, there is one particular case that fails the selftests with
> "./rxtimestamp: Expected swtstamp to not be set." error printing in
> testcase 6.

In case you ever find yourself looking for ways to improve our tests 
may I suggest trying to speed up fcnal_test.sh?  That'd be a very
productive use of time.

Jason Xing Aug. 27, 2024, 3:27 p.m. UTC | #9

Hello Willem,

On Tue, Aug 27, 2024 at 9:20 PM Willem de Bruijn
<[e-mail]> wrote:
>
> Jason Xing wrote:
> > On Tue, Aug 27, 2024 at 2:43 AM Willem de Bruijn
> > <[e-mail]> wrote:
> > >
> > > Jason Xing wrote:
> > > > On Tue, Aug 27, 2024 at 12:03 AM Willem de Bruijn
> > > > <[e-mail]> wrote:
> > > > >
> > > > > Jason Xing wrote:
> > > > > > Hello Willem,
> > > > > >
> > > > > > On Mon, Aug 26, 2024 at 9:24 PM Willem de Bruijn
> > > > > > <[e-mail]> wrote:
> > > > > > >
> > > > > > > Jason Xing wrote:
> > > > > > > > From: Jason Xing <[e-mail]>
> > > > > > > >
> > > > > > > > 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_HARDWARE option first, then running

Sorry, I found one mistake I made, it should be "and set
SOF_TIMESTAMPING_RX_SOFTWARE".

> > > > > > > > ./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.
> > > > > > > >
> > > > > > > > Signed-off-by: Jason Xing <[e-mail]>
> > > > > > > > ---
> > > > > > > >  net/ipv4/tcp.c | 10 ++++++++--
> > > > > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > > > > > index 8514257f4ecd..49e73d66c57d 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,19 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> > > > > > > >                       }
> > > > > > > >               }
> > > > > > > >
> > > > > > > > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > > > > > +             /* skb may contain timestamp because another socket
> > > > > > > > +              * turned on netstamp_needed_key which allows generate
> > > > > > > > +              * the timestamp. So we need to check the current socket.
> > > > > > > > +              */
> > > > > > > > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > > > > > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> > > > > > > >                       has_timestamping = true;
> > > > > > > >               else
> > > > > > > >                       tss->ts[0] = (struct timespec64) {0};
> > > > > > > >       }
[...]
> > >
> > > > Besides those two concepts you mentioned, could you explain if there
> > > > are side effects that the series has and what kind of bad consequences
> > > > that the series could bring?
> > >
> > > It doesn't do the same for hardware timestamping, creating
> > > inconsistency.
>
> Taking a closer look at the code, there are actually already two weird
> special cases here.
>
> SOF_TIMESTAMPING_RX_HARDWARE never has to be passed, as rx hardware
> timestamp generation is configured through SIOCSHWTSTAMP.

Do you refer to the patch [1/2] I wrote? To be more specific, is it
about the above wrong commit message which I just modified?

Things could happen when other unrelated threads set
SOF_TIMESTAMPING_RX_SOFTWARE instead of SOF_TIMESTAMPING_RX_HARDWARE.

Sorry for the confusion.

>
> SOF_TIMESTAMPING_RX_SOFTWARE already enables timestamp reporting from
> sock_recv_timestamp(), while reporting should not be conditional on
> this generation flag.

I'm not sure if you're talking about patch [2/2] in the series. But I guess so.

I can see what you mean here: you don't like combining the reporting
flag and generation flag, right? But If we don't check whether those
two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
protocols like udp will fail as we talked before.

netstamp_needed_key cannot be implemented as per socket feature (at
that time when the driver just pass the skb to the rx stack, we don't
know which socket the skb belongs to). Since we cannot prevent this
from happening during its generation period, I suppose we can delay
the check and try to stop it when it has to report, I mean, in
sock_recv_timestamp().

Or am I missing something? What would you suggest?

>
>         /*
>          * generate control messages if
>          * - receive time stamping in software requested
>          * - software time stamp available and wanted
>          * - hardware time stamps available and wanted
>          */
>         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
>             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
>             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
>             (hwtstamps->hwtstamp &&
>              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
>                 __sock_recv_timestamp(msg, sk, skb);
>
> I evidently already noticed this back in 2014, when I left a note in
> commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> sk_flags"):
>
>     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
>     timestamp logic (netstamp_needed). That can be simplified and this
>     last key removed, but will leave that for a separate patch.
>
> But I do not see __sock_recv_timestamp toggling the feature either
> then or now, so I think this is vestigial and can be removed.

I'm not so sure about the unix case, I can see this call trace:
unix_dgram_recvmsg()->__unix_dgram_recvmsg()->__sock_recv_timestamp().

The reason why I added the check in in __sock_recv_timestamp () in the
patch [2/2] is considering the above call trace.

One thing I can be sure of is that removing the modification in
__sock_recv_timestamp in that patch doesn't affect the selftests.

Please correct me if I'm wrong.

>
> > >
> > > Changing established interfaces always risks production issues. In
> > > this case, I'm not convinced that the benefit outweighs this risk.
> >
> > I got it.
> >
> > I'm thinking that I'm not the first one and the last one who know/find
> > this long standing "issue", could we at least documentented it
> > somewhere, like adding comments in the selftests or Documentation, to
> > avoid the similar confusion in the future? Or change the behaviour in
> > the rxtimestamp.c test? What do you think about it? Adding
> > documentation or comments is the simplest way:)
>
> I can see the value of your extra filter. Given the above examples, it
> won't be the first subtle variance from the API design, either.

Really appreciate that you understand me :)

>
> So either way is fine with me: change it or leave it.
>
> But in both ways, yes: please update the documentation accordingly.

Roger that, sir. I will do it.

>
> And if you do choose to change it, please be ready to revert on report
> of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
> because that always worked as they subtly relied on another daemon to
> enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.

Yes, I still chose to change it and try to make it in the correct
direction. So if there are future reports, please let me know, I will
surely keep a close eye on it.

Thanks,
Jason

Jason Xing Aug. 27, 2024, 3:31 p.m. UTC | #10

Hello Jakub,

On Tue, Aug 27, 2024 at 10:46 PM Jakub Kicinski <[e-mail]> wrote:
>
> On Sun, 25 Aug 2024 23:24:39 +0800 Jason Xing wrote:
> > However, there is one particular case that fails the selftests with
> > "./rxtimestamp: Expected swtstamp to not be set." error printing in
> > testcase 6.
>
> In case you ever find yourself looking for ways to improve our tests
> may I suggest trying to speed up fcnal_test.sh?  That'd be a very
> productive use of time.

Actually, I'm very into this timestamping feature which can be
extended in some aspects for debug&trace in production.

Since you mentioned that we can do more about that test, I think I
will take a deep look at it during the weekend :)

Thanks for letting me know :p

Willem de Bruijn Aug. 27, 2024, 5:08 p.m. UTC | #11

> > > > > Besides those two concepts you mentioned, could you explain if there
> > > > > are side effects that the series has and what kind of bad consequences
> > > > > that the series could bring?
> > > >
> > > > It doesn't do the same for hardware timestamping, creating
> > > > inconsistency.
> >
> > Taking a closer look at the code, there are actually already two weird
> > special cases here.
> >
> > SOF_TIMESTAMPING_RX_HARDWARE never has to be passed, as rx hardware
> > timestamp generation is configured through SIOCSHWTSTAMP.
> 
> Do you refer to the patch [1/2] I wrote? To be more specific, is it
> about the above wrong commit message which I just modified?
> 
> Things could happen when other unrelated threads set
> SOF_TIMESTAMPING_RX_SOFTWARE instead of SOF_TIMESTAMPING_RX_HARDWARE.
> 
> Sorry for the confusion.

No, this is referring to the current state.

> >
> > SOF_TIMESTAMPING_RX_SOFTWARE already enables timestamp reporting from
> > sock_recv_timestamp(), while reporting should not be conditional on
> > this generation flag.
> 
> I'm not sure if you're talking about patch [2/2] in the series. But I guess so.

Nope, same thing. I mention a commit from 2014.

> I can see what you mean here: you don't like combining the reporting
> flag and generation flag, right? But If we don't check whether those
> two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> protocols like udp will fail as we talked before.
> 
> netstamp_needed_key cannot be implemented as per socket feature (at
> that time when the driver just pass the skb to the rx stack, we don't
> know which socket the skb belongs to). Since we cannot prevent this
> from happening during its generation period, I suppose we can delay
> the check and try to stop it when it has to report, I mean, in
> sock_recv_timestamp().
> 
> Or am I missing something? What would you suggest?
> 
> >
> >         /*
> >          * generate control messages if
> >          * - receive time stamping in software requested
> >          * - software time stamp available and wanted
> >          * - hardware time stamps available and wanted
> >          */
> >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> >             (hwtstamps->hwtstamp &&
> >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> >                 __sock_recv_timestamp(msg, sk, skb);
> >
> > I evidently already noticed this back in 2014, when I left a note in
> > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > sk_flags"):
> >
> >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> >     timestamp logic (netstamp_needed). That can be simplified and this
> >     last key removed, but will leave that for a separate patch.
> >
> > But I do not see __sock_recv_timestamp toggling the feature either
> > then or now, so I think this is vestigial and can be removed.
> 
> I'm not so sure about the unix case, I can see this call trace:
> unix_dgram_recvmsg()->__unix_dgram_recvmsg()->__sock_recv_timestamp().
> 
> The reason why I added the check in in __sock_recv_timestamp () in the
> patch [2/2] is considering the above call trace.
> 
> One thing I can be sure of is that removing the modification in
> __sock_recv_timestamp in that patch doesn't affect the selftests.
> 
> Please correct me if I'm wrong.

I think we're talking alongside each other. I was pointing to code
before your patch.
 
> >
> > > >
> > > > Changing established interfaces always risks production issues. In
> > > > this case, I'm not convinced that the benefit outweighs this risk.
> > >
> > > I got it.
> > >
> > > I'm thinking that I'm not the first one and the last one who know/find
> > > this long standing "issue", could we at least documentented it
> > > somewhere, like adding comments in the selftests or Documentation, to
> > > avoid the similar confusion in the future? Or change the behaviour in
> > > the rxtimestamp.c test? What do you think about it? Adding
> > > documentation or comments is the simplest way:)
> >
> > I can see the value of your extra filter. Given the above examples, it
> > won't be the first subtle variance from the API design, either.
> 
> Really appreciate that you understand me :)
> 
> >
> > So either way is fine with me: change it or leave it.
> >
> > But in both ways, yes: please update the documentation accordingly.
> 
> Roger that, sir. I will do it.
> 
> >
> > And if you do choose to change it, please be ready to revert on report
> > of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
> > because that always worked as they subtly relied on another daemon to
> > enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.
> 
> Yes, I still chose to change it and try to make it in the correct
> direction. So if there are future reports, please let me know, I will
> surely keep a close eye on it.

Sounds good, thanks.

Jason Xing Aug. 28, 2024, 12:43 a.m. UTC | #12

On Wed, Aug 28, 2024 at 1:08 AM Willem de Bruijn
<[e-mail]> wrote:
>
> > > > > > Besides those two concepts you mentioned, could you explain if there
> > > > > > are side effects that the series has and what kind of bad consequences
> > > > > > that the series could bring?
> > > > >
> > > > > It doesn't do the same for hardware timestamping, creating
> > > > > inconsistency.
> > >
> > > Taking a closer look at the code, there are actually already two weird
> > > special cases here.
> > >
> > > SOF_TIMESTAMPING_RX_HARDWARE never has to be passed, as rx hardware
> > > timestamp generation is configured through SIOCSHWTSTAMP.
> >
> > Do you refer to the patch [1/2] I wrote? To be more specific, is it
> > about the above wrong commit message which I just modified?
> >
> > Things could happen when other unrelated threads set
> > SOF_TIMESTAMPING_RX_SOFTWARE instead of SOF_TIMESTAMPING_RX_HARDWARE.
> >
> > Sorry for the confusion.
>
> No, this is referring to the current state.
>
> > >
> > > SOF_TIMESTAMPING_RX_SOFTWARE already enables timestamp reporting from
> > > sock_recv_timestamp(), while reporting should not be conditional on
> > > this generation flag.
> >
> > I'm not sure if you're talking about patch [2/2] in the series. But I guess so.
>
> Nope, same thing. I mention a commit from 2014.

I thought you asked me to change these two last night. Actually you
were only stating the fact: two cases where we use both generation and
reporting flags already exist before.

Okay, I finally got it. It's fine to me from my point of view :)

>
> > I can see what you mean here: you don't like combining the reporting
> > flag and generation flag, right? But If we don't check whether those
> > two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> > SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> > protocols like udp will fail as we talked before.
> >
> > netstamp_needed_key cannot be implemented as per socket feature (at
> > that time when the driver just pass the skb to the rx stack, we don't
> > know which socket the skb belongs to). Since we cannot prevent this
> > from happening during its generation period, I suppose we can delay
> > the check and try to stop it when it has to report, I mean, in
> > sock_recv_timestamp().
> >
> > Or am I missing something? What would you suggest?
> >
> > >
> > >         /*
> > >          * generate control messages if
> > >          * - receive time stamping in software requested
> > >          * - software time stamp available and wanted
> > >          * - hardware time stamps available and wanted
> > >          */
> > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > >             (hwtstamps->hwtstamp &&
> > >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > >                 __sock_recv_timestamp(msg, sk, skb);
> > >
> > > I evidently already noticed this back in 2014, when I left a note in
> > > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > > sk_flags"):
> > >
> > >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> > >     timestamp logic (netstamp_needed). That can be simplified and this
> > >     last key removed, but will leave that for a separate patch.
> > >
> > > But I do not see __sock_recv_timestamp toggling the feature either
> > > then or now, so I think this is vestigial and can be removed.

After investigating more of it, as your previous commit said, the
legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
flag from enum sock_flags {}, right? Do you expect me to do that? If
so, I would love to do it :)

But I still don't get it when you say "__sock_recv_timestamp toggling
the feature", could you say more, please? I'm not sure if it has
something to do with the above line.

Thanks for your patience:)

> >
> > I'm not so sure about the unix case, I can see this call trace:
> > unix_dgram_recvmsg()->__unix_dgram_recvmsg()->__sock_recv_timestamp().
> >
> > The reason why I added the check in in __sock_recv_timestamp () in the
> > patch [2/2] is considering the above call trace.
> >
> > One thing I can be sure of is that removing the modification in
> > __sock_recv_timestamp in that patch doesn't affect the selftests.
> >
> > Please correct me if I'm wrong.
>
> I think we're talking alongside each other. I was pointing to code
> before your patch.
>
> > >
> > > > >
> > > > > Changing established interfaces always risks production issues. In
> > > > > this case, I'm not convinced that the benefit outweighs this risk.
> > > >
> > > > I got it.
> > > >
> > > > I'm thinking that I'm not the first one and the last one who know/find
> > > > this long standing "issue", could we at least documentented it
> > > > somewhere, like adding comments in the selftests or Documentation, to
> > > > avoid the similar confusion in the future? Or change the behaviour in
> > > > the rxtimestamp.c test? What do you think about it? Adding
> > > > documentation or comments is the simplest way:)
> > >
> > > I can see the value of your extra filter. Given the above examples, it
> > > won't be the first subtle variance from the API design, either.
> >
> > Really appreciate that you understand me :)
> >
> > >
> > > So either way is fine with me: change it or leave it.
> > >
> > > But in both ways, yes: please update the documentation accordingly.
> >
> > Roger that, sir. I will do it.
> >
> > >
> > > And if you do choose to change it, please be ready to revert on report
> > > of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
> > > because that always worked as they subtly relied on another daemon to
> > > enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.
> >
> > Yes, I still chose to change it and try to make it in the correct
> > direction. So if there are future reports, please let me know, I will
> > surely keep a close eye on it.
>
> Sounds good, thanks.

So let me organize my thoughts here.

In the next move, I would do such things:
1) keep two patches in this series as they are.
2) add some descriptions about "this commit introduces subtle
variance, if the application that only pass
SOF_TIMESTAMPING_SOFTWARE..." something like this in the Documentation
file.
3) remove the last key SOCK_TIMESTAMPING_RX_SOFTWARE from enum
sk_flags, if you want me to do so :)

If there is something weird here, please point it out so that I can
make the right move.

Thanks,
Jason

Willem de Bruijn Aug. 28, 2024, 1:44 p.m. UTC | #13

> > > I can see what you mean here: you don't like combining the reporting
> > > flag and generation flag, right? But If we don't check whether those
> > > two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> > > SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> > > protocols like udp will fail as we talked before.
> > >
> > > netstamp_needed_key cannot be implemented as per socket feature (at
> > > that time when the driver just pass the skb to the rx stack, we don't
> > > know which socket the skb belongs to). Since we cannot prevent this
> > > from happening during its generation period, I suppose we can delay
> > > the check and try to stop it when it has to report, I mean, in
> > > sock_recv_timestamp().
> > >
> > > Or am I missing something? What would you suggest?
> > >
> > > >
> > > >         /*
> > > >          * generate control messages if
> > > >          * - receive time stamping in software requested
> > > >          * - software time stamp available and wanted
> > > >          * - hardware time stamps available and wanted
> > > >          */
> > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > > >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > > >             (hwtstamps->hwtstamp &&
> > > >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > > >                 __sock_recv_timestamp(msg, sk, skb);
> > > >
> > > > I evidently already noticed this back in 2014, when I left a note in
> > > > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > > > sk_flags"):
> > > >
> > > >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> > > >     timestamp logic (netstamp_needed). That can be simplified and this
> > > >     last key removed, but will leave that for a separate patch.
> > > >
> > > > But I do not see __sock_recv_timestamp toggling the feature either
> > > > then or now, so I think this is vestigial and can be removed.
> 
> After investigating more of it, as your previous commit said, the
> legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
> SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
> flag from enum sock_flags {}, right? Do you expect me to do that? If
> so, I would love to do it :)

I did not say that. I said that the specific line here appears
vestigial.

> > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||

One thing at a time. Let's focus on the change you proposed to me.
 
> But I still don't get it when you say "__sock_recv_timestamp toggling
> the feature", could you say more, please? I'm not sure if it has
> something to do with the above line.

SOF_TIMESTAMPING_RX_SOFTWARE is a request to enable software receive
timestamp *generation*. This is done by calling net_enable_timestamp.

I did not immediately see a path from __sock_recv_timestamp to
net_enable_timestamp, so I don't see a point in entering that function
based on this flag.

> > > > I can see the value of your extra filter. Given the above examples, it
> > > > won't be the first subtle variance from the API design, either.
> > >
> > > Really appreciate that you understand me :)
> > >
> > > >
> > > > So either way is fine with me: change it or leave it.
> > > >
> > > > But in both ways, yes: please update the documentation accordingly.
> > >
> > > Roger that, sir. I will do it.
> > >
> > > >
> > > > And if you do choose to change it, please be ready to revert on report
> > > > of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
> > > > because that always worked as they subtly relied on another daemon to
> > > > enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.
> > >
> > > Yes, I still chose to change it and try to make it in the correct
> > > direction. So if there are future reports, please let me know, I will
> > > surely keep a close eye on it.
> >
> > Sounds good, thanks.
> 
> So let me organize my thoughts here.
> 
> In the next move, I would do such things:
> 1) keep two patches in this series as they are.
> 2) add some descriptions about "this commit introduces subtle
> variance, if the application that only pass
> SOF_TIMESTAMPING_SOFTWARE..." something like this in the Documentation
> file.

Make it crystal clear that the distinction between timestamp
generation and timestamp reporting, which this document goes out of
its way to explain, does not hold for receive timestamping.

> 3) remove the last key SOCK_TIMESTAMPING_RX_SOFTWARE from enum
> sk_flags, if you want me to do so :)

Nope.

> If there is something weird here, please point it out so that I can
> make the right move.
> 
> Thanks,
> Jason

Jason Xing Aug. 28, 2024, 2:02 p.m. UTC | #14

On Wed, Aug 28, 2024 at 9:44 PM Willem de Bruijn
<[e-mail]> wrote:
>
> > > > I can see what you mean here: you don't like combining the reporting
> > > > flag and generation flag, right? But If we don't check whether those
> > > > two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> > > > SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> > > > protocols like udp will fail as we talked before.
> > > >
> > > > netstamp_needed_key cannot be implemented as per socket feature (at
> > > > that time when the driver just pass the skb to the rx stack, we don't
> > > > know which socket the skb belongs to). Since we cannot prevent this
> > > > from happening during its generation period, I suppose we can delay
> > > > the check and try to stop it when it has to report, I mean, in
> > > > sock_recv_timestamp().
> > > >
> > > > Or am I missing something? What would you suggest?
> > > >
> > > > >
> > > > >         /*
> > > > >          * generate control messages if
> > > > >          * - receive time stamping in software requested
> > > > >          * - software time stamp available and wanted
> > > > >          * - hardware time stamps available and wanted
> > > > >          */
> > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > > > >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > > > >             (hwtstamps->hwtstamp &&
> > > > >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > > > >                 __sock_recv_timestamp(msg, sk, skb);
> > > > >
> > > > > I evidently already noticed this back in 2014, when I left a note in
> > > > > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > > > > sk_flags"):
> > > > >
> > > > >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> > > > >     timestamp logic (netstamp_needed). That can be simplified and this
> > > > >     last key removed, but will leave that for a separate patch.
> > > > >
> > > > > But I do not see __sock_recv_timestamp toggling the feature either
> > > > > then or now, so I think this is vestigial and can be removed.
> >
> > After investigating more of it, as your previous commit said, the
> > legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
> > SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
> > flag from enum sock_flags {}, right? Do you expect me to do that? If
> > so, I would love to do it :)
>
> I did not say that. I said that the specific line here appears
> vestigial.
>
> > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||

Thanks for your patience.

I think I will remove the above two lines as one patch with your
suggested-by tag in the series so that later we can easily review each
of them.

>
> One thing at a time. Let's focus on the change you proposed to me.
>
> > But I still don't get it when you say "__sock_recv_timestamp toggling
> > the feature", could you say more, please? I'm not sure if it has
> > something to do with the above line.
>
> SOF_TIMESTAMPING_RX_SOFTWARE is a request to enable software receive
> timestamp *generation*. This is done by calling net_enable_timestamp.
>
> I did not immediately see a path from __sock_recv_timestamp to
> net_enable_timestamp, so I don't see a point in entering that function
> based on this flag.

I see. It does make sense. We don't need the generation flag to test
if we need to report here.

I will remove them by quoting your saying.

>
> > > > > I can see the value of your extra filter. Given the above examples, it
> > > > > won't be the first subtle variance from the API design, either.
> > > >
> > > > Really appreciate that you understand me :)
> > > >
> > > > >
> > > > > So either way is fine with me: change it or leave it.
> > > > >
> > > > > But in both ways, yes: please update the documentation accordingly.
> > > >
> > > > Roger that, sir. I will do it.
> > > >
> > > > >
> > > > > And if you do choose to change it, please be ready to revert on report
> > > > > of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
> > > > > because that always worked as they subtly relied on another daemon to
> > > > > enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.
> > > >
> > > > Yes, I still chose to change it and try to make it in the correct
> > > > direction. So if there are future reports, please let me know, I will
> > > > surely keep a close eye on it.
> > >
> > > Sounds good, thanks.
> >
> > So let me organize my thoughts here.
> >
> > In the next move, I would do such things:
> > 1) keep two patches in this series as they are.
> > 2) add some descriptions about "this commit introduces subtle
> > variance, if the application that only pass
> > SOF_TIMESTAMPING_SOFTWARE..." something like this in the Documentation
> > file.
>
> Make it crystal clear that the distinction between timestamp
> generation and timestamp reporting, which this document goes out of
> its way to explain, does not hold for receive timestamping.

Sure, I will add it into the documentation as well.

Thanks for your help.

Willem de Bruijn Aug. 28, 2024, 2:10 p.m. UTC | #15

Jason Xing wrote:
> On Wed, Aug 28, 2024 at 9:44 PM Willem de Bruijn
> <[e-mail]> wrote:
> >
> > > > > I can see what you mean here: you don't like combining the reporting
> > > > > flag and generation flag, right? But If we don't check whether those
> > > > > two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> > > > > SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> > > > > protocols like udp will fail as we talked before.
> > > > >
> > > > > netstamp_needed_key cannot be implemented as per socket feature (at
> > > > > that time when the driver just pass the skb to the rx stack, we don't
> > > > > know which socket the skb belongs to). Since we cannot prevent this
> > > > > from happening during its generation period, I suppose we can delay
> > > > > the check and try to stop it when it has to report, I mean, in
> > > > > sock_recv_timestamp().
> > > > >
> > > > > Or am I missing something? What would you suggest?
> > > > >
> > > > > >
> > > > > >         /*
> > > > > >          * generate control messages if
> > > > > >          * - receive time stamping in software requested
> > > > > >          * - software time stamp available and wanted
> > > > > >          * - hardware time stamps available and wanted
> > > > > >          */
> > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > > > > >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > > > > >             (hwtstamps->hwtstamp &&
> > > > > >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > > > > >                 __sock_recv_timestamp(msg, sk, skb);
> > > > > >
> > > > > > I evidently already noticed this back in 2014, when I left a note in
> > > > > > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > > > > > sk_flags"):
> > > > > >
> > > > > >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> > > > > >     timestamp logic (netstamp_needed). That can be simplified and this
> > > > > >     last key removed, but will leave that for a separate patch.
> > > > > >
> > > > > > But I do not see __sock_recv_timestamp toggling the feature either
> > > > > > then or now, so I think this is vestigial and can be removed.
> > >
> > > After investigating more of it, as your previous commit said, the
> > > legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
> > > SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
> > > flag from enum sock_flags {}, right? Do you expect me to do that? If
> > > so, I would love to do it :)
> >
> > I did not say that. I said that the specific line here appears
> > vestigial.
> >
> > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> 
> Thanks for your patience.
> 
> I think I will remove the above two lines as one patch with your
> suggested-by tag in the series so that later we can easily review each
> of them.

Please don't. As I said right below this:

One thing at a time. Let's focus on the change you proposed to me.

The above is just a hunch on first read. I would have to spend more
time to convince myself that it is indeed correct and safe. At which
point I might as well send it myself too..

More importantly, let's not expand what is intended to be a stand
alone improvement with tangential changes. It just makes reviewing
harder and slows it down.
 
> >
> > One thing at a time. Let's focus on the change you proposed to me.
> >
> > > But I still don't get it when you say "__sock_recv_timestamp toggling
> > > the feature", could you say more, please? I'm not sure if it has
> > > something to do with the above line.
> >
> > SOF_TIMESTAMPING_RX_SOFTWARE is a request to enable software receive
> > timestamp *generation*. This is done by calling net_enable_timestamp.
> >
> > I did not immediately see a path from __sock_recv_timestamp to
> > net_enable_timestamp, so I don't see a point in entering that function
> > based on this flag.
> 
> I see. It does make sense. We don't need the generation flag to test
> if we need to report here.
> 
> I will remove them by quoting your saying.

See above.

> >
> > > > > > I can see the value of your extra filter. Given the above examples, it
> > > > > > won't be the first subtle variance from the API design, either.
> > > > >
> > > > > Really appreciate that you understand me :)
> > > > >
> > > > > >
> > > > > > So either way is fine with me: change it or leave it.
> > > > > >
> > > > > > But in both ways, yes: please update the documentation accordingly.
> > > > >
> > > > > Roger that, sir. I will do it.
> > > > >
> > > > > >
> > > > > > And if you do choose to change it, please be ready to revert on report
> > > > > > of breakage. Applications that only pass SOF_TIMESTAMPING_SOFTWARE,
> > > > > > because that always worked as they subtly relied on another daemon to
> > > > > > enable SOF_TIMESTAMPING_RX_SOFTWARE, for instance.
> > > > >
> > > > > Yes, I still chose to change it and try to make it in the correct
> > > > > direction. So if there are future reports, please let me know, I will
> > > > > surely keep a close eye on it.
> > > >
> > > > Sounds good, thanks.
> > >
> > > So let me organize my thoughts here.
> > >
> > > In the next move, I would do such things:
> > > 1) keep two patches in this series as they are.
> > > 2) add some descriptions about "this commit introduces subtle
> > > variance, if the application that only pass
> > > SOF_TIMESTAMPING_SOFTWARE..." something like this in the Documentation
> > > file.
> >
> > Make it crystal clear that the distinction between timestamp
> > generation and timestamp reporting, which this document goes out of
> > its way to explain, does not hold for receive timestamping.
> 
> Sure, I will add it into the documentation as well.
> 
> Thanks for your help.

Jason Xing Aug. 28, 2024, 2:18 p.m. UTC | #16

On Wed, Aug 28, 2024 at 10:10 PM Willem de Bruijn
<[e-mail]> wrote:
>
> Jason Xing wrote:
> > On Wed, Aug 28, 2024 at 9:44 PM Willem de Bruijn
> > <[e-mail]> wrote:
> > >
> > > > > > I can see what you mean here: you don't like combining the reporting
> > > > > > flag and generation flag, right? But If we don't check whether those
> > > > > > two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> > > > > > SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> > > > > > protocols like udp will fail as we talked before.
> > > > > >
> > > > > > netstamp_needed_key cannot be implemented as per socket feature (at
> > > > > > that time when the driver just pass the skb to the rx stack, we don't
> > > > > > know which socket the skb belongs to). Since we cannot prevent this
> > > > > > from happening during its generation period, I suppose we can delay
> > > > > > the check and try to stop it when it has to report, I mean, in
> > > > > > sock_recv_timestamp().
> > > > > >
> > > > > > Or am I missing something? What would you suggest?
> > > > > >
> > > > > > >
> > > > > > >         /*
> > > > > > >          * generate control messages if
> > > > > > >          * - receive time stamping in software requested
> > > > > > >          * - software time stamp available and wanted
> > > > > > >          * - hardware time stamps available and wanted
> > > > > > >          */
> > > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > > > > > >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > > > > > >             (hwtstamps->hwtstamp &&
> > > > > > >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > > > > > >                 __sock_recv_timestamp(msg, sk, skb);
> > > > > > >
> > > > > > > I evidently already noticed this back in 2014, when I left a note in
> > > > > > > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > > > > > > sk_flags"):
> > > > > > >
> > > > > > >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> > > > > > >     timestamp logic (netstamp_needed). That can be simplified and this
> > > > > > >     last key removed, but will leave that for a separate patch.
> > > > > > >
> > > > > > > But I do not see __sock_recv_timestamp toggling the feature either
> > > > > > > then or now, so I think this is vestigial and can be removed.
> > > >
> > > > After investigating more of it, as your previous commit said, the
> > > > legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
> > > > SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
> > > > flag from enum sock_flags {}, right? Do you expect me to do that? If
> > > > so, I would love to do it :)
> > >
> > > I did not say that. I said that the specific line here appears
> > > vestigial.
> > >
> > > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> >
> > Thanks for your patience.
> >
> > I think I will remove the above two lines as one patch with your
> > suggested-by tag in the series so that later we can easily review each
> > of them.
>
> Please don't. As I said right below this:
>
> One thing at a time. Let's focus on the change you proposed to me.
>
> The above is just a hunch on first read. I would have to spend more
> time to convince myself that it is indeed correct and safe. At which
> point I might as well send it myself too..
>
> More importantly, let's not expand what is intended to be a stand
> alone improvement with tangential changes. It just makes reviewing
> harder and slows it down.

I got it :)

I think I will not touch the __sock_recv_timestamp() function and then
move those test statements from sock_recv_timestamp() to
__sock_recv_timestamp() in patch [2/2].

Let me send a v2 soon to see if they are correct.

Thanks,
Jason

Patch

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 8514257f4ecd..49e73d66c57d 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,19 @@  void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
 			}
 		}
 
-		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
+		/* skb may contain timestamp because another socket
+		 * turned on netstamp_needed_key which allows generate
+		 * the timestamp. So we need to check the current socket.
+		 */
+		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};