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