Message-ID: <66d9df2c33de1_1eae1a294d6@willemb.c.googlers.com.notmuch>
Date: Thu, 05 Sep 2024 12:41:16 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 shuah@...nel.org, 
 willemb@...gle.com, 
 linux-kselftest@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v4 3/4] net-timestamp: extend
 SOF_TIMESTAMPING_OPT_RX_FILTER for hardware use

Jason Xing wrote:
> On Thu, Sep 5, 2024 at 10:46 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Jason Xing wrote:
> > > On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn
> > > <willemdebruijn.kernel@...il.com> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@...cent.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@...nel.org>
> > > > > Signed-off-by: Jason Xing <kernelxing@...cent.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

Powered by blists - more mailing lists