[PATCH net-next v3 0/2] net-timestamp: introduce a flag to filter out rx software report

[PATCH net-next v3 0/2] net-timestamp: introduce a flag to filter out rx software report

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

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

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

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

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


Jason Xing (2):
  net-timestamp: filter out report when setting
    SOF_TIMESTAMPING_SOFTWARE
  rxtimestamp.c: add the test for
    SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER

 Documentation/networking/timestamping.rst | 12 ++++++++++++
 include/uapi/linux/net_tstamp.h           |  3 ++-
 net/core/sock.c                           |  4 ++++
 net/ethtool/common.c                      |  1 +
 net/ipv4/tcp.c                            |  7 +++++--
 net/socket.c                              |  5 ++++-
 tools/testing/selftests/net/rxtimestamp.c |  5 +++++
 7 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.37.3

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

introduce a new flag SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER in the
receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
out rx timestamp report, especially after a process turns on
netstamp_needed_key which can time stamp every incoming skb.

Previously, We found out if an application starts first which turns on
netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
could also get rx timestamp. Now we handle this case by introducing this
new flag without breaking users.

In this way, we have two kinds of combination:
1. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
will surely allow users to get the rx timestamp report.
2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
while the skb is timestamped, it will stop reporting the rx timestamp.

Another thing about errqueue in this patch I have a few words to say:
In this case, we need to handle the egress path carefully, or else
reporting the tx timestamp will fail. Egress path and ingress path will
finally call sock_recv_timestamp(). We have to distinguish them.
Errqueue is a good indicator to reflect the flow direction.

Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
1. Willem suggested this alternative way to solve the issue, so I
added his Suggested-by tag here. Thanks!
---
 Documentation/networking/timestamping.rst | 12 ++++++++++++
 include/uapi/linux/net_tstamp.h           |  3 ++-
 net/core/sock.c                           |  4 ++++
 net/ethtool/common.c                      |  1 +
 net/ipv4/tcp.c                            |  7 +++++--
 net/socket.c                              |  5 ++++-
 6 files changed, 28 insertions(+), 4 deletions(-)

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

[PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Test when we use SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER with
SOF_TIMESTAMPING_SOFTWARE. The expected result is no rx timestamp
report.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 tools/testing/selftests/net/rxtimestamp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/net/rxtimestamp.c b/tools/testing/selftests/net/rxtimestamp.c
index 9eb42570294d..f625e74663a0 100644
--- a/tools/testing/selftests/net/rxtimestamp.c
+++ b/tools/testing/selftests/net/rxtimestamp.c
@@ -97,6 +97,11 @@ static struct test_case test_cases[] = {
 			| SOF_TIMESTAMPING_RX_HARDWARE },
 		{}
 	},
+	{
+		{ .so_timestamping = SOF_TIMESTAMPING_SOFTWARE
+			| SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER },
+		{}
+	},
 	{
 		{ .so_timestamping = SOF_TIMESTAMPING_SOFTWARE
 			| SOF_TIMESTAMPING_RX_SOFTWARE },
-- 
2.37.3

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

[Willem de Bruijn]

Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> introduce a new flag SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER in the
> receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
> out rx timestamp report, especially after a process turns on
> netstamp_needed_key which can time stamp every incoming skb.
> 
> Previously, We found out if an application starts first which turns on
> netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> could also get rx timestamp. Now we handle this case by introducing this
> new flag without breaking users.
> 
> In this way, we have two kinds of combination:
> 1. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
> will surely allow users to get the rx timestamp report.
> 2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
> while the skb is timestamped, it will stop reporting the rx timestamp.

The one point this commit does not explain is why a process would want
to request software timestamp reporting, but not receive software
timestamp generation. The only use I see is when the application does
request SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE.
 
> Another thing about errqueue in this patch I have a few words to say:
> In this case, we need to handle the egress path carefully, or else
> reporting the tx timestamp will fail. Egress path and ingress path will
> finally call sock_recv_timestamp(). We have to distinguish them.
> Errqueue is a good indicator to reflect the flow direction.

Good find on skb_is_err_queue rather than open-coding PACKET_OUTGOING.

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

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

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

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

Documentation should describe the current state, not a history of
changes (log), so wording like "finally" does not belong.

Re: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER

[Willem de Bruijn]

Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Test when we use SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER with
> SOF_TIMESTAMPING_SOFTWARE. The expected result is no rx timestamp
> report.
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>

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

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

[Jason Xing]

On Sat, Aug 31, 2024 at 10:50 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > introduce a new flag SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER in the
> > receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
> > out rx timestamp report, especially after a process turns on
> > netstamp_needed_key which can time stamp every incoming skb.
> >
> > Previously, We found out if an application starts first which turns on
> > netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> > could also get rx timestamp. Now we handle this case by introducing this
> > new flag without breaking users.
> >
> > In this way, we have two kinds of combination:
> > 1. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
> > will surely allow users to get the rx timestamp report.
> > 2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
> > while the skb is timestamped, it will stop reporting the rx timestamp.
>
> The one point this commit does not explain is why a process would want
> to request software timestamp reporting, but not receive software
> timestamp generation. The only use I see is when the application does
> request SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE.

Yes, you're right.

>
> > Another thing about errqueue in this patch I have a few words to say:
> > In this case, we need to handle the egress path carefully, or else
> > reporting the tx timestamp will fail. Egress path and ingress path will
> > finally call sock_recv_timestamp(). We have to distinguish them.
> > Errqueue is a good indicator to reflect the flow direction.
>
> Good find on skb_is_err_queue rather than open-coding PACKET_OUTGOING.
>
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
>
> Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks for your review:)

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

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

