Jason Xing wrote: > 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 :) 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!
