On Fri, Sep 6, 2024 at 12:41 AM Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> wrote: > > 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. 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.