Thanks,
Jason

Re: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER

[kernel test robot]

Hi Jason,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Xing/net-timestamp-filter-out-report-when-setting-SOF_TIMESTAMPING_SOFTWARE/20240830-234014
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240830153751.86895-3-kerneljasonxing%40gmail.com
patch subject: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240902/202409020124.YybQQDrP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202409020124.YybQQDrP-lkp@intel.com/

All errors (new ones prefixed by >>):

>> rxtimestamp.c:102:6: error: use of undeclared identifier 'SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER'
     102 |                         | SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER },
         |                           ^
>> rxtimestamp.c:373:20: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
     373 |                         for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
         |                                         ^~~~~~~~~~~~~~~~~~~~~~
   ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
      61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
         |                                ^~~~~
   rxtimestamp.c:380:13: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
     380 |                         if (t >= ARRAY_SIZE(test_cases))
         |                                  ^~~~~~~~~~~~~~~~~~~~~~
   ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
      61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
         |                                ^~~~~
   rxtimestamp.c:419:19: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
     419 |                 for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
   ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
      61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
         |                                ^~~~~
   4 errors generated.


vim +/SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER +102 tools/testing/selftests/net/rxtimestamp.c

16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   67  
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   68  static struct test_case test_cases[] = {
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   69  	{ {}, {} },
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   70  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   71  		{ .so_timestamp = 1 },
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   72  		{ .tstamp = true }
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   73  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   74  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   75  		{ .so_timestampns = 1 },
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   76  		{ .tstampns = true }
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   77  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   78  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   79  		{ .so_timestamp = 1, .so_timestampns = 1 },
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   80  		{ .tstampns = true }
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   81  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   82  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   83  		{ .so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE },
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   84  		{}
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   85  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   86  	{
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   87  		/* Loopback device does not support hw timestamps. */
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   88  		{ .so_timestamping = SOF_TIMESTAMPING_RX_HARDWARE },
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   89  		{}
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   90  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   91  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   92  		{ .so_timestamping = SOF_TIMESTAMPING_SOFTWARE },
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   93  		.warn_on_fail = true
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   94  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   95  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04   96  		{ .so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   97  			| SOF_TIMESTAMPING_RX_HARDWARE },
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   98  		{}
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22   99  	},
93e82194e6a4ec9 tools/testing/selftests/net/rxtimestamp.c                     Jason Xing   2024-08-30  100  	{
93e82194e6a4ec9 tools/testing/selftests/net/rxtimestamp.c                     Jason Xing   2024-08-30  101  		{ .so_timestamping = SOF_TIMESTAMPING_SOFTWARE
93e82194e6a4ec9 tools/testing/selftests/net/rxtimestamp.c                     Jason Xing   2024-08-30 @102  			| SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER },
93e82194e6a4ec9 tools/testing/selftests/net/rxtimestamp.c                     Jason Xing   2024-08-30  103  		{}
93e82194e6a4ec9 tools/testing/selftests/net/rxtimestamp.c                     Jason Xing   2024-08-30  104  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  105  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04  106  		{ .so_timestamping = SOF_TIMESTAMPING_SOFTWARE
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  107  			| SOF_TIMESTAMPING_RX_SOFTWARE },
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04  108  		{ .swtstamp = true }
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  109  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  110  	{
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04  111  		{ .so_timestamp = 1, .so_timestamping = SOF_TIMESTAMPING_SOFTWARE
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  112  			| SOF_TIMESTAMPING_RX_SOFTWARE },
f551e2fdaf81b7b tools/testing/selftests/net/rxtimestamp.c                     Tanner Love  2020-07-04  113  		{ .tstamp = true, .swtstamp = true }
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  114  	},
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  115  };
16e781224198be0 tools/testing/selftests/networking/timestamping/rxtimestamp.c Mike Maloney 2017-08-22  116  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER

[Jason Xing]

On Mon, Sep 2, 2024 at 9:49 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Jason,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on net-next/main]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Xing/net-timestamp-filter-out-report-when-setting-SOF_TIMESTAMPING_SOFTWARE/20240830-234014
> base:   net-next/main
> patch link:    https://lore.kernel.org/r/20240830153751.86895-3-kerneljasonxing%40gmail.com
> patch subject: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
> :::::: branch date: 2 days ago
> :::::: commit date: 2 days ago
> compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240902/202409020124.YybQQDrP-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/r/202409020124.YybQQDrP-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> >> rxtimestamp.c:102:6: error: use of undeclared identifier 'SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER'
>      102 |                         | SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER },
>          |                           ^
> >> rxtimestamp.c:373:20: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
>      373 |                         for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
>          |                                         ^~~~~~~~~~~~~~~~~~~~~~
>    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
>       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>          |                                ^~~~~
>    rxtimestamp.c:380:13: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
>      380 |                         if (t >= ARRAY_SIZE(test_cases))
>          |                                  ^~~~~~~~~~~~~~~~~~~~~~
>    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
>       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>          |                                ^~~~~
>    rxtimestamp.c:419:19: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
>      419 |                 for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
>          |                                 ^~~~~~~~~~~~~~~~~~~~~~
>    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
>       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>          |                                ^~~~~
>    4 errors generated.

