Message-ID: <67b74c47c14c7_261ab62943@willemb.c.googlers.com.notmuch>
Date: Thu, 20 Feb 2025 10:37:43 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Pauli Virtanen <pav@....fi>, 
 linux-bluetooth@...r.kernel.org, 
 Luiz Augusto von Dentz <luiz.dentz@...il.com>, 
 netdev@...r.kernel.org, 
 davem@...emloft.net, 
 kuba@...nel.org
Subject: Re: [PATCH v4 1/5] net-timestamp: COMPLETION timestamp on packet tx
 completion

Jason Xing wrote:
> On Thu, Feb 20, 2025 at 10:35 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Jason Xing wrote:
> > > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@....fi> wrote:
> > > >
> > > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> > > > when hardware reports a packet completed.
> > > >
> > > > Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> > > > exist in the HCI specification except for ISO packets, and the hardware
> > > > has a queue where packets may wait.  In this case the software SND
> > > > timestamp only reflects the kernel-side part of the total latency
> > > > (usually small) and queue length (usually 0 unless HW buffers
> > > > congested), whereas the completion report time is more informative of
> > > > the true latency.
> > > >
> > > > It may also be useful in other cases where HW TX timestamps cannot be
> > > > obtained and user wants to estimate an upper bound to when the TX
> > > > probably happened.
> > > >
> > > > Signed-off-by: Pauli Virtanen <pav@....fi>
> > > > ---
> > > >
> > > > Notes:
> > > >     v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
> > > >         together with SND, to save a bit in skb_shared_info.tx_flags
> > > >
> > > >         As it then cannot be set per-skb, reject setting it via CMSG.
> > > >
> > > >  Documentation/networking/timestamping.rst | 9 +++++++++
> > > >  include/uapi/linux/errqueue.h             | 1 +
> > > >  include/uapi/linux/net_tstamp.h           | 6 ++++--
> > > >  net/core/sock.c                           | 2 ++
> > > >  net/ethtool/common.c                      | 1 +
> > > >  5 files changed, 17 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > > index 61ef9da10e28..5034dfe326c0 100644
> > > > --- a/Documentation/networking/timestamping.rst
> > > > +++ b/Documentation/networking/timestamping.rst
> > > > @@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
> > > >    cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > > >    This flag can be enabled via both socket options and control messages.
> > > >
> > > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > > +  Request tx timestamps on packet tx completion, for the packets that
> > > > +  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion
> > >
> > > Is it mandatory for other drivers that will try to use
> > > SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled
> > > both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be
> > > better if you add the limitation in sock_set_timestamping() so that
> > > the same rule can be applied to other drivers.
> > >
> > > But may I ask why you tried to couple them so tight in the version?
> > > Could you say more about this? It's optional, right? IIUC, you
> > > expected the driver to have both timestamps and then calculate the
> > > delta easily?
> >
> > This is a workaround around the limited number of bits available in
> > skb_shared_info.tx_flags.
> 
> Oh, I'm surprised I missed the point even though I revisited the
> previous discussion.
> 
> Pauli, please add the limitation when users setsockopt in
> sock_set_timestamping() :)
> 
> >
> > Pauli could claim last available bit 7.. but then you would need to
> > find another bit for SKBTX_BPF ;)
> 
> Right :D
> 
> >
> > FWIW I think we could probably free up 1 or 2 bits if we look closely,
> > e.g., of SKBTX_HW_TSTAMP_USE_CYCLES or SKBTX_WIFI_STATUS.
> 
> Good. Will you submit a patch series to do that, or...?

Reclaiming space is really up to whoever needs it.

I'll take a quick look, just to see if there is an obvious path and
we can postpone this whole conversation to next time we need a bit.

> 
> >
> > But given that two uses for those bits are in review at the same time,
> > I doubt that this is the last such feature that is added.
> >
> > This workaround is clever. Only, we're leaking implementation
> > limitations into the API, making it API non-uniform and thus more
> > complex.
> 
> Probably not a big deal because previously OPT_ID_TCP is also bound with OPT_ID?

That is also unfortunate. Ideally we had never needed OPT_ID_TCP.
 
> >
> > On the one hand, the feature should work just like all the existing
> > ones, and thus be configurable independently, and maintaining a
> > consistent API. But, this does require a tx_flags bit. And the
> > same for each subsequent such feature that gets proposed.
> >
> > On the other, if we can anticipate more such additional requests,
> > then perhaps it does make sense to use only one bit in the skb to
> > encode whether the skb is sampled (in this case SKBTX_SW_TSTAMP),
> > and use per-socket sk_tsflags to encode which types of timestamps
> > to generate for such sampled packets.
> 
> Very good idea, I think.

After giving it some thought, I prefer the first option to maintain
the same API. Let's see if we can harvest a bit and use that for this
new completion point.

> 
> >
> > This is more or less how sampling in the new BPF mode works too.
> >
> > It is just not how SO_TIMESTAMPING works for existing bits.
> 
> If needed, especially when a new bit is added, I think we can refactor
> this part in the future? Or can we adjust it in advance? Speaking of
> the design itself, it's really good :)

We cannot change existing behavior.

> Thanks,
> Jason
> 
> >
> >
> > There's something to be said for either approach IMHO.

Powered by blists - more mailing lists