| Message ID | 20240906095640.77533-2-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net-timestamp: introduce a flag to filter out rx software and hardware report | expand |
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)) { > if_index = 0; > if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV) > -- > 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
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)