I didn't get how it happened? I've already test it locally.

Is it because the test environment didn't update the header files by
using the command like "make headers_install && cp -r
usr/include/linux /usr/include/"?

If the applications or some userspace tools try to use the new flag,
it should update the header file to sync the uapi file first.

Thanks,
Jason

Re: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER

[Philip Li]

On Mon, Sep 02, 2024 at 10:41:28AM +0800, Jason Xing wrote:
> On Mon, Sep 2, 2024 at 9:49 AM kernel test robot <lkp@intel.com> wrote:
> >
> > Hi Jason,
> >
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on net-next/main]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Xing/net-timestamp-filter-out-report-when-setting-SOF_TIMESTAMPING_SOFTWARE/20240830-234014
> > base:   net-next/main
> > patch link:    https://lore.kernel.org/r/20240830153751.86895-3-kerneljasonxing%40gmail.com
> > patch subject: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
> > :::::: branch date: 2 days ago
> > :::::: commit date: 2 days ago
> > compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240902/202409020124.YybQQDrP-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/r/202409020124.YybQQDrP-lkp@intel.com/
> >
> > All errors (new ones prefixed by >>):
> >
> > >> rxtimestamp.c:102:6: error: use of undeclared identifier 'SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER'
> >      102 |                         | SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER },
> >          |                           ^
> > >> rxtimestamp.c:373:20: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
> >      373 |                         for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
> >          |                                         ^~~~~~~~~~~~~~~~~~~~~~
> >    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
> >       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> >          |                                ^~~~~
> >    rxtimestamp.c:380:13: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
> >      380 |                         if (t >= ARRAY_SIZE(test_cases))
> >          |                                  ^~~~~~~~~~~~~~~~~~~~~~
> >    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
> >       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> >          |                                ^~~~~
> >    rxtimestamp.c:419:19: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
> >      419 |                 for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
> >          |                                 ^~~~~~~~~~~~~~~~~~~~~~
> >    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
> >       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> >          |                                ^~~~~
> >    4 errors generated.
> 
> I didn't get how it happened? I've already test it locally.
> 
> Is it because the test environment didn't update the header files by
> using the command like "make headers_install && cp -r
> usr/include/linux /usr/include/"?

Sorry about the false report, kindly ignore this. And thanks for the hint,
we will check to understand the root cause of this wrong report and fix the
bot asap.

> 
> If the applications or some userspace tools try to use the new flag,
> it should update the header file to sync the uapi file first.
> 
> Thanks,
> Jason
> 

Re: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER

[Jason Xing]

