[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <66d8c21d3042a_163d93294cb@willemb.c.googlers.com.notmuch>
Date: Wed, 04 Sep 2024 16:25:01 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
willemb@...gle.com,
davem@...emloft.net,
edumazet@...gle.com,
pabeni@...hat.com,
dsahern@...nel.org,
netdev@...r.kernel.org,
Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 1/2] net-timestamp: filter out report when
setting SOF_TIMESTAMPING_SOFTWARE
Jason Xing wrote:
> On Wed, Sep 4, 2024 at 6:13 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Jakub Kicinski wrote:
> > > On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > > > + if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > > > + val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > > + return -EINVAL;
> > >
> > >
> > > > - if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > + if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > + (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > > + !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> > > > has_timestamping = true;
> > > > else
> > > > tss->ts[0] = (struct timespec64) {0};
> > > > }
> > >
> > > > 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_SOFTWARE_FILTER))) &&
> > >
> > > Willem, do you prefer to keep the:
> > >
> > > tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > >
> > > conditions?IIUC we prevent both from being set at once. So
> > >
> > > !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > >
> > > is sufficient (and, subjectively, more intuitive).
> >
> > Good point. Yes, let's definitely simplify.
> >
> > > Question #2 -- why are we only doing this for SW stamps?
> > > HW stamps for TCP are also all or nothing.
> >
> > Fair. Else we'll inevitably add a
> > SOF_TIMESTAMPING_OPT_RX_HARDWARE_FILTER at some point.
> >
> > There probably is no real use to filter one, but not the other.
> >
> > So SOF_TIMESTAMPING_OPT_RX_FILTER then, and also apply
> > to the branch below:
> >
> > if (shhwtstamps &&
> > (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > !skb_is_swtx_tstamp(skb, false_tstamp)) {
> >
> > and same for tcp_recv_timestamp.
>
> When I'm looking at this part, I noticed that RAW_HARDWARE is actually
> a tx report flag instead of rx, please also see the kdoc you wrote a
> long time ago:
>
> SOF_TIMESTAMPING_RAW_HARDWARE:
> Report hardware timestamps as generated by
> SOF_TIMESTAMPING_TX_HARDWARE when available.
Right, this is analogous to the software part that you modify:
if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
empty = 0;
The idea is to also add for hardware timestamps your suggested
condition that the socket also sets the timestamp generation flag
SOF_TIMESTAMPING_RX_HARDWARE or that the new OPT_RX_FILTER flag
is not set.
> If so, OPT_RX_FILTER doesn't fit for the name of tx timestamp.
>
> I wonder if I can only revise the series with the code simplified as
> Jakub suggested and then repost it? I think we need to choose a new
> name for this tx hardware report case, like
> SOF_TIMESTAMPING_OPT_TX_HARDWARE_FILTER?
>
> Since it belongs to the tx path, can I put it into another series or a
> new patch in the current series where I will explicitly explain why we
> also need to introduce this new flag?
I think the confusion here comes from that comment that
SOF_TIMESTAMPING_RAW_HARDWARE only reports
SOF_TIMESTAMPING_TX_HARDWARE generated timestamps. This statement is
incorrect and should be revised. It also reports
SOF_TIMESTAMPING_RX_HARDWARE.
Unless I'm missing something. But I think the author of that statement
is the one who made the mistake. Who is.. also me.
Powered by blists - more mailing lists