Re: [PATCH v4 1/5] net-timestamp: COMPLETION timestamp on packet tx completion

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Willem de Bruijn wrote:
> Jason Xing wrote:
> > On Thu, Feb 20, 2025 at 10:35 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.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@iki.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.

SKBTX_HW_TSTAMP_USE_CYCLES is only true if SOF_TIMESTAMPING_BIND_PHC.
It cannot be set per cmsg (is not in SOF_TIMESTAMPING_TX_RECORD_MASK),
so no need to record it per skb.

It only has two drivers using it, which can easily be updated:

	-                if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES)
	+                if (skb->sk &&
	+                    READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
					tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;

They later call skb_tstamp_tx, which does nothing if !skb->sk.
Only cost is a higher cost of accessing the sk cacheline.

SKBTX_WIFI_STATUS essentially follows the same argument. It can only
be set in the sockopt. It has a handful more callsites that would need
to be updated. sock_flag(sk, SOCK_WIFI_STATUS) will be tested without
the socket lock held. But this is already the case in the UDP lockless
fast path through ip_make_skb.

SKBTX_HW_TSTAMP_NETDEV is only used on Rx. Could shadow another bit
that is used only on Tx.

SKBTX_IN_PROGRESS is only used by the driver to suppress the software
tx timestamp from skb_tx_timestamp if a later hardware timestamp will
be generated. Predates SOF_TIMESTAMPING_OPT_TX_SWHW.

In short plenty of bits we can reclaim if we try.

SKBTX_BPF was just merged, so we will have to reclaim one. The first
one seems most straightforward.