On Mon, Sep 2, 2024 at 12:25 PM Philip Li <philip.li@intel.com> wrote:
>
> On Mon, Sep 02, 2024 at 10:41:28AM +0800, Jason Xing wrote:
> > On Mon, Sep 2, 2024 at 9:49 AM kernel test robot <lkp@intel.com> wrote:
> > >
> > > Hi Jason,
> > >
> > > kernel test robot noticed the following build errors:
> > >
> > > [auto build test ERROR on net-next/main]
> > >
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Xing/net-timestamp-filter-out-report-when-setting-SOF_TIMESTAMPING_SOFTWARE/20240830-234014
> > > base:   net-next/main
> > > patch link:    https://lore.kernel.org/r/20240830153751.86895-3-kerneljasonxing%40gmail.com
> > > patch subject: [PATCH net-next v3 2/2] rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER
> > > :::::: branch date: 2 days ago
> > > :::::: commit date: 2 days ago
> > > compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240902/202409020124.YybQQDrP-lkp@intel.com/reproduce)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <lkp@intel.com>
> > > | Closes: https://lore.kernel.org/r/202409020124.YybQQDrP-lkp@intel.com/
> > >
> > > All errors (new ones prefixed by >>):
> > >
> > > >> rxtimestamp.c:102:6: error: use of undeclared identifier 'SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER'
> > >      102 |                         | SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER },
> > >          |                           ^
> > > >> rxtimestamp.c:373:20: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
> > >      373 |                         for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
> > >          |                                         ^~~~~~~~~~~~~~~~~~~~~~
> > >    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
> > >       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> > >          |                                ^~~~~
> > >    rxtimestamp.c:380:13: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
> > >      380 |                         if (t >= ARRAY_SIZE(test_cases))
> > >          |                                  ^~~~~~~~~~~~~~~~~~~~~~
> > >    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
> > >       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> > >          |                                ^~~~~
> > >    rxtimestamp.c:419:19: error: invalid application of 'sizeof' to an incomplete type 'struct test_case[]'
> > >      419 |                 for (t = 0; t < ARRAY_SIZE(test_cases); t++) {
> > >          |                                 ^~~~~~~~~~~~~~~~~~~~~~
> > >    ./../kselftest.h:61:32: note: expanded from macro 'ARRAY_SIZE'
> > >       61 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> > >          |                                ^~~~~
> > >    4 errors generated.
> >
> > I didn't get how it happened? I've already test it locally.
> >
> > Is it because the test environment didn't update the header files by
> > using the command like "make headers_install && cp -r
> > usr/include/linux /usr/include/"?
>
> Sorry about the false report, kindly ignore this. And thanks for the hint,
> we will check to understand the root cause of this wrong report and fix the
> bot asap.

That's all right :)

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

[Jakub Kicinski]

On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> +	if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> +	    val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> +		return -EINVAL;


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

>  	memset(&tss, 0, sizeof(tss));
>  	tsflags = READ_ONCE(sk->sk_tsflags);
> -	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> +	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +	     (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> +	     skb_is_err_queue(skb) ||
> +	     !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER))) &&

Willem, do you prefer to keep the:

	tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)

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

	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)

is sufficient (and, subjectively, more intuitive).

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

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

[Willem de Bruijn]

Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > +	if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > +	    val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > +		return -EINVAL;
> 
> 
> > -		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > +		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +		    (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > +		     !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> >  			has_timestamping = true;
> >  		else
> >  			tss->ts[0] = (struct timespec64) {0};
> >  	}
> 
> >  	memset(&tss, 0, sizeof(tss));
> >  	tsflags = READ_ONCE(sk->sk_tsflags);
> > -	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> > +	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +	     (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > +	     skb_is_err_queue(skb) ||
> > +	     !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER))) &&
> 
> Willem, do you prefer to keep the:
> 
> 	tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> 	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> 
> conditions?IIUC we prevent both from being set at once. So 
> 
> 	!(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> 
> is sufficient (and, subjectively, more intuitive).

Good point. Yes, let's definitely simplify.

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

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

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

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

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

and same for tcp_recv_timestamp.

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

[Jason Xing]

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

Will do it.

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

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

Thanks,
Jason

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

[Jason Xing]

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

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

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

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

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

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

Thanks,
Jason

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

[Willem de Bruijn]

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

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

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

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


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

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

Unless I'm missing something. But I think the author of that statement
is the one who made the mistake. Who is.. also me.

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

[Jason Xing]

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

That's all right :)

Then, the OPT_RX_FILTER is fine under this circumstance.

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

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

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

Thanks for your help!