On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > Jason Xing wrote: > > From: Jason Xing <kernelxing@xxxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > Signed-off-by: Jason Xing <kernelxing@xxxxxxxxxxx> > > --- > > Link: https://lore.kernel.org/all/20240903121940.6390b958@xxxxxxxxxx/ > > --- > > 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!
