Message-ID: <66dbbbac6a6df_2b0bd0294d3@willemb.c.googlers.com.notmuch>
Date: Fri, 06 Sep 2024 22:34:20 -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 v5 1/2] net-timestamp: introduce
 SOF_TIMESTAMPING_OPT_RX_FILTER flag

Jason Xing wrote:
> On Sat, Sep 7, 2024 at 7:24 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <kernelxing@...cent.com>
> > >
> > > introduce a new flag SOF_TIMESTAMPING_OPT_RX_FILTER in the receive
> > > path. User can set it with SOF_TIMESTAMPING_SOFTWARE to filter
> > > out rx software timestamp report, especially after a process turns on
> > > netstamp_needed_key which can time stamp every incoming skb.
> > >
> > > Previously, we found out if an application starts first which turns on
> > > netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> > > could also get rx timestamp. Now we handle this case by introducing this
> > > new flag without breaking users.
> > >
> > > Quoting Willem to explain why we need the flag:
> > > "why a process would want to request software timestamp reporting, but
> > > not receive software timestamp generation. The only use I see is when
> > > the application does request
> > > SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_TX_SOFTWARE."
> > >
> > > Similarly, this new flag could also be used for hardware case where we
> > > can set it with SOF_TIMESTAMPING_RAW_HARDWARE, then we won't receive
> > > hardware receive timestamp.
> > >
> > > Another thing about errqueue in this patch I have a few words to say:
> > > In this case, we need to handle the egress path carefully, or else
> > > reporting the tx timestamp will fail. Egress path and ingress path will
> > > finally call sock_recv_timestamp(). We have to distinguish them.
> > > Errqueue is a good indicator to reflect the flow direction.
> > >
> > > Suggested-by: Willem de Bruijn <willemb@...gle.com>
> > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> >
> > High level: where is the harm in receiving unsolicited timestamps?
> > A process can easily ignore them. I do wonder if the only use case is
> > an overly strict testcase. Was reminded of this as I tried to write
> > a more concise paragraph for the documentation.
> 
> You raised a good question.
> 
> I think It's more of a design consideration instead of a bugfix
> actually. So it is not solving a bug which makes the apps wrong but
> gives users a hint that we can explicitly and accurately do what we
> want and we expect.
> 
> Let's assume: if we remove all the report flags design, what will
> happen? It can work of course. I don't believe that people choose to
> enable the generation flag but are not willing to report it. Of
> course, It's another thing. I'm just saying.

Let's not debate the existing API. Its design predates both of our
contributions.

> I wonder if it makes sense to you :) ?

I don't see a strong use case. I know we're on v5 while I reopen that
point, sorry.

It seems simpler to me to not read spurious fields that are not
requested, rather than to explicitly request them to be set to zero.

Adding more flags is not free. An extra option adds mental load for
casual users of this alread complex API. This may certainly sound
hypocritical coming from me, as I added my fair share. But I hope
their functionality outweighs that cost (well.. in at least one case
it was an ugly fix for a bad first attempt.. OPT_ID). 

I got to this point trying to condense the proposed documentation.
We can add this if you feel strongly.

But then my main feedback is that the doc has to be shorter and to
the point. Why would a user user this? No background on how we got
here, what they might already do accidentally.
> >
> > Otherwise implementation looks fine, only the tiniest nit.
> >
> > > @@ -946,11 +946,17 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> > >
> > >       memset(&tss, 0, sizeof(tss));
> > >       tsflags = READ_ONCE(sk->sk_tsflags);
> > > -     if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> > > +     if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > +          (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > +          skb_is_err_queue(skb) ||
> > > +          !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
> >
> > Nit: these statements should all align on the inner brace, so indent
> > by one character.
> 
> I'm not that sure about the format, please help me to review here:
> 
> @@ -946,11 +946,17 @@ void __sock_recv_timestamp(struct msghdr *msg,
> struct sock *sk,
> 
>         memset(&tss, 0, sizeof(tss));
>         tsflags = READ_ONCE(sk->sk_tsflags);
> -       if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> +       if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +            (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> +             skb_is_err_queue(skb) ||
> +             !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
>             ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
>                 empty = 0;
>         if (shhwtstamps &&
> -           (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> +           (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE &&
> +            (tsflags & SOF_TIMESTAMPING_RX_HARDWARE ||
> +             skb_is_err_queue(skb) ||
> +             !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
>             !skb_is_swtx_tstamp(skb, false_tstamp)) {
>                 if_index = 0;
>                 if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)


That's right

  (A &&
   (B ||
    C))

> >
> > >           ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
> > >               empty = 0;
> > >       if (shhwtstamps &&
> > > -         (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > > +         (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE &&
> > > +         (tsflags & SOF_TIMESTAMPING_RX_HARDWARE ||
> 
> same here and the following two statements? Should I also indent by
> one char by the way?

Same rule on all new code introduced in the patch.

> > > +         skb_is_err_queue(skb) ||
> > > +         !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
> > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > >               if_index = 0;
> > >               if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
> > > --
> > > 2.37.3
> > >
> >
> >

Powered by blists - more mailing lists