| Message ID | 20240905071738.3725-4-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> > > 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)) > 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 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_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. > > > > 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!
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_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... 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_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... > > 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.
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)