Message-ID: <CAL+tcoAD4t3f2vMquxwSNVzpRfyHWvG9NYfkjTWZZfd7oHCSGw@mail.gmail.com>
Date: Fri, 21 Feb 2025 08:26:14 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: 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, gerhard@...leder-embedded.com
Subject: Re: [PATCH v4 1/5] net-timestamp: COMPLETION timestamp on packet tx completion

On Fri, Feb 21, 2025 at 7:19 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Willem de Bruijn wrote:
> > 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.
>
> 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.

Those flags look like sub-features to me, not like the completion one.
Occupying one bit in the skb is luxury for them.

>
> 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.

Thanks for the detailed analysis. I just checked them out and agreed.

> In short plenty of bits we can reclaim if we try.
>
> SKBTX_BPF was just merged, so we will have to reclaim one.

It's worth knowing that we probably will work on top of the bpf-next
net branch if so.

Do you want to reclaim every possible bit in one go? One series can
complete the work. // If there is anything, feel free to ask me to
implement/co-work :)

> The first one seems most straightforward.

If there are more flags than the tx_flags can have in the future, I
think we can turn to the second method you mentioned.

Could we harvest one or more to have a better uniformed design before
working on this completion feature, I'm wondering?

Thanks,
Jason

Powered by blists - more mailing lists