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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <67a972a6e2704_14761294b0@willemb.c.googlers.com.notmuch>
Date: Sun, 09 Feb 2025 22:29:42 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Pauli Virtanen <pav@....fi>, 
 linux-bluetooth@...r.kernel.org
Cc: Pauli Virtanen <pav@....fi>, 
 Luiz Augusto von Dentz <luiz.dentz@...il.com>, 
 netdev@...r.kernel.org, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 willemdebruijn.kernel@...il.com
Subject: Re: [PATCH v3 1/5] net-timestamp: COMPLETION timestamp on packet tx
 completion

Pauli Virtanen wrote:
> Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> when hardware reports a packet completed.
> 
> Completion tstamp is useful for Bluetooth, where hardware tx timestamps
> cannot be obtained 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.

Getting the completion timestamp may indeed be useful more broadly.

Alternatively, the HW timestamp is relatively imprecisely defined so
you could even just use that. Ideally, a hw timestamp conforms to IEEE
1588v2 PHY: first symbol on the wire IIRC. But in many cases this is
not the case. It is not feasible at line rate, or the timestamp is
only taken when the completion is written over PCI, which may be
subject to PCI backpressure and happen after transmission on the wire.
As a result, the worst case hw tstamp must already be assumed not much
earlier than a completion timestamp.

That said, +1 on adding explicit well defined measurement point
instead.


> Signed-off-by: Pauli Virtanen <pav@....fi>
> ---
>  Documentation/networking/timestamping.rst | 9 +++++++++
>  include/linux/skbuff.h                    | 6 +++++-
>  include/uapi/linux/errqueue.h             | 1 +
>  include/uapi/linux/net_tstamp.h           | 6 ++++--
>  net/ethtool/common.c                      | 1 +
>  net/socket.c                              | 3 +++
>  6 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 61ef9da10e28..de2afed7a516 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.  The completion
> +  timestamp is generated by the kernel when it receives packet a
> +  completion report from the hardware. Hardware may report multiple
> +  packets at once, and completion timestamps reflect the timing of the
> +  report and not actual tx time. The completion timestamps are
> +  currently implemented only for: Bluetooth L2CAP and ISO.  This
> +  flag can be enabled via both socket options and control messages.
> +

Either we should support this uniformly, or it should be possible to
query whether a driver supports this.

Unfortunately all completion callbacks are driver specific.

But drivers that support hwtstamps will call skb_tstamp_tx with
nonzero hwtstamps. We could use that also to compute and queue
a completion timestamp if requested. At least for existing NIC
drivers.

>  1.3.2 Timestamp Reporting
>  ^^^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index bb2b751d274a..3707c9075ae9 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -489,10 +489,14 @@ enum {
>  
>  	/* generate software time stamp when entering packet scheduling */
>  	SKBTX_SCHED_TSTAMP = 1 << 6,
> +
> +	/* generate software time stamp on packet tx completion */
> +	SKBTX_COMPLETION_TSTAMP = 1 << 7,
>  };
>  
>  #define SKBTX_ANY_SW_TSTAMP	(SKBTX_SW_TSTAMP    | \
> -				 SKBTX_SCHED_TSTAMP)
> +				 SKBTX_SCHED_TSTAMP | \
> +				 SKBTX_COMPLETION_TSTAMP)

These fields are used in the skb_shared_info tx_flags field.
Which is a very scarce resource. This takes the last available bit.
That is my only possible concern: the opportunity cost.

>  #define SKBTX_ANY_TSTAMP	(SKBTX_HW_TSTAMP | \
>  				 SKBTX_HW_TSTAMP_USE_CYCLES | \
>  				 SKBTX_ANY_SW_TSTAMP)
> diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
> index 3c70e8ac14b8..1ea47309d772 100644
> --- a/include/uapi/linux/errqueue.h
> +++ b/include/uapi/linux/errqueue.h
> @@ -73,6 +73,7 @@ enum {
>  	SCM_TSTAMP_SND,		/* driver passed skb to NIC, or HW */
>  	SCM_TSTAMP_SCHED,	/* data entered the packet scheduler */
>  	SCM_TSTAMP_ACK,		/* data acknowledged by peer */
> +	SCM_TSTAMP_COMPLETION,	/* packet tx completion */
>  };
>  
>  #endif /* _UAPI_LINUX_ERRQUEUE_H */
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index 55b0ab51096c..383213de612a 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -44,8 +44,9 @@ enum {
>  	SOF_TIMESTAMPING_BIND_PHC = (1 << 15),
>  	SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16),
>  	SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17),
> +	SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18),
>  
> -	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER,
> +	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION,
>  	SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
>  				 SOF_TIMESTAMPING_LAST
>  };
> @@ -58,7 +59,8 @@ enum {
>  #define SOF_TIMESTAMPING_TX_RECORD_MASK	(SOF_TIMESTAMPING_TX_HARDWARE | \
>  					 SOF_TIMESTAMPING_TX_SOFTWARE | \
>  					 SOF_TIMESTAMPING_TX_SCHED | \
> -					 SOF_TIMESTAMPING_TX_ACK)
> +					 SOF_TIMESTAMPING_TX_ACK | \
> +					 SOF_TIMESTAMPING_TX_COMPLETION)
>  
>  /**
>   * struct so_timestamping - SO_TIMESTAMPING parameter
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 2bd77c94f9f1..75e3b756012e 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -431,6 +431,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = {
>  	[const_ilog2(SOF_TIMESTAMPING_BIND_PHC)]     = "bind-phc",
>  	[const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)]   = "option-id-tcp",
>  	[const_ilog2(SOF_TIMESTAMPING_OPT_RX_FILTER)] = "option-rx-filter",
> +	[const_ilog2(SOF_TIMESTAMPING_TX_COMPLETION)] = "completion-transmit",

just "tx-completion"?