Jason Xing wrote: > On Thu, Sep 5, 2024 at 10:46 PM Willem de Bruijn > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > 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. > > 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
