[PATCH net-next v5 0/2] 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.
v5 Link: https://lore.kernel.org/all/20240905071738.3725-1-kerneljasonxing@gmail.com/ 1. squash the hardware case patch into this one (Willem) 2. update corresponding commit message and doc (Willem) 3. remove the limitation in sock_set_timestamping() and restore the simplification branches. (Willem) 4. add missing type and another test in selftests
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 (2): net-timestamp: introduce SOF_TIMESTAMPING_OPT_RX_FILTER flag net-timestamp: add selftests for SOF_TIMESTAMPING_OPT_RX_FILTER
Documentation/networking/timestamping.rst | 27 +++++++++++++++++++++++ include/uapi/linux/net_tstamp.h | 3 ++- net/ethtool/common.c | 1 + net/ipv4/tcp.c | 9 ++++++-- net/socket.c | 10 +++++++-- tools/testing/selftests/net/rxtimestamp.c | 18 +++++++++++++++ 6 files changed, 63 insertions(+), 5 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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com --- v5 Link: https://lore.kernel.org/all/20240905071738.3725-1-kerneljasonxing@gmail.com/ 1. squash the hardware case patch into this one (Willem) 2. update corresponding commit message and doc (Willem) 3. remove the limitation in sock_set_timestamping() and restore the simplification branches. (Willem)
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 | 27 +++++++++++++++++++++++ include/uapi/linux/net_tstamp.h | 3 ++- net/ethtool/common.c | 1 + net/ipv4/tcp.c | 9 ++++++-- net/socket.c | 10 +++++++-- 5 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 9c7773271393..ae122ae3df72 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -267,6 +267,33 @@ 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 to filter out software/hardware receive timestamps. Existing + applications can 1) set SOF_TIMESTAMPING_SOFTWARE only, or 2) set + SOF_TIMESTAMPING_RAW_HARDWARE only because they implicitly came to + depend on another (usually daemon) process to enable receive + timestamps systemwide. Enabling the flag along with + SOF_TIMESTAMPING_SOFTWARE or SOF_TIMESTAMPING_RAW_HARDWARE will not + report the software or hardware receive 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 first like setting + SOF_TIMESTAMPING_RX_HARDWARE, then another one only passing report + flag could also get the receive timestamp. + + If the new applications 1) enable the flag along with + SOF_TIMESTAMPING_SOFTWARE and SOF_TIMESTAMPING_TX_SOFTWARE, or + 2) enable it with SOF_TIMESTAMPING_RAW_HARDWARE and + SOF_TIMESTAMPING_TX_HARDWARE, they will only receive software or + hardware transmit timestamp respectively. + + If the new applications 1) enable the flag along with + SOF_TIMESTAMPING_SOFTWARE and SOF_TIMESTAMPING_RX_SOFTWARE, or + 2) enable it with SOF_TIMESTAMPING_RAW_HARDWARE and + SOF_TIMESTAMPING_RX_HARDWARE, they will still receive software or + hardware receive timestamp respectively. + 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/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..e359a9161445 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,18 @@ 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_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 && + (tsflags & SOF_TIMESTAMPING_RX_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 fcbdd5bc47ac..1c2fd1a317af 100644 --- a/net/socket.c +++ b/net/socket.c @@ -946,11 +946,17 @@ 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_FILTER))) && 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_RX_HARDWARE || + skb_is_err_queue(skb) || + !(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
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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
Otherwise implementation looks fine, only the tiniest nit.
@@ -946,11 +946,17 @@ 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_FILTER))) &&
Nit: these statements should all align on the inner brace, so indent by one character.
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_RX_HARDWARE ||skb_is_err_queue(skb) ||!(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) && !skb_is_swtx_tstamp(skb, false_tstamp)) {-- 2.37.3
On Sat, Sep 7, 2024 at 7:24 AM 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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
You raised a good question.
I think It's more of a design consideration instead of a bugfix actually. So it is not solving a bug which makes the apps wrong but gives users a hint that we can explicitly and accurately do what we want and we expect.
Let's assume: if we remove all the report flags design, what will happen? It can work of course. I don't believe that people choose to enable the generation flag but are not willing to report it. Of course, It's another thing. I'm just saying.
I wonder if it makes sense to you :) ?
Otherwise implementation looks fine, only the tiniest nit.
@@ -946,11 +946,17 @@ 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_FILTER))) &&Nit: these statements should all align on the inner brace, so indent by one character.
I'm not that sure about the format, please help me to review here:
@@ -946,11 +946,17 @@ 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_FILTER))) && 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_RX_HARDWARE || + skb_is_err_queue(skb) || + !(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)
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_RX_HARDWARE ||
same here and the following two statements? Should I also indent by one char by the way?
skb_is_err_queue(skb) ||!(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)-- 2.37.3
On Sat, Sep 7, 2024 at 9:23 AM Jason Xing kerneljasonxing@gmail.com wrote:
On Sat, Sep 7, 2024 at 7:24 AM 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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
You raised a good question.
I think It's more of a design consideration instead of a bugfix actually. So it is not solving a bug which makes the apps wrong but gives users a hint that we can explicitly and accurately do what we want and we expect.
One more thing: if I recall correctly, the initial reason I proposed is that the rx report flag is not controlled under per socket but maybe affected by others. It's against the expectation.
Jason Xing wrote:
On Sat, Sep 7, 2024 at 7:24 AM 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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
You raised a good question.
I think It's more of a design consideration instead of a bugfix actually. So it is not solving a bug which makes the apps wrong but gives users a hint that we can explicitly and accurately do what we want and we expect.
Let's assume: if we remove all the report flags design, what will happen? It can work of course. I don't believe that people choose to enable the generation flag but are not willing to report it. Of course, It's another thing. I'm just saying.
Let's not debate the existing API. Its design predates both of our contributions.
I wonder if it makes sense to you :) ?
I don't see a strong use case. I know we're on v5 while I reopen that point, sorry.
It seems simpler to me to not read spurious fields that are not requested, rather than to explicitly request them to be set to zero.
Adding more flags is not free. An extra option adds mental load for casual users of this alread complex API. This may certainly sound hypocritical coming from me, as I added my fair share. But I hope their functionality outweighs that cost (well.. in at least one case it was an ugly fix for a bad first attempt.. OPT_ID).
I got to this point trying to condense the proposed documentation. We can add this if you feel strongly.
But then my main feedback is that the doc has to be shorter and to the point. Why would a user user this? No background on how we got here, what they might already do accidentally.
Otherwise implementation looks fine, only the tiniest nit.
@@ -946,11 +946,17 @@ 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_FILTER))) &&Nit: these statements should all align on the inner brace, so indent by one character.
I'm not that sure about the format, please help me to review here:
@@ -946,11 +946,17 @@ 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_FILTER))) && 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_RX_HARDWARE ||skb_is_err_queue(skb) ||!(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)
That's right
(A && (B || C))
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_RX_HARDWARE ||same here and the following two statements? Should I also indent by one char by the way?
Same rule on all new code introduced in the patch.
skb_is_err_queue(skb) ||!(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)-- 2.37.3
On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Sat, Sep 7, 2024 at 7:24 AM 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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
You raised a good question.
I think It's more of a design consideration instead of a bugfix actually. So it is not solving a bug which makes the apps wrong but gives users a hint that we can explicitly and accurately do what we want and we expect.
Let's assume: if we remove all the report flags design, what will happen? It can work of course. I don't believe that people choose to enable the generation flag but are not willing to report it. Of course, It's another thing. I'm just saying.
Let's not debate the existing API. Its design predates both of our contributions.
Yep.
I wonder if it makes sense to you :) ?
I don't see a strong use case. I know we're on v5 while I reopen that point, sorry.
That's all right. No worries.
It seems simpler to me to not read spurious fields that are not requested, rather than to explicitly request them to be set to zero.
Adding more flags is not free. An extra option adds mental load for casual users of this alread complex API. This may certainly sound hypocritical coming from me, as I added my fair share. But I hope their functionality outweighs that cost (well.. in at least one case it was an ugly fix for a bad first attempt.. OPT_ID).
I understand what you meant here. But I'm lost...
For some users, they might use the tsflags in apps to test whether they need to receive/report the rx timestamp or not, and someday they will notice there are unexpected timestamps that come out. As we know, it's more of a design consideration about whether the users can control it by setsockopt...
In addition to the design itself, above is the only use case I know.
I got to this point trying to condense the proposed documentation. We can add this if you feel strongly.
If the new flag is not good for future development, we can stop it and then _only_ document the special case, which we both agreed about a week ago.
Personally, I don't want to let it go easily. But It's just me. You are the maintainer, so you have to make the decision. I'm totally fine with either way. Thanks.
I was only trying to make the feature better. At least, we both have tried a lot.
But then my main feedback is that the doc has to be shorter and to
It's truly very long, to be honest. I thought I needed to list the possible combination use cases.
the point. Why would a user user this? No background on how we got here, what they might already do accidentally.
It looks like I should remove those use cases? And then clarify the reason is per socket control?
I have no idea if I should continue on this.
Thanks, Jason
Jason Xing wrote:
On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Sat, Sep 7, 2024 at 7:24 AM 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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
You raised a good question.
I think It's more of a design consideration instead of a bugfix actually. So it is not solving a bug which makes the apps wrong but gives users a hint that we can explicitly and accurately do what we want and we expect.
Let's assume: if we remove all the report flags design, what will happen? It can work of course. I don't believe that people choose to enable the generation flag but are not willing to report it. Of course, It's another thing. I'm just saying.
Let's not debate the existing API. Its design predates both of our contributions.
Yep.
I wonder if it makes sense to you :) ?
I don't see a strong use case. I know we're on v5 while I reopen that point, sorry.
That's all right. No worries.
It seems simpler to me to not read spurious fields that are not requested, rather than to explicitly request them to be set to zero.
Adding more flags is not free. An extra option adds mental load for casual users of this alread complex API. This may certainly sound hypocritical coming from me, as I added my fair share. But I hope their functionality outweighs that cost (well.. in at least one case it was an ugly fix for a bad first attempt.. OPT_ID).
I understand what you meant here. But I'm lost...
For some users, they might use the tsflags in apps to test whether they need to receive/report the rx timestamp or not, and someday they will notice there are unexpected timestamps that come out. As we know, it's more of a design consideration about whether the users can control it by setsockopt...
In addition to the design itself, above is the only use case I know.
Ok. I'm on the fence, but not a hard no. Evidently you see value, so others may too.
A pendantic use case is if the caller expects other cmsg, but not these. Then the amount of cmsg space used will depend on a third process enabling receive timestamps. Again, can be worked around. But admittedly is surprising behavior.
I got to this point trying to condense the proposed documentation. We can add this if you feel strongly.
If the new flag is not good for future development, we can stop it and then _only_ document the special case, which we both agreed about a week ago.
Personally, I don't want to let it go easily. But It's just me. You are the maintainer, so you have to make the decision. I'm totally fine with either way. Thanks.
I was only trying to make the feature better. At least, we both have tried a lot.
But then my main feedback is that the doc has to be shorter and to
It's truly very long, to be honest. I thought I needed to list the possible combination use cases.
the point. Why would a user user this? No background on how we got here, what they might already do accidentally.
It looks like I should remove those use cases? And then clarify the reason is per socket control?
I have no idea if I should continue on this.
My attempt to document, feel free to revise:
SOF_TIMESTAMPING_OPT_RX_FILTER:
Filter out spurious receive timestamps: report a receive timestamp only if the matching timestamp generation flag is enabled.
Receive timestamps are generated early in the ingress path, before a packet's destination socket is known. If any socket enables receive timestamps, packets for all socket will receive timestamped packets. Including those that request timestamp reporting with SOF_TIMESTAMPING_SOFTWARE and/or SOF_TIMESTAMPING_RAW_HARDWARE, but do not request receive timestamp generation. This can happen when requesting transmit timestamps only.
Receiving spurious timestamps is generally benign. A process can ignore the unexpected non-zero value. But it makes behavior subtly dependent on other sockets. This flag isolates the socket for more deterministic behavior.
Hello Willem,
On Mon, Sep 9, 2024 at 3:41 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Sat, Sep 7, 2024 at 10:34 AM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
On Sat, Sep 7, 2024 at 7:24 AM 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."
Similarly, this new flag could also be used for hardware case where we can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive hardware receive 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 Signed-off-by: Jason Xing kernelxing@tencent.com
High level: where is the harm in receiving unsolicited timestamps? A process can easily ignore them. I do wonder if the only use case is an overly strict testcase. Was reminded of this as I tried to write a more concise paragraph for the documentation.
You raised a good question.
I think It's more of a design consideration instead of a bugfix actually. So it is not solving a bug which makes the apps wrong but gives users a hint that we can explicitly and accurately do what we want and we expect.
Let's assume: if we remove all the report flags design, what will happen? It can work of course. I don't believe that people choose to enable the generation flag but are not willing to report it. Of course, It's another thing. I'm just saying.
Let's not debate the existing API. Its design predates both of our contributions.
Yep.
I wonder if it makes sense to you :) ?
I don't see a strong use case. I know we're on v5 while I reopen that point, sorry.
That's all right. No worries.
It seems simpler to me to not read spurious fields that are not requested, rather than to explicitly request them to be set to zero.
Adding more flags is not free. An extra option adds mental load for casual users of this alread complex API. This may certainly sound hypocritical coming from me, as I added my fair share. But I hope their functionality outweighs that cost (well.. in at least one case it was an ugly fix for a bad first attempt.. OPT_ID).
I understand what you meant here. But I'm lost...
For some users, they might use the tsflags in apps to test whether they need to receive/report the rx timestamp or not, and someday they will notice there are unexpected timestamps that come out. As we know, it's more of a design consideration about whether the users can control it by setsockopt...
In addition to the design itself, above is the only use case I know.
Ok. I'm on the fence, but not a hard no. Evidently you see value, so others may too.
A pendantic use case is if the caller expects other cmsg, but not these. Then the amount of cmsg space used will depend on a third process enabling receive timestamps. Again, can be worked around. But admittedly is surprising behavior.
I got to this point trying to condense the proposed documentation. We can add this if you feel strongly.
If the new flag is not good for future development, we can stop it and then _only_ document the special case, which we both agreed about a week ago.
Personally, I don't want to let it go easily. But It's just me. You are the maintainer, so you have to make the decision. I'm totally fine with either way. Thanks.
I was only trying to make the feature better. At least, we both have tried a lot.
But then my main feedback is that the doc has to be shorter and to
It's truly very long, to be honest. I thought I needed to list the possible combination use cases.
the point. Why would a user user this? No background on how we got here, what they might already do accidentally.
It looks like I should remove those use cases? And then clarify the reason is per socket control?
I have no idea if I should continue on this.
My attempt to document, feel free to revise:
SOF_TIMESTAMPING_OPT_RX_FILTER:
Filter out spurious receive timestamps: report a receive timestamp only if the matching timestamp generation flag is enabled.
Receive timestamps are generated early in the ingress path, before a packet's destination socket is known. If any socket enables receive timestamps, packets for all socket will receive timestamped packets. Including those that request timestamp reporting with SOF_TIMESTAMPING_SOFTWARE and/or SOF_TIMESTAMPING_RAW_HARDWARE, but do not request receive timestamp generation. This can happen when requesting transmit timestamps only.
Receiving spurious timestamps is generally benign. A process can ignore the unexpected non-zero value. But it makes behavior subtly dependent on other sockets. This flag isolates the socket for more deterministic behavior.
Willem, thanks so much for your effort!!! I learn a lot these days.
I'm going to completely replace it with your description.
Thanks, Jason
From: Jason Xing kernelxing@tencent.com
Test a few possible cases where we use SOF_TIMESTAMPING_OPT_RX_FILTER with software or hardware report/generation flag.
Signed-off-by: Jason Xing kernelxing@tencent.com --- 1. add a new combination test when both flags are set at once --- tools/testing/selftests/net/rxtimestamp.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/tools/testing/selftests/net/rxtimestamp.c b/tools/testing/selftests/net/rxtimestamp.c index 9eb42570294d..16ac4df55fdb 100644 --- a/tools/testing/selftests/net/rxtimestamp.c +++ b/tools/testing/selftests/net/rxtimestamp.c @@ -57,6 +57,8 @@ 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), + SOF_FLAG(SOF_TIMESTAMPING_RAW_HARDWARE), };
static struct socket_type socket_types[] = { @@ -97,6 +99,22 @@ 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 + | SOF_TIMESTAMPING_OPT_RX_FILTER }, + { .swtstamp = true } + }, { { .so_timestamping = SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_RX_SOFTWARE },
linux-kselftest-mirror@lists.linaro.org
-
Jason Xing -
Willem de Bruijn