[PATCH net-next v4 0/4] net-timestamp: introduce a flag to filter out rx software and hardware report
From: Jason Xing kernelxing@tencent.com
When one socket is set SOF_TIMESTAMPING_RX_SOFTWARE which means the whole system turns on the netstamp_needed_key button, other sockets that only have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx timestamp information even without setting SOF_TIMESTAMPING_RX_SOFTWARE generation flag.
How to solve it without breaking users? We introduce a new flag named SOF_TIMESTAMPING_OPT_RX_FILTER. Using it together with SOF_TIMESTAMPING_SOFTWARE can stop reporting the rx software timestamp.
Similarly, we also filter out the hardware case where one process enables the rx hardware generation flag, then another process only passing SOF_TIMESTAMPING_RAW_HARDWARE gets the timestamp. So we can set both SOF_TIMESTAMPING_RAW_HARDWARE and SOF_TIMESTAMPING_OPT_RX_FILTER to stop reporting rx hardware timestamp after this patch applied.
v4 Link: https://lore.kernel.org/all/20240830153751.86895-1-kerneljasonxing@gmail.com... 1. revise the doc and commit message (Willem) 2. add patch [2/4] to make the doc right (Willem) 3. add patch [3/4] to cover the hardware use (Willem) 4. add testcase for hardware use. Note: the reason why I split into 4 patches is try to make each commit clean, atomic, easy to review.
v3 Link: https://lore.kernel.org/all/20240828160145.68805-1-kerneljasonxing@gmail.com... 1. introduce a new flag to avoid application breakage, suggested by Willem. 2. add it into the selftests.
v2 Link: https://lore.kernel.org/all/20240825152440.93054-1-kerneljasonxing@gmail.com... Discussed with Willem 1. update the documentation accordingly 2. add more comments in each patch 3. remove the previous test statements in __sock_recv_timestamp()
Jason Xing (4): net-timestamp: filter out report when setting SOF_TIMESTAMPING_SOFTWARE net-timestamp: correct the use of SOF_TIMESTAMPING_RAW_HARDWARE net-timestamp: extend SOF_TIMESTAMPING_OPT_RX_FILTER for hardware use rxtimestamp.c: add the test for SOF_TIMESTAMPING_OPT_RX_FILTER
Documentation/networking/timestamping.rst | 18 +++++++++++++++++- include/uapi/linux/net_tstamp.h | 3 ++- net/core/sock.c | 5 +++++ net/ethtool/common.c | 1 + net/ipv4/tcp.c | 7 +++++-- net/socket.c | 7 +++++-- tools/testing/selftests/net/rxtimestamp.c | 11 +++++++++++ 7 files changed, 46 insertions(+), 6 deletions(-)
From: Jason Xing kernelxing@tencent.com
introduce a new flag SOF_TIMESTAMPING_OPT_RX_FILTER in the receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter out rx software timestamp report, especially after a process turns on netstamp_needed_key which can time stamp every incoming skb.
Previously, we found out if an application starts first which turns on netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE could also get rx timestamp. Now we handle this case by introducing this new flag without breaking users.
Quoting Willem to explain why we need the flag: "why a process would want to request software timestamp reporting, but not receive software timestamp generation. The only use I see is when the application does request SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE."
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 software timestamp report. 2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_FILTER while the skb is timestamped, it will stop reporting the rx software timestamp.
Another thing about errqueue in this patch I have a few words to say: In this case, we need to handle the egress path carefully, or else reporting the tx timestamp will fail. Egress path and ingress path will finally call sock_recv_timestamp(). We have to distinguish them. Errqueue is a good indicator to reflect the flow direction.
Suggested-by: Willem de Bruijn willemb@google.com Reviewed-by: Willem de Bruijn willemb@google.com Signed-off-by: Jason Xing kernelxing@tencent.com --- v4 Link: https://lore.kernel.org/all/20240830153751.86895-2-kerneljasonxing@gmail.com... 1. revise the commit message and doc (Willem) 2. simplify the test statement (Jakub) 3. add Willem's reviewed-by tag (Willem)
v3 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 | 6 ++++-- net/socket.c | 4 +++- 6 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 5e93cd71f99f..37ead02be3b1 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_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_FILTER prevents the application from being + influenced by others and let the application 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..858339d1c1c4 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_FILTER = (1 << 17),
- SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_ID_TCP, + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_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..6a93344e21cf 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_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 781834ef57c3..6c245e59bbc1 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -427,6 +427,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_FILTER)] = "option-rx-filter", }; static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT);
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 8a5680b4e786..a0c57c8b77bd 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,15 @@ 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_OPT_RX_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..f8609d649ed3 100644 --- a/net/socket.c +++ b/net/socket.c @@ -946,7 +946,9 @@ 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 && + (skb_is_err_queue(skb) || + !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) && ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0)) empty = 0; if (shhwtstamps &&
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
introduce a new flag SOF_TIMESTAMPING_OPT_RX_FILTER in the receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter out rx software timestamp report, especially after a process turns on netstamp_needed_key which can time stamp every incoming skb.
Previously, we found out if an application starts first which turns on netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE could also get rx timestamp. Now we handle this case by introducing this new flag without breaking users.
Quoting Willem to explain why we need the flag: "why a process would want to request software timestamp reporting, but not receive software timestamp generation. The only use I see is when the application does request SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE."
In this way, we have two kinds of combination:
- setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
will surely allow users to get the rx software timestamp report. 2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_FILTER while the skb is timestamped, it will stop reporting the rx software timestamp.
Another thing about errqueue in this patch I have a few words to say: In this case, we need to handle the egress path carefully, or else reporting the tx timestamp will fail. Egress path and ingress path will finally call sock_recv_timestamp(). We have to distinguish them. Errqueue is a good indicator to reflect the flow direction.
Suggested-by: Willem de Bruijn willemb@google.com Reviewed-by: Willem de Bruijn willemb@google.com
nit: Reviewed-by tags are only sticky if no changes are made.
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 5e93cd71f99f..37ead02be3b1 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_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.
This raises the question: why would a process request report flag SOF_TIMESTAMPING_SOFTWARE without generate flag SOF_TIMESTAMPING_RX_SOFTWARE? The only sensible use case I see is when it sets SOF_TIMSETAMPING_TX_SOFTWARE. Probably good to mention that.
May also be good to mention that existing applications sometimes set SOF_TIMESTAMPING_SOFTWARE only, because they implicitly came to depend on another (usually daemon) process to enable rx timestamps systemwide.
- SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being
- influenced by others and let the application choose whether to report
- the timestamp in the receive path or not.
I'd drop this paragraph. It's more of a value statement.
On Thu, Sep 5, 2024 at 9:37 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_FILTER in the receive path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter out rx software timestamp report, especially after a process turns on netstamp_needed_key which can time stamp every incoming skb.
Previously, we found out if an application starts first which turns on netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE could also get rx timestamp. Now we handle this case by introducing this new flag without breaking users.
Quoting Willem to explain why we need the flag: "why a process would want to request software timestamp reporting, but not receive software timestamp generation. The only use I see is when the application does request SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE."
In this way, we have two kinds of combination:
- setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
will surely allow users to get the rx software timestamp report. 2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_OPT_RX_FILTER while the skb is timestamped, it will stop reporting the rx software timestamp.
Another thing about errqueue in this patch I have a few words to say: In this case, we need to handle the egress path carefully, or else reporting the tx timestamp will fail. Egress path and ingress path will finally call sock_recv_timestamp(). We have to distinguish them. Errqueue is a good indicator to reflect the flow direction.
Suggested-by: Willem de Bruijn willemb@google.com Reviewed-by: Willem de Bruijn willemb@google.com
nit: Reviewed-by tags are only sticky if no changes are made.
I got it. I will remove it.
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 5e93cd71f99f..37ead02be3b1 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_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.
This raises the question: why would a process request report flag SOF_TIMESTAMPING_SOFTWARE without generate flag SOF_TIMESTAMPING_RX_SOFTWARE? The only sensible use case I see is when it sets SOF_TIMSETAMPING_TX_SOFTWARE. Probably good to mention that.
May also be good to mention that existing applications sometimes set SOF_TIMESTAMPING_SOFTWARE only, because they implicitly came to depend on another (usually daemon) process to enable rx timestamps systemwide.
Much better. Thanks. I will add them too.
- SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being
- influenced by others and let the application choose whether to report
- the timestamp in the receive path or not.
I'd drop this paragraph. It's more of a value statement.
I see. Will drop it.
Thanks, Jason
From: Jason Xing kernelxing@tencent.com
SOF_TIMESTAMPING_RAW_HARDWARE is a report flag which passes the timestamps generated by either SOF_TIMESTAMPING_TX_HARDWARE or SOF_TIMESTAMPING_RX_HARDWARE to the userspace all the time.
So let us revise the doc here.
Suggested-by: Willem de Bruijn willemdebruijn.kernel@gmail.com Signed-off-by: Jason Xing kernelxing@tencent.com --- Link: https://lore.kernel.org/all/66d8c21d3042a_163d93294cb@willemb.c.googlers.com... --- Documentation/networking/timestamping.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 37ead02be3b1..ac57d9de2f11 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -158,7 +158,8 @@ SOF_TIMESTAMPING_SYS_HARDWARE:
SOF_TIMESTAMPING_RAW_HARDWARE: Report hardware timestamps as generated by - SOF_TIMESTAMPING_TX_HARDWARE when available. + SOF_TIMESTAMPING_TX_HARDWARE or SOF_TIMESTAMPING_RX_HARDWARE + when available.
1.3.3 Timestamp Options
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
SOF_TIMESTAMPING_RAW_HARDWARE is a report flag which passes the timestamps generated by either SOF_TIMESTAMPING_TX_HARDWARE or SOF_TIMESTAMPING_RX_HARDWARE to the userspace all the time.
So let us revise the doc here.
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
As an exception to the rule, as a one word fix, can be squashed into the feature patch, I think.
Link: https://lore.kernel.org/all/66d8c21d3042a_163d93294cb@willemb.c.googlers.com...
Please put these at the top of the Suggested-by/Signed-off-by/..-by block. Their more useful to future readers than to current followers of the mailing list.
On Thu, Sep 5, 2024 at 9:38 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
SOF_TIMESTAMPING_RAW_HARDWARE is a report flag which passes the timestamps generated by either SOF_TIMESTAMPING_TX_HARDWARE or SOF_TIMESTAMPING_RX_HARDWARE to the userspace all the time.
So let us revise the doc here.
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
As an exception to the rule, as a one word fix, can be squashed into the feature patch, I think.
Link: https://lore.kernel.org/all/66d8c21d3042a_163d93294cb@willemb.c.googlers.com...
Please put these at the top of the Suggested-by/Signed-off-by/..-by block. Their more useful to future readers than to current followers of the mailing list.
But I have to ask if it looks a little messy if the first patch will grow much bigger than before with adding some suggested-by tags and links, and a separate paragraph to explain why we touch the doc...
Cooking one patch which saves my much energy is surely easier for me but not for readers probably, I assume.
Thanks, Jason
Jason Xing wrote:
On Thu, Sep 5, 2024 at 9:38 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
SOF_TIMESTAMPING_RAW_HARDWARE is a report flag which passes the timestamps generated by either SOF_TIMESTAMPING_TX_HARDWARE or SOF_TIMESTAMPING_RX_HARDWARE to the userspace all the time.
So let us revise the doc here.
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
As an exception to the rule, as a one word fix, can be squashed into the feature patch, I think.
Link: https://lore.kernel.org/all/66d8c21d3042a_163d93294cb@willemb.c.googlers.com...
Please put these at the top of the Suggested-by/Signed-off-by/..-by block. Their more useful to future readers than to current followers of the mailing list.
But I have to ask if it looks a little messy if the first patch will grow much bigger than before with adding some suggested-by tags and links, and a separate paragraph to explain why we touch the doc...
Cooking one patch which saves my much energy is surely easier for me but not for readers probably, I assume.
Ok. This small patch can even be stand-alone. No need for a series.
On Thu, Sep 5, 2024 at 10:45 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Thu, Sep 5, 2024 at 9:38 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
SOF_TIMESTAMPING_RAW_HARDWARE is a report flag which passes the timestamps generated by either SOF_TIMESTAMPING_TX_HARDWARE or SOF_TIMESTAMPING_RX_HARDWARE to the userspace all the time.
So let us revise the doc here.
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
As an exception to the rule, as a one word fix, can be squashed into the feature patch, I think.
Link: https://lore.kernel.org/all/66d8c21d3042a_163d93294cb@willemb.c.googlers.com...
Please put these at the top of the Suggested-by/Signed-off-by/..-by block. Their more useful to future readers than to current followers of the mailing list.
But I have to ask if it looks a little messy if the first patch will grow much bigger than before with adding some suggested-by tags and links, and a separate paragraph to explain why we touch the doc...
Cooking one patch which saves my much energy is surely easier for me but not for readers probably, I assume.
Ok. This small patch can even be stand-alone. No need for a series.
Got it. I will send it as a standalone patch tomorrow since I need to wait more than 24 hours.
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com --- Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/ --- Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp.
SOF_TIMESTAMPING_OPT_RX_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. + Used in the receive software/hardware timestamp. Enabling the flag + along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE + will not report the rx timestamp to the userspace so that it can + filter out the cases where 1) one process starts first which turns + on netstamp_needed_key through setting generation flags like + SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables + generating the hardware timestamp already, then another one only + passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the + rx timestamp.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ 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_FILTER) + if (val & SOF_TIMESTAMPING_OPT_RX_FILTER && + (val & SOF_TIMESTAMPING_RX_SOFTWARE || + val & SOF_TIMESTAMPING_RX_HARDWARE)) return -EINVAL;
if (val & SOF_TIMESTAMPING_OPT_ID && diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index a0c57c8b77bd..23f0722aa801 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2283,7 +2283,8 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk, }
if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) { - if (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) + if (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE && + !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER)) has_timestamping = true; else tss->ts[2] = (struct timespec64) {0}; diff --git a/net/socket.c b/net/socket.c index f8609d649ed3..bfbae2069fbb 100644 --- a/net/socket.c +++ b/net/socket.c @@ -952,7 +952,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0)) empty = 0; if (shhwtstamps && - (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && + (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE && + !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER)) && !skb_is_swtx_tstamp(skb, false_tstamp)) { if_index = 0; if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com
Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp. SOF_TIMESTAMPING_OPT_RX_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.
- Used in the receive software/hardware timestamp. Enabling the flag
- along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
- will not report the rx timestamp to the userspace so that it can
- filter out the cases where 1) one process starts first which turns
- on netstamp_needed_key through setting generation flags like
- SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
- generating the hardware timestamp already, then another one only
- passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
- rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors across its lifetime. Even if it is only two SHA1s apart.
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ 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_FILTER)
- if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&
(val & SOF_TIMESTAMPING_RX_SOFTWARE ||val & SOF_TIMESTAMPING_RX_HARDWARE))
There may be legitimate use cases of wanting to receive hardware receive timestamps, plus software transmit timestamp, but suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \ SOF_TIMESTAMPING_RX_HARDWARE | \ SOF_TIMESTAMPING_SOFTWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_OPT_RX_FILTER
Admittedly this seems a bit contrived. But it's little hassle to support it?
We just can no longer use the branch simplification that Jakub pointed out.
On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com
Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp.
SOF_TIMESTAMPING_OPT_RX_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.
- Used in the receive software/hardware timestamp. Enabling the flag
- along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
- will not report the rx timestamp to the userspace so that it can
- filter out the cases where 1) one process starts first which turns
- on netstamp_needed_key through setting generation flags like
- SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
- generating the hardware timestamp already, then another one only
- passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
- rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors across its lifetime. Even if it is only two SHA1s apart.
I thought about last night as well. Like the patch [2/4] and this patch, the reason why I wanted to split is because I have to explain a lot for both hw and sw in one patch. One patch mixes different things.
No strong preference. If you still think so, I definitely can squash them as you said :)
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ 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_FILTER)
if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&(val & SOF_TIMESTAMPING_RX_SOFTWARE ||val & SOF_TIMESTAMPING_RX_HARDWARE)) return -EINVAL;There may be legitimate use cases of wanting to receive hardware receive timestamps, plus software transmit timestamp, but suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \ SOF_TIMESTAMPING_RX_HARDWARE | \ SOF_TIMESTAMPING_SOFTWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_OPT_RX_FILTER
Oh, right, it can happen! RAW_HARDWARE is a little bit different, covering both ingress and egress path.
Admittedly this seems a bit contrived. But it's little hassle to support it?
We just can no longer use the branch simplification that Jakub pointed out.
I see. I'm going to do two things as you said: 1) restore the simplification branch 2) only take care of software case in sock_set_timestamping()
Thanks for pointing this out!
Jason Xing wrote:
On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com
Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp.
SOF_TIMESTAMPING_OPT_RX_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.
- Used in the receive software/hardware timestamp. Enabling the flag
- along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
- will not report the rx timestamp to the userspace so that it can
- filter out the cases where 1) one process starts first which turns
- on netstamp_needed_key through setting generation flags like
- SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
- generating the hardware timestamp already, then another one only
- passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
- rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors across its lifetime. Even if it is only two SHA1s apart.
I thought about last night as well. Like the patch [2/4] and this patch, the reason why I wanted to split is because I have to explain a lot for both hw and sw in one patch. One patch mixes different things.
No strong preference. If you still think so, I definitely can squash them as you said :)
No strong preference on 2/4. See other reply.
In this case, patch 1/4 introduces some behavior and 3/4 immediately updates it. I think it makes more sense to combine them.
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ 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_FILTER)
if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&(val & SOF_TIMESTAMPING_RX_SOFTWARE ||val & SOF_TIMESTAMPING_RX_HARDWARE)) return -EINVAL;There may be legitimate use cases of wanting to receive hardware receive timestamps, plus software transmit timestamp, but suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \ SOF_TIMESTAMPING_RX_HARDWARE | \ SOF_TIMESTAMPING_SOFTWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_OPT_RX_FILTEROh, right, it can happen! RAW_HARDWARE is a little bit different, covering both ingress and egress path.
As said, it is a bit contrived. Feel free to disagree and keep as is too.
Admittedly this seems a bit contrived. But it's little hassle to support it?
We just can no longer use the branch simplification that Jakub pointed out.
I see. I'm going to do two things as you said:
- restore the simplification branch
- only take care of software case in sock_set_timestamping()
Thanks for pointing this out!
On Thu, Sep 5, 2024 at 10:46 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com
Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp.
SOF_TIMESTAMPING_OPT_RX_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.
- Used in the receive software/hardware timestamp. Enabling the flag
- along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
- will not report the rx timestamp to the userspace so that it can
- filter out the cases where 1) one process starts first which turns
- on netstamp_needed_key through setting generation flags like
- SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
- generating the hardware timestamp already, then another one only
- passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
- rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors across its lifetime. Even if it is only two SHA1s apart.
I thought about last night as well. Like the patch [2/4] and this patch, the reason why I wanted to split is because I have to explain a lot for both hw and sw in one patch. One patch mixes different things.
No strong preference. If you still think so, I definitely can squash them as you said :)
No strong preference on 2/4. See other reply.
In this case, patch 1/4 introduces some behavior and 3/4 immediately updates it. I think it makes more sense to combine them.
Roger that. Will squash this one:)
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ 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_FILTER)
if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&(val & SOF_TIMESTAMPING_RX_SOFTWARE ||val & SOF_TIMESTAMPING_RX_HARDWARE)) return -EINVAL;There may be legitimate use cases of wanting to receive hardware receive timestamps, plus software transmit timestamp, but suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \ SOF_TIMESTAMPING_RX_HARDWARE | \ SOF_TIMESTAMPING_SOFTWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_OPT_RX_FILTER
Sorry, I think my initial understanding at first read is not right. I was thinking you want this combination to pass the check in sock_set_timestamping().
If the users insist on receiving "hardware receive timestamps" with OPT_RX_FILTER enabled in this case, I think we should implement another new flag, say, OPT_RX_HARDWARE_FILTER...
Oh, right, it can happen! RAW_HARDWARE is a little bit different, covering both ingress and egress path.
As said, it is a bit contrived. Feel free to disagree and keep as is too.
Well, I can keep it as is. It's easy for me, saving much energy, but...you already pointed out/ noticed this kind of use case that is not invalid.
If we want to tackle it well, we need to add a new flag for the hardware case, then we can individually control each of them, which is a more fine-grained control honestly. I'm totally fine with it as long as it will be good for users in the long run :)
If so, adding a new patch into this series (like patch [3/4]) seems inevitable. It won't take much time, I feel.
Any further thoughts?
Thanks, Jason
Jason Xing wrote:
On Thu, Sep 5, 2024 at 10:46 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com
Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp.
SOF_TIMESTAMPING_OPT_RX_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.
- Used in the receive software/hardware timestamp. Enabling the flag
- along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
- will not report the rx timestamp to the userspace so that it can
- filter out the cases where 1) one process starts first which turns
- on netstamp_needed_key through setting generation flags like
- SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
- generating the hardware timestamp already, then another one only
- passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
- rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors across its lifetime. Even if it is only two SHA1s apart.
I thought about last night as well. Like the patch [2/4] and this patch, the reason why I wanted to split is because I have to explain a lot for both hw and sw in one patch. One patch mixes different things.
No strong preference. If you still think so, I definitely can squash them as you said :)
No strong preference on 2/4. See other reply.
In this case, patch 1/4 introduces some behavior and 3/4 immediately updates it. I think it makes more sense to combine them.
Roger that. Will squash this one:)
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ 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_FILTER)
if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&(val & SOF_TIMESTAMPING_RX_SOFTWARE ||val & SOF_TIMESTAMPING_RX_HARDWARE)) return -EINVAL;There may be legitimate use cases of wanting to receive hardware receive timestamps, plus software transmit timestamp, but suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \ SOF_TIMESTAMPING_RX_HARDWARE | \ SOF_TIMESTAMPING_SOFTWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_OPT_RX_FILTERSorry, I think my initial understanding at first read is not right. I was thinking you want this combination to pass the check in sock_set_timestamping().
If the users insist on receiving "hardware receive timestamps" with OPT_RX_FILTER enabled in this case, I think we should implement another new flag, say, OPT_RX_HARDWARE_FILTER...
My interpretation of the OPT_RX_FILTER flag is:
Only return RX timestamps if the socket also has the corresponding reporting flag set.
So it is valid to have
SOF_TIMESTAMPING_RAW_HARDWARE | SOF_TIMESTAMPING_RX_HARDWARE | SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_OPT_RX_FILTER
To filter out the software rx timestamps, but let through the hardware rx timestamps.
Oh, right, it can happen! RAW_HARDWARE is a little bit different, covering both ingress and egress path.
As said, it is a bit contrived. Feel free to disagree and keep as is too.
Well, I can keep it as is. It's easy for me, saving much energy, but...you already pointed out/ noticed this kind of use case that is not invalid.
If we want to tackle it well, we need to add a new flag for the hardware case, then we can individually control each of them, which is a more fine-grained control honestly. I'm totally fine with it as long as it will be good for users in the long run :)
If so, adding a new patch into this series (like patch [3/4]) seems inevitable. It won't take much time, I feel.
Any further thoughts?
Thanks, Jason
On Fri, Sep 6, 2024 at 12:41 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Thu, Sep 5, 2024 at 10:46 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
In the previous patch, we found things could happen in the rx software timestamp. Here, we also noticed that, for rx hardware timestamp case, it could happen when one process enables the rx hardware timestamp generating flag first, then another process only setting SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware timestamp.
In this patch, we extend the OPT_RX_FILTER flag to filter out the above case for hardware use.
Suggested-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Jason Xing kernelxing@tencent.com
Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
Documentation/networking/timestamping.rst | 15 +++++++++------ net/core/sock.c | 5 +++-- net/ipv4/tcp.c | 3 ++- net/socket.c | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index ac57d9de2f11..55e79ea71f3e 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW: each containing just one timestamp.
SOF_TIMESTAMPING_OPT_RX_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.
- Used in the receive software/hardware timestamp. Enabling the flag
- along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
- will not report the rx timestamp to the userspace so that it can
- filter out the cases where 1) one process starts first which turns
- on netstamp_needed_key through setting generation flags like
- SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
- generating the hardware timestamp already, then another one only
- passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
- rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors across its lifetime. Even if it is only two SHA1s apart.
I thought about last night as well. Like the patch [2/4] and this patch, the reason why I wanted to split is because I have to explain a lot for both hw and sw in one patch. One patch mixes different things.
No strong preference. If you still think so, I definitely can squash them as you said :)
No strong preference on 2/4. See other reply.
In this case, patch 1/4 introduces some behavior and 3/4 immediately updates it. I think it makes more sense to combine them.
Roger that. Will squash this one:)
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being influenced by others and let the application choose whether to report diff --git a/net/core/sock.c b/net/core/sock.c index 6a93344e21cf..dc4a43cfff59 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -908,8 +908,9 @@ 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_FILTER)
if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&(val & SOF_TIMESTAMPING_RX_SOFTWARE ||val & SOF_TIMESTAMPING_RX_HARDWARE)) return -EINVAL;There may be legitimate use cases of wanting to receive hardware receive timestamps, plus software transmit timestamp, but suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \ SOF_TIMESTAMPING_RX_HARDWARE | \ SOF_TIMESTAMPING_SOFTWARE | \ SOF_TIMESTAMPING_TX_SOFTWARE | \ SOF_TIMESTAMPING_OPT_RX_FILTERSorry, I think my initial understanding at first read is not right. I was thinking you want this combination to pass the check in sock_set_timestamping().
If the users insist on receiving "hardware receive timestamps" with OPT_RX_FILTER enabled in this case, I think we should implement another new flag, say, OPT_RX_HARDWARE_FILTER...
My interpretation of the OPT_RX_FILTER flag is:
Only return RX timestamps if the socket also has the corresponding reporting flag set.
So it is valid to have
SOF_TIMESTAMPING_RAW_HARDWARE | SOF_TIMESTAMPING_RX_HARDWARE | SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_OPT_RX_FILTER
To filter out the software rx timestamps, but let through the hardware rx timestamps.
I see. Thanks for your advice.
If both SOF_TIMESTAMPING_RX_SOFTWARE and SOF_TIMESTAMPING_OPT_RX_FILTER are set at once, the latter will not take any effect.
I will 1) completely remove the limitation in sock_set_timestamping(), 2) restore those simplification branches.
From: Jason Xing kernelxing@tencent.com
Test when we use SOF_TIMESTAMPING_OPT_RX_FILTER with software or hardware report flag. The expected result is no rx timestamp report.
Reviewed-by: Willem de Bruijn willemb@google.com Signed-off-by: Jason Xing kernelxing@tencent.com --- tools/testing/selftests/net/rxtimestamp.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/testing/selftests/net/rxtimestamp.c b/tools/testing/selftests/net/rxtimestamp.c index 9eb42570294d..9760abdb6e05 100644 --- a/tools/testing/selftests/net/rxtimestamp.c +++ b/tools/testing/selftests/net/rxtimestamp.c @@ -57,6 +57,7 @@ static struct sof_flag sof_flags[] = { SOF_FLAG(SOF_TIMESTAMPING_SOFTWARE), SOF_FLAG(SOF_TIMESTAMPING_RX_SOFTWARE), SOF_FLAG(SOF_TIMESTAMPING_RX_HARDWARE), + SOF_FLAG(SOF_TIMESTAMPING_OPT_RX_FILTER), };
static struct socket_type socket_types[] = { @@ -97,6 +98,16 @@ static struct test_case test_cases[] = { | SOF_TIMESTAMPING_RX_HARDWARE }, {} }, + { + { .so_timestamping = SOF_TIMESTAMPING_RAW_HARDWARE + | SOF_TIMESTAMPING_OPT_RX_FILTER }, + {} + }, + { + { .so_timestamping = SOF_TIMESTAMPING_SOFTWARE + | SOF_TIMESTAMPING_OPT_RX_FILTER }, + {} + }, { { .so_timestamping = SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_RX_SOFTWARE },
On Thu, Sep 5, 2024 at 3:18 PM Jason Xing kerneljasonxing@gmail.com wrote:
From: Jason Xing kernelxing@tencent.com
Test when we use SOF_TIMESTAMPING_OPT_RX_FILTER with software or hardware report flag. The expected result is no rx timestamp report.
Reviewed-by: Willem de Bruijn willemb@google.com Signed-off-by: Jason Xing kernelxing@tencent.com
tools/testing/selftests/net/rxtimestamp.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/testing/selftests/net/rxtimestamp.c b/tools/testing/selftests/net/rxtimestamp.c index 9eb42570294d..9760abdb6e05 100644 --- a/tools/testing/selftests/net/rxtimestamp.c +++ b/tools/testing/selftests/net/rxtimestamp.c @@ -57,6 +57,7 @@ static struct sof_flag sof_flags[] = { SOF_FLAG(SOF_TIMESTAMPING_SOFTWARE), SOF_FLAG(SOF_TIMESTAMPING_RX_SOFTWARE), SOF_FLAG(SOF_TIMESTAMPING_RX_HARDWARE),
SOF_FLAG(SOF_TIMESTAMPING_OPT_RX_FILTER),
Ah.. I missed adding SOF_TIMESTAMPING_RAW_HARDWARE :S
I'll repost it in 24 hour.
};
static struct socket_type socket_types[] = { @@ -97,6 +98,16 @@ static struct test_case test_cases[] = { | SOF_TIMESTAMPING_RX_HARDWARE }, {} },
{{ .so_timestamping = SOF_TIMESTAMPING_RAW_HARDWARE| SOF_TIMESTAMPING_OPT_RX_FILTER },{}},{{ .so_timestamping = SOF_TIMESTAMPING_SOFTWARE| SOF_TIMESTAMPING_OPT_RX_FILTER },{}}, { { .so_timestamping = SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_RX_SOFTWARE },-- 2.37.3
linux-kselftest-mirror@lists.linaro.org
-
Jason Xing -
Willem de Bruijn