[PATCH v3 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO

[PATCH v3 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO

[Pauli Virtanen]

Add support for TX timestamping in Bluetooth ISO/L2CAP/SCO sockets.

Add new COMPLETION timestamp type, to report a software timestamp when
the hardware reports a packet completed. (Cc netdev for this)

Overview
========

The packet flow in Bluetooth is the following. Timestamps added here
indicated:

user sendmsg() generates skbs
|
* skb waits in net/bluetooth queue for a free HW packet slot
|
* orphan skb, send to driver -> TSTAMP_SND
|
* driver: send packet data to transport (eg. USB)
|
* wait for transport completion
|
* driver: transport tx completion, free skb (some do this immediately)
|
* packet waits in HW side queue
|
* HCI report for packet completion -> TSTAMP_COMPLETION (for non-SCO)

In addition, we may want to do the following in future (but not
implemented in this series as we don't have ISO sequence number
synchronization yet which is needed first, moreover e.g. Intel
controllers return only zeros in timestamps):

* if packet is ISO, send HCI LE Read ISO TX Sync
|
* HCI response -> hardware TSTAMP_SND for the packet the response
  corresponds to if it was waiting for one, might not be possible
  to get a tstamp for every packet

Bluetooth does not have tx timestamps in the completion reports from
hardware, and only for ISO packets there are HCI commands in
specification for querying timestamps afterward.

The drivers do not provide ways to get timestamps either, I'm also not
aware if some devices would have vendor-specific commands to get them.

Driver-side timestamps
======================

Generating SND on driver side may be slightly more accurate, but that
requires changing the BT driver API to not orphan skbs first.  In theory
this probably won't cause problems, but it is not done in this patchset.

For some of the drivers it won't gain much. E.g. btusb immediately
submits the URB, so if one would emit SND just before submit (as
drivers/net/usb/usbnet.c does), it is essentially identical to emitting
before sending to driver.  btintel_pcie looks like it does synchronous
send, so looks the same.  hci_serdev has internal queue, iiuc flushing
as fast as data can be transferred, but it shouldn't be waiting for
hardware slots due to HCI flow control.

Unless HW buffers are full, packets mostly wait on the HW side.  E.g.
with btusb (non-SCO) median time from sendmsg() to URB generation is
~0.1 ms, to USB completion ~0.5 ms, and HCI completion report at ~5 ms.

The exception is SCO, for which HCI flow control is disabled, so they do
not get completion events so it's possible to build up queues inside the
driver. For SCO, COMPLETION needs to be generated from driver side, eg.
for btusb maybe at URB completion.  This could be useful for SCO PCM
modes (but which are more or less obsolete nowadays), where USB isoc
data rate matches audio data rate, so queues on USB side may build up.

Use cases
=========

In audio use cases we want to track and avoid queues building up, to
control latency, especially in cases like ISO where the controller has a
fixed schedule that the sending application must match.  E.g.
application can aim to keep 1 packet in HW queue, so it has 2*7.5ms of
slack for being woken up too late.

Applications can use SND & COMPLETION timestamps to track in-kernel and
in-HW packet queues separately.  This can matter for ISO, where the
specification allows HW to use the timings when it gets packets to
determine what packets are synchronized together. Applications can use
SND to track that.

Changes in v3
=============

- Add new COMPLETION timestamp type, and emit it in HCI completion.

- Emit SND instead of SCHED, when sending to driver.

- Do not emit SCHED timestamps.

- Don't safeguard tx_q length explicitly. Now that hci_sched_acl_blk()
  is no more, the scheduler flow control is guaranteed to keep it
  bounded.

- Fix L2CAP stream sockets to use the bytestream timestamp conventions.

Pauli Virtanen (5):
  net-timestamp: COMPLETION timestamp on packet tx completion
  Bluetooth: add support for skb TX SND/COMPLETION timestamping
  Bluetooth: ISO: add TX timestamping
  Bluetooth: L2CAP: add TX timestamping
  Bluetooth: SCO: add TX timestamping socket-level mechanism

 Documentation/networking/timestamping.rst |   9 ++
 include/linux/skbuff.h                    |   6 +-
 include/net/bluetooth/bluetooth.h         |   1 +
 include/net/bluetooth/hci_core.h          |  13 +++
 include/net/bluetooth/l2cap.h             |   3 +-
 include/uapi/linux/errqueue.h             |   1 +
 include/uapi/linux/net_tstamp.h           |   6 +-
 net/bluetooth/6lowpan.c                   |   2 +-
 net/bluetooth/hci_conn.c                  | 117 ++++++++++++++++++++++
 net/bluetooth/hci_core.c                  |  17 +++-
 net/bluetooth/hci_event.c                 |   4 +
 net/bluetooth/iso.c                       |  24 ++++-
 net/bluetooth/l2cap_core.c                |  41 +++++++-
 net/bluetooth/l2cap_sock.c                |  15 ++-
 net/bluetooth/sco.c                       |  19 +++-
 net/bluetooth/smp.c                       |   2 +-
 net/ethtool/common.c                      |   1 +
 net/socket.c                              |   3 +
 18 files changed, 263 insertions(+), 21 deletions(-)

-- 
2.48.1

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

[Pauli Virtanen]

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.

Signed-off-by: Pauli Virtanen <pav@iki.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.
+
 
 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)
 #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",
 };
 static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT);
 
diff --git a/net/socket.c b/net/socket.c
index 4afe31656a2b..22b7f6f50889 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -693,6 +693,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
 	if (tsflags & SOF_TIMESTAMPING_TX_SCHED)
 		flags |= SKBTX_SCHED_TSTAMP;
 
+	if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION)
+		flags |= SKBTX_COMPLETION_TSTAMP;
+
 	*tx_flags = flags;
 }
 EXPORT_SYMBOL(__sock_tx_timestamp);
-- 
2.48.1

[PATCH v3 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping

[Pauli Virtanen]

Support enabling TX timestamping for some skbs, and track them until
packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
completion report from hardware.

Generate software SCM_TSTAMP_SND before sending to driver. Sending from
driver requires changes in the driver API, and drivers mostly are going
to send the skb immediately.

Make the default situation with no COMPLETION TX timestamping more
efficient by only counting packets in the queue when there is nothing to
track.  When there is something to track, we need to make clones, since
the driver may modify sent skbs.

The tx_q queue length is bounded by the hdev flow control, which will
not send new packets before it has got completion reports for old ones.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/hci_core.h |  13 ++++
 net/bluetooth/hci_conn.c         | 117 +++++++++++++++++++++++++++++++
 net/bluetooth/hci_core.c         |  17 +++--
 net/bluetooth/hci_event.c        |   4 ++
 4 files changed, 146 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 05919848ea95..1f539a9881ad 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -261,6 +261,12 @@ struct adv_info {
 	struct delayed_work	rpa_expired_cb;
 };
 
+struct tx_queue {
+	struct sk_buff_head queue;
+	unsigned int extra;
+	unsigned int tracked;
+};
+
 #define HCI_MAX_ADV_INSTANCES		5
 #define HCI_DEFAULT_ADV_DURATION	2
 
@@ -733,6 +739,8 @@ struct hci_conn {
 	struct sk_buff_head data_q;
 	struct list_head chan_list;
 
+	struct tx_queue tx_q;
+
 	struct delayed_work disc_work;
 	struct delayed_work auto_accept_work;
 	struct delayed_work idle_work;
@@ -1571,6 +1579,11 @@ void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
 void hci_conn_failed(struct hci_conn *conn, u8 status);
 u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
 
+void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb);
+void hci_conn_tx_dequeue(struct hci_conn *conn);
+void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
+			    const struct sockcm_cookie *sockc);
+
 /*
  * hci_conn_get() and hci_conn_put() are used to control the life-time of an
  * "hci_conn" object. They do not guarantee that the hci_conn object is running,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d097e308a755..e437290d8b70 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -27,6 +27,7 @@
 
 #include <linux/export.h>
 #include <linux/debugfs.h>
+#include <linux/errqueue.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -1002,6 +1003,7 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t
 	}
 
 	skb_queue_head_init(&conn->data_q);
+	skb_queue_head_init(&conn->tx_q.queue);
 
 	INIT_LIST_HEAD(&conn->chan_list);
 	INIT_LIST_HEAD(&conn->link_list);
@@ -1155,6 +1157,7 @@ void hci_conn_del(struct hci_conn *conn)
 	}
 
 	skb_queue_purge(&conn->data_q);
+	skb_queue_purge(&conn->tx_q.queue);
 
 	/* Remove the connection from the list and cleanup its remaining
 	 * state. This is a separate function since for some cases like
@@ -3064,3 +3067,117 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
 	 */
 	return hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
 }
+
+void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
+			    const struct sockcm_cookie *sockc)
+{
+	struct sock *sk = skb ? skb->sk : NULL;
+
+	/* This shall be called on a single skb of those generated by user
+	 * sendmsg(), and only when the sendmsg() does not return error to
+	 * user. This is required for keeping the tskey that increments here in
+	 * sync with possible sendmsg() counting by user.
+	 *
+	 * Stream sockets shall set key_offset to sendmsg() length in bytes
+	 * and call with the last fragment, others to 1 and first fragment.
+	 */
+
+	if (!skb || !sockc || !sk || !key_offset)
+		return;
+
+	sock_tx_timestamp(sk, sockc, &skb_shinfo(skb)->tx_flags);
+
+	if (sockc->tsflags & SOF_TIMESTAMPING_OPT_ID &&
+	    sockc->tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) {
+		if (sockc->tsflags & SOCKCM_FLAG_TS_OPT_ID) {
+			skb_shinfo(skb)->tskey = sockc->ts_opt_id;
+		} else {
+			int key = atomic_add_return(key_offset, &sk->sk_tskey);
+
+			skb_shinfo(skb)->tskey = key - 1;
+		}
+	}
+}
+
+void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
+{
+	struct tx_queue *comp = &conn->tx_q;
+	bool track = false;
+
+	/* Emit SND now, ie. just before sending to driver */
+	if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP))
+		__skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SND);
+
+	/* COMPLETION tstamp is emitted for tracked skb later in Number of
+	 * Completed Packets event. Available only for flow controlled cases.
+	 *
+	 * TODO: SCO support (needs to be done in drivers)
+	 */
+	switch (conn->type) {
+	case ISO_LINK:
+	case ACL_LINK:
+	case LE_LINK:
+		break;
+	default:
+		return;
+	}
+
+	if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
+		track = true;
+
+	/* If nothing is tracked, just count extra skbs at the queue head */
+	if (!track && !comp->tracked) {
+		comp->extra++;
+		return;
+	}
+
+	if (track) {
+		skb = skb_clone_sk(skb);
+		if (!skb)
+			goto count_only;
+
+		comp->tracked++;
+	} else {
+		skb = skb_clone(skb, GFP_KERNEL);
+		if (!skb)
+			goto count_only;
+	}
+
+	skb_queue_tail(&comp->queue, skb);
+	return;
+
+count_only:
+	/* Stop tracking skbs, and only count. This will not emit timestamps for
+	 * the packets, but if we get here something is more seriously wrong.
+	 */
+	comp->tracked = 0;
+	comp->extra += skb_queue_len(&comp->queue) + 1;
+	skb_queue_purge(&comp->queue);
+}
+
+void hci_conn_tx_dequeue(struct hci_conn *conn)
+{
+	struct tx_queue *comp = &conn->tx_q;
+	struct sk_buff *skb;
+
+	/* If there are tracked skbs, the counted extra go before dequeuing real
+	 * skbs, to keep ordering. When nothing is tracked, the ordering doesn't
+	 * matter so dequeue real skbs first to get rid of them ASAP.
+	 */
+	if (comp->extra && (comp->tracked || skb_queue_empty(&comp->queue))) {
+		comp->extra--;
+		return;
+	}
+
+	skb = skb_dequeue(&comp->queue);
+	if (!skb)
+		return;
+
+	if (skb->sk) {
+		comp->tracked--;
+		__skb_tstamp_tx(skb, NULL, NULL, skb->sk,
+				SCM_TSTAMP_COMPLETION);
+	}
+
+	kfree_skb(skb);
+}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e7ec12437c8b..e0845188f626 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3025,6 +3025,13 @@ static int hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
 
+static int hci_send_conn_frame(struct hci_dev *hdev, struct hci_conn *conn,
+			       struct sk_buff *skb)
+{
+	hci_conn_tx_queue(conn, skb);
+	return hci_send_frame(hdev, skb);
+}
+
 /* Send HCI command */
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen,
 		 const void *param)
@@ -3562,7 +3569,7 @@ static void hci_sched_sco(struct hci_dev *hdev)
 	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);
-			hci_send_frame(hdev, skb);
+			hci_send_conn_frame(hdev, conn, skb);
 
 			conn->sent++;
 			if (conn->sent == ~0)
@@ -3586,7 +3593,7 @@ static void hci_sched_esco(struct hci_dev *hdev)
 						     &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);
-			hci_send_frame(hdev, skb);
+			hci_send_conn_frame(hdev, conn, skb);
 
 			conn->sent++;
 			if (conn->sent == ~0)
@@ -3620,7 +3627,7 @@ static void hci_sched_acl_pkt(struct hci_dev *hdev)
 			hci_conn_enter_active_mode(chan->conn,
 						   bt_cb(skb)->force_active);
 
-			hci_send_frame(hdev, skb);
+			hci_send_conn_frame(hdev, chan->conn, skb);
 			hdev->acl_last_tx = jiffies;
 
 			hdev->acl_cnt--;
@@ -3676,7 +3683,7 @@ static void hci_sched_le(struct hci_dev *hdev)
 
 			skb = skb_dequeue(&chan->data_q);
 
-			hci_send_frame(hdev, skb);
+			hci_send_conn_frame(hdev, chan->conn, skb);
 			hdev->le_last_tx = jiffies;
 
 			(*cnt)--;
@@ -3710,7 +3717,7 @@ static void hci_sched_iso(struct hci_dev *hdev)
 	while (*cnt && (conn = hci_low_sent(hdev, ISO_LINK, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);
-			hci_send_frame(hdev, skb);
+			hci_send_conn_frame(hdev, conn, skb);
 
 			conn->sent++;
 			if (conn->sent == ~0)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 2cc7a9306350..144b442180f7 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4405,6 +4405,7 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
 		struct hci_comp_pkts_info *info = &ev->handles[i];
 		struct hci_conn *conn;
 		__u16  handle, count;
+		unsigned int i;
 
 		handle = __le16_to_cpu(info->handle);
 		count  = __le16_to_cpu(info->count);
@@ -4415,6 +4416,9 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
 
 		conn->sent -= count;
 
+		for (i = 0; i < count; ++i)
+			hci_conn_tx_dequeue(conn);
+
 		switch (conn->type) {
 		case ACL_LINK:
 			hdev->acl_cnt += count;
-- 
2.48.1

[PATCH v3 3/5] Bluetooth: ISO: add TX timestamping

[Pauli Virtanen]

Add BT_SCM_ERROR socket CMSG type.

Support TX timestamping in ISO sockets.

Support MSG_ERRQUEUE in ISO recvmsg.

If a packet from sendmsg() is fragmented, only the first ACL fragment is
timestamped.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/iso.c               | 24 ++++++++++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 435250c72d56..bbefde319f95 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -156,6 +156,7 @@ struct bt_voice {
 #define BT_PKT_STATUS           16
 
 #define BT_SCM_PKT_STATUS	0x03
+#define BT_SCM_ERROR		0x04
 
 #define BT_ISO_QOS		17
 
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 44acddf58a0c..f497759a2af5 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -518,7 +518,8 @@ static struct bt_iso_qos *iso_sock_get_qos(struct sock *sk)
 	return &iso_pi(sk)->qos;
 }
 
-static int iso_send_frame(struct sock *sk, struct sk_buff *skb)
+static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
+			  const struct sockcm_cookie *sockc)
 {
 	struct iso_conn *conn = iso_pi(sk)->conn;
 	struct bt_iso_qos *qos = iso_sock_get_qos(sk);
@@ -538,10 +539,12 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb)
 	hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len,
 						      HCI_ISO_STATUS_VALID));
 
-	if (sk->sk_state == BT_CONNECTED)
+	if (sk->sk_state == BT_CONNECTED) {
+		hci_setup_tx_timestamp(skb, 1, sockc);
 		hci_send_iso(conn->hcon, skb);
-	else
+	} else {
 		len = -ENOTCONN;
+	}
 
 	return len;
 }
@@ -1348,6 +1351,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 {
 	struct sock *sk = sock->sk;
 	struct sk_buff *skb, **frag;
+	struct sockcm_cookie sockc;
 	size_t mtu;
 	int err;
 
@@ -1360,6 +1364,14 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (msg->msg_flags & MSG_OOB)
 		return -EOPNOTSUPP;
 
+	sockcm_init(&sockc, sk);
+
+	if (msg->msg_controllen) {
+		err = sock_cmsg_send(sk, msg, &sockc);
+		if (err)
+			return err;
+	}
+
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_CONNECTED) {
@@ -1405,7 +1417,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	lock_sock(sk);
 
 	if (sk->sk_state == BT_CONNECTED)
-		err = iso_send_frame(sk, skb);
+		err = iso_send_frame(sk, skb, &sockc);
 	else
 		err = -ENOTCONN;
 
@@ -1474,6 +1486,10 @@ static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 
 	BT_DBG("sk %p", sk);
 
+	if (unlikely(flags & MSG_ERRQUEUE))
+		return sock_recv_errqueue(sk, msg, len, SOL_BLUETOOTH,
+					  BT_SCM_ERROR);
+
 	if (test_and_clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
 		sock_hold(sk);
 		lock_sock(sk);
-- 
2.48.1

[PATCH v3 4/5] Bluetooth: L2CAP: add TX timestamping

[Pauli Virtanen]

Support TX timestamping in L2CAP sockets.

Support MSG_ERRQUEUE recvmsg.

For other than SOCK_STREAM L2CAP sockets, if a packet from sendmsg() is
fragmented, only the first ACL fragment is timestamped.

For SOCK_STREAM L2CAP sockets, use the bytestream convention and
timestamp the last fragment and count bytes in tskey.

Timestamps are not generated in the Enhanced Retransmission mode, as
meaning of COMPLETION stamp is unclear if L2CAP layer retransmits.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h |  3 ++-
 net/bluetooth/6lowpan.c       |  2 +-
 net/bluetooth/l2cap_core.c    | 41 ++++++++++++++++++++++++++++++++---
 net/bluetooth/l2cap_sock.c    | 15 ++++++++++++-
 net/bluetooth/smp.c           |  2 +-
 5 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 9189354c568f..00e182a22720 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -955,7 +955,8 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason);
 int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 		       bdaddr_t *dst, u8 dst_type, u16 timeout);
 int l2cap_chan_reconfigure(struct l2cap_chan *chan, __u16 mtu);
-int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len);
+int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
+		    const struct sockcm_cookie *sockc);
 void l2cap_chan_busy(struct l2cap_chan *chan, int busy);
 void l2cap_chan_rx_avail(struct l2cap_chan *chan, ssize_t rx_avail);
 int l2cap_chan_check_security(struct l2cap_chan *chan, bool initiator);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 50cfec8ccac4..0eb1ecb54105 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -443,7 +443,7 @@ static int send_pkt(struct l2cap_chan *chan, struct sk_buff *skb,
 	memset(&msg, 0, sizeof(msg));
 	iov_iter_kvec(&msg.msg_iter, ITER_SOURCE, &iv, 1, skb->len);
 
-	err = l2cap_chan_send(chan, &msg, skb->len);
+	err = l2cap_chan_send(chan, &msg, skb->len, NULL);
 	if (err > 0) {
 		netdev->stats.tx_bytes += err;
 		netdev->stats.tx_packets++;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 6cdc1dc3a7f9..6865a0f51df5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2514,7 +2514,33 @@ static void l2cap_le_flowctl_send(struct l2cap_chan *chan)
 	       skb_queue_len(&chan->tx_q));
 }
 
-int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
+static void l2cap_tx_timestamp(struct sk_buff *skb,
+			       const struct sockcm_cookie *sockc,
+			       size_t len)
+{
+	struct sock *sk = skb ? skb->sk : NULL;
+
+	if (sk && sk->sk_type == SOCK_STREAM)
+		hci_setup_tx_timestamp(skb, len, sockc);
+	else
+		hci_setup_tx_timestamp(skb, 1, sockc);
+}
+
+static void l2cap_tx_timestamp_seg(struct sk_buff_head *queue,
+				   const struct sockcm_cookie *sockc,
+				   size_t len)
+{
+	struct sk_buff *skb = skb_peek(queue);
+	struct sock *sk = skb ? skb->sk : NULL;
+
+	if (sk && sk->sk_type == SOCK_STREAM)
+		l2cap_tx_timestamp(skb_peek_tail(queue), sockc, len);
+	else
+		l2cap_tx_timestamp(skb, sockc, len);
+}
+
+int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
+		    const struct sockcm_cookie *sockc)
 {
 	struct sk_buff *skb;
 	int err;
@@ -2529,6 +2555,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
 		if (IS_ERR(skb))
 			return PTR_ERR(skb);
 
+		l2cap_tx_timestamp(skb, sockc, len);
+
 		l2cap_do_send(chan, skb);
 		return len;
 	}
@@ -2552,6 +2580,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
 		if (err)
 			return err;
 
+		l2cap_tx_timestamp_seg(&seg_queue, sockc, len);
+
 		skb_queue_splice_tail_init(&seg_queue, &chan->tx_q);
 
 		l2cap_le_flowctl_send(chan);
@@ -2573,6 +2603,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
 		if (IS_ERR(skb))
 			return PTR_ERR(skb);
 
+		l2cap_tx_timestamp(skb, sockc, len);
+
 		l2cap_do_send(chan, skb);
 		err = len;
 		break;
@@ -2596,10 +2628,13 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
 		if (err)
 			break;
 
-		if (chan->mode == L2CAP_MODE_ERTM)
+		if (chan->mode == L2CAP_MODE_ERTM) {
+			/* TODO: ERTM mode timestamping */
 			l2cap_tx(chan, NULL, &seg_queue, L2CAP_EV_DATA_REQUEST);
-		else
+		} else {
+			l2cap_tx_timestamp_seg(&seg_queue, sockc, len);
 			l2cap_streaming_send(chan, &seg_queue);
+		}
 
 		err = len;
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index acd11b268b98..92305530e2e5 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1106,6 +1106,7 @@ static int l2cap_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 {
 	struct sock *sk = sock->sk;
 	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+	struct sockcm_cookie sockc;
 	int err;
 
 	BT_DBG("sock %p, sk %p", sock, sk);
@@ -1120,6 +1121,14 @@ static int l2cap_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (sk->sk_state != BT_CONNECTED)
 		return -ENOTCONN;
 
+	sockcm_init(&sockc, sk);
+
+	if (msg->msg_controllen) {
+		err = sock_cmsg_send(sk, msg, &sockc);
+		if (err)
+			return err;
+	}
+
 	lock_sock(sk);
 	err = bt_sock_wait_ready(sk, msg->msg_flags);
 	release_sock(sk);
@@ -1127,7 +1136,7 @@ static int l2cap_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		return err;
 
 	l2cap_chan_lock(chan);
-	err = l2cap_chan_send(chan, msg, len);
+	err = l2cap_chan_send(chan, msg, len, &sockc);
 	l2cap_chan_unlock(chan);
 
 	return err;
@@ -1168,6 +1177,10 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
 	int err;
 
+	if (unlikely(flags & MSG_ERRQUEUE))
+		return sock_recv_errqueue(sk, msg, len, SOL_BLUETOOTH,
+					  BT_SCM_ERROR);
+
 	lock_sock(sk);
 
 	if (sk->sk_state == BT_CONNECT2 && test_bit(BT_SK_DEFER_SETUP,
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 8b9724fd752a..f5e5c2f111b9 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -608,7 +608,7 @@ static void smp_send_cmd(struct l2cap_conn *conn, u8 code, u16 len, void *data)
 
 	iov_iter_kvec(&msg.msg_iter, ITER_SOURCE, iv, 2, 1 + len);
 
-	l2cap_chan_send(chan, &msg, 1 + len);
+	l2cap_chan_send(chan, &msg, 1 + len, NULL);
 
 	if (!chan->data)
 		return;
-- 
2.48.1

[PATCH v3 5/5] Bluetooth: SCO: add TX timestamping socket-level mechanism

[Pauli Virtanen]

Support TX timestamping in SCO sockets.

Support MSG_ERRQUEUE in SCO recvmsg.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/sco.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index aa7bfe26cb40..f39c57ac594f 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -370,7 +370,8 @@ static int sco_connect(struct sock *sk)
 	return err;
 }
 
-static int sco_send_frame(struct sock *sk, struct sk_buff *skb)
+static int sco_send_frame(struct sock *sk, struct sk_buff *skb,
+			  const struct sockcm_cookie *sockc)
 {
 	struct sco_conn *conn = sco_pi(sk)->conn;
 	int len = skb->len;
@@ -381,6 +382,7 @@ static int sco_send_frame(struct sock *sk, struct sk_buff *skb)
 
 	BT_DBG("sk %p len %d", sk, len);
 
+	hci_setup_tx_timestamp(skb, 1, sockc);
 	hci_send_sco(conn->hcon, skb);
 
 	return len;
@@ -776,6 +778,7 @@ static int sco_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 {
 	struct sock *sk = sock->sk;
 	struct sk_buff *skb;
+	struct sockcm_cookie sockc;
 	int err;
 
 	BT_DBG("sock %p, sk %p", sock, sk);
@@ -787,6 +790,14 @@ static int sco_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (msg->msg_flags & MSG_OOB)
 		return -EOPNOTSUPP;
 
+	sockcm_init(&sockc, sk);
+
+	if (msg->msg_controllen) {
+		err = sock_cmsg_send(sk, msg, &sockc);
+		if (err)
+			return err;
+	}
+
 	skb = bt_skb_sendmsg(sk, msg, len, len, 0, 0);
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
@@ -794,7 +805,7 @@ static int sco_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	lock_sock(sk);
 
 	if (sk->sk_state == BT_CONNECTED)
-		err = sco_send_frame(sk, skb);
+		err = sco_send_frame(sk, skb, &sockc);
 	else
 		err = -ENOTCONN;
 
@@ -860,6 +871,10 @@ static int sco_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct sock *sk = sock->sk;
 	struct sco_pinfo *pi = sco_pi(sk);
 
+	if (unlikely(flags & MSG_ERRQUEUE))
+		return sock_recv_errqueue(sk, msg, len, SOL_BLUETOOTH,
+					  BT_SCM_ERROR);
+
 	lock_sock(sk);
 
 	if (sk->sk_state == BT_CONNECT2 &&
-- 
2.48.1

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

[Willem de Bruijn]

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@iki.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"?

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

[Jason Xing]

Hi Pauli,

On Sun, Feb 9, 2025 at 6:40 PM 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, where hardware tx timestamps
> cannot be obtained except for ISO packets, and the hardware has a queue

Could you say more about why the hw timestamp cannot be obtained?

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

It's worth mentioning the earlier discussion that took place last year
in the commit log. It's hard to retrieve such an important discussion
buried in the mailing list, which should be helpful for people to get
to know/recall the exact background :)

https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/.
https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/

And it's also good to attach the real use case from the following link
for readers to know the exact case :)

https://lore.kernel.org/all/7ade362f178297751e8a0846e0342d5086623edc.camel@iki.fi/
Quoting here:
"
sendmsg() from user generates skbs to net/bluetooth side queue
|
* wait in net/bluetooth side queue until HW has free packet slot
|
* send to driver (-> SCM_TSTAMP_SCHED*)
|
* driver (usu. ASAP) queues to transport e.g. USB
|
* transport tx complete, skb freed
|
* packet waits in hardware-side buffers (usu. the largest delay)
|
* packet completion report from HW (-> SCM_TSTAMP_SND*)
|
* for one packet type, HW timestamp for last tx packet can queried
The packet completion report does not imply the packet was received.
"

> Signed-off-by: Pauli Virtanen <pav@iki.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.
> +

I'd like to know if this flag can also be applied to NICs which have
already implemented the hardware timestamp, like Intel i40e, no?

Thanks,
Jason


>
>  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)
>  #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",
>  };
>  static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT);
>
> diff --git a/net/socket.c b/net/socket.c
> index 4afe31656a2b..22b7f6f50889 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -693,6 +693,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
>         if (tsflags & SOF_TIMESTAMPING_TX_SCHED)
>                 flags |= SKBTX_SCHED_TSTAMP;
>
> +       if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION)
> +               flags |= SKBTX_COMPLETION_TSTAMP;
> +
>         *tx_flags = flags;
>  }
>  EXPORT_SYMBOL(__sock_tx_timestamp);
> --
> 2.48.1
>
>

Re: [PATCH v3 3/5] Bluetooth: ISO: add TX timestamping

[Jason Xing]

On Sun, Feb 9, 2025 at 6:39 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add BT_SCM_ERROR socket CMSG type.
>
> Support TX timestamping in ISO sockets.
>
> Support MSG_ERRQUEUE in ISO recvmsg.
>
> If a packet from sendmsg() is fragmented, only the first ACL fragment is
> timestamped.
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  include/net/bluetooth/bluetooth.h |  1 +
>  net/bluetooth/iso.c               | 24 ++++++++++++++++++++----
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 435250c72d56..bbefde319f95 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -156,6 +156,7 @@ struct bt_voice {
>  #define BT_PKT_STATUS           16
>
>  #define BT_SCM_PKT_STATUS      0x03
> +#define BT_SCM_ERROR           0x04
>
>  #define BT_ISO_QOS             17
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 44acddf58a0c..f497759a2af5 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -518,7 +518,8 @@ static struct bt_iso_qos *iso_sock_get_qos(struct sock *sk)
>         return &iso_pi(sk)->qos;
>  }
>
> -static int iso_send_frame(struct sock *sk, struct sk_buff *skb)
> +static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> +                         const struct sockcm_cookie *sockc)
>  {
>         struct iso_conn *conn = iso_pi(sk)->conn;
>         struct bt_iso_qos *qos = iso_sock_get_qos(sk);
> @@ -538,10 +539,12 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb)
>         hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len,
>                                                       HCI_ISO_STATUS_VALID));
>
> -       if (sk->sk_state == BT_CONNECTED)
> +       if (sk->sk_state == BT_CONNECTED) {
> +               hci_setup_tx_timestamp(skb, 1, sockc);
>                 hci_send_iso(conn->hcon, skb);
> -       else
> +       } else {
>                 len = -ENOTCONN;
> +       }
>
>         return len;
>  }
> @@ -1348,6 +1351,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>  {
>         struct sock *sk = sock->sk;
>         struct sk_buff *skb, **frag;
> +       struct sockcm_cookie sockc;
>         size_t mtu;
>         int err;
>
> @@ -1360,6 +1364,14 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>         if (msg->msg_flags & MSG_OOB)
>                 return -EOPNOTSUPP;
>
> +       sockcm_init(&sockc, sk);

No need to initialize other irrelevant fields since Willem started to
clean up this kind of init phase in TCP[1].

[1]: https://lore.kernel.org/all/20250206193521.2285488-2-willemdebruijn.kernel@gmail.com/

> +
> +       if (msg->msg_controllen) {
> +               err = sock_cmsg_send(sk, msg, &sockc);
> +               if (err)
> +                       return err;
> +       }
> +

I'm not familiar with bluetooth, but I'm wondering if the above code
snippet is supposed to be protected by the socket lock as below since
I refer to TCP as an example? Is it possible that multiple threads
call this sendmsg at the same time?

>         lock_sock(sk);
>
>         if (sk->sk_state != BT_CONNECTED) {
> @@ -1405,7 +1417,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>         lock_sock(sk);
>
>         if (sk->sk_state == BT_CONNECTED)
> -               err = iso_send_frame(sk, skb);
> +               err = iso_send_frame(sk, skb, &sockc);
>         else
>                 err = -ENOTCONN;
>
> @@ -1474,6 +1486,10 @@ static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>
>         BT_DBG("sk %p", sk);
>
> +       if (unlikely(flags & MSG_ERRQUEUE))
> +               return sock_recv_errqueue(sk, msg, len, SOL_BLUETOOTH,
> +                                         BT_SCM_ERROR);
> +
>         if (test_and_clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
>                 sock_hold(sk);
>                 lock_sock(sk);
> --
> 2.48.1
>
>

Re: [PATCH v3 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping

[Jason Xing]

On Sun, Feb 9, 2025 at 6:40 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Support enabling TX timestamping for some skbs, and track them until
> packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> completion report from hardware.
>
> Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> driver requires changes in the driver API, and drivers mostly are going
> to send the skb immediately.
>
> Make the default situation with no COMPLETION TX timestamping more
> efficient by only counting packets in the queue when there is nothing to
> track.  When there is something to track, we need to make clones, since
> the driver may modify sent skbs.
>
> The tx_q queue length is bounded by the hdev flow control, which will
> not send new packets before it has got completion reports for old ones.
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  include/net/bluetooth/hci_core.h |  13 ++++
>  net/bluetooth/hci_conn.c         | 117 +++++++++++++++++++++++++++++++
>  net/bluetooth/hci_core.c         |  17 +++--
>  net/bluetooth/hci_event.c        |   4 ++
>  4 files changed, 146 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 05919848ea95..1f539a9881ad 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -261,6 +261,12 @@ struct adv_info {
>         struct delayed_work     rpa_expired_cb;
>  };
>
> +struct tx_queue {
> +       struct sk_buff_head queue;
> +       unsigned int extra;
> +       unsigned int tracked;
> +};
> +
>  #define HCI_MAX_ADV_INSTANCES          5
>  #define HCI_DEFAULT_ADV_DURATION       2
>
> @@ -733,6 +739,8 @@ struct hci_conn {
>         struct sk_buff_head data_q;
>         struct list_head chan_list;
>
> +       struct tx_queue tx_q;
> +
>         struct delayed_work disc_work;
>         struct delayed_work auto_accept_work;
>         struct delayed_work idle_work;
> @@ -1571,6 +1579,11 @@ void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
>  void hci_conn_failed(struct hci_conn *conn, u8 status);
>  u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
>
> +void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb);
> +void hci_conn_tx_dequeue(struct hci_conn *conn);
> +void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
> +                           const struct sockcm_cookie *sockc);
> +
>  /*
>   * hci_conn_get() and hci_conn_put() are used to control the life-time of an
>   * "hci_conn" object. They do not guarantee that the hci_conn object is running,
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index d097e308a755..e437290d8b70 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -27,6 +27,7 @@
>
>  #include <linux/export.h>
>  #include <linux/debugfs.h>
> +#include <linux/errqueue.h>
>
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> @@ -1002,6 +1003,7 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t
>         }
>
>         skb_queue_head_init(&conn->data_q);
> +       skb_queue_head_init(&conn->tx_q.queue);
>
>         INIT_LIST_HEAD(&conn->chan_list);
>         INIT_LIST_HEAD(&conn->link_list);
> @@ -1155,6 +1157,7 @@ void hci_conn_del(struct hci_conn *conn)
>         }
>
>         skb_queue_purge(&conn->data_q);
> +       skb_queue_purge(&conn->tx_q.queue);
>
>         /* Remove the connection from the list and cleanup its remaining
>          * state. This is a separate function since for some cases like
> @@ -3064,3 +3067,117 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
>          */
>         return hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
>  }
> +
> +void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
> +                           const struct sockcm_cookie *sockc)
> +{
> +       struct sock *sk = skb ? skb->sk : NULL;
> +
> +       /* This shall be called on a single skb of those generated by user
> +        * sendmsg(), and only when the sendmsg() does not return error to
> +        * user. This is required for keeping the tskey that increments here in
> +        * sync with possible sendmsg() counting by user.
> +        *
> +        * Stream sockets shall set key_offset to sendmsg() length in bytes
> +        * and call with the last fragment, others to 1 and first fragment.
> +        */
> +
> +       if (!skb || !sockc || !sk || !key_offset)
> +               return;
> +
> +       sock_tx_timestamp(sk, sockc, &skb_shinfo(skb)->tx_flags);
> +
> +       if (sockc->tsflags & SOF_TIMESTAMPING_OPT_ID &&
> +           sockc->tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) {
> +               if (sockc->tsflags & SOCKCM_FLAG_TS_OPT_ID) {
> +                       skb_shinfo(skb)->tskey = sockc->ts_opt_id;

Will applications take control of managing the tskey on their own
instead of kernel in the future?

> +               } else {
> +                       int key = atomic_add_return(key_offset, &sk->sk_tskey);
> +
> +                       skb_shinfo(skb)->tskey = key - 1;
> +               }
> +       }
> +}
> +
> +void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
> +{
> +       struct tx_queue *comp = &conn->tx_q;
> +       bool track = false;
> +
> +       /* Emit SND now, ie. just before sending to driver */
> +       if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP))

Nit: I think it's not necessary to test if skb->sk is NULL here since
__skb_tstamp_tx() will do the check.

> +               __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SND);
> +
> +       /* COMPLETION tstamp is emitted for tracked skb later in Number of
> +        * Completed Packets event. Available only for flow controlled cases.
> +        *
> +        * TODO: SCO support (needs to be done in drivers)
> +        */
> +       switch (conn->type) {
> +       case ISO_LINK:
> +       case ACL_LINK:
> +       case LE_LINK:
> +               break;
> +       default:
> +               return;
> +       }
> +
> +       if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> +               track = true;
> +
> +       /* If nothing is tracked, just count extra skbs at the queue head */
> +       if (!track && !comp->tracked) {
> +               comp->extra++;
> +               return;
> +       }
> +
> +       if (track) {
> +               skb = skb_clone_sk(skb);
> +               if (!skb)
> +                       goto count_only;
> +
> +               comp->tracked++;
> +       } else {
> +               skb = skb_clone(skb, GFP_KERNEL);
> +               if (!skb)
> +                       goto count_only;
> +       }
> +
> +       skb_queue_tail(&comp->queue, skb);
> +       return;
> +
> +count_only:
> +       /* Stop tracking skbs, and only count. This will not emit timestamps for
> +        * the packets, but if we get here something is more seriously wrong.
> +        */
> +       comp->tracked = 0;
> +       comp->extra += skb_queue_len(&comp->queue) + 1;
> +       skb_queue_purge(&comp->queue);
> +}
> +
> +void hci_conn_tx_dequeue(struct hci_conn *conn)
> +{
> +       struct tx_queue *comp = &conn->tx_q;
> +       struct sk_buff *skb;
> +
> +       /* If there are tracked skbs, the counted extra go before dequeuing real
> +        * skbs, to keep ordering. When nothing is tracked, the ordering doesn't
> +        * matter so dequeue real skbs first to get rid of them ASAP.
> +        */
> +       if (comp->extra && (comp->tracked || skb_queue_empty(&comp->queue))) {
> +               comp->extra--;
> +               return;
> +       }
> +
> +       skb = skb_dequeue(&comp->queue);
> +       if (!skb)
> +               return;
> +
> +       if (skb->sk) {
> +               comp->tracked--;
> +               __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> +                               SCM_TSTAMP_COMPLETION);
> +       }
> +
> +       kfree_skb(skb);
> +}
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index e7ec12437c8b..e0845188f626 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3025,6 +3025,13 @@ static int hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>         return 0;
>  }
>
> +static int hci_send_conn_frame(struct hci_dev *hdev, struct hci_conn *conn,
> +                              struct sk_buff *skb)
> +{
> +       hci_conn_tx_queue(conn, skb);
> +       return hci_send_frame(hdev, skb);
> +}
> +
>  /* Send HCI command */
>  int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen,
>                  const void *param)
> @@ -3562,7 +3569,7 @@ static void hci_sched_sco(struct hci_dev *hdev)
>         while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
>                 while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>                         BT_DBG("skb %p len %d", skb, skb->len);
> -                       hci_send_frame(hdev, skb);
> +                       hci_send_conn_frame(hdev, conn, skb);
>
>                         conn->sent++;
>                         if (conn->sent == ~0)
> @@ -3586,7 +3593,7 @@ static void hci_sched_esco(struct hci_dev *hdev)
>                                                      &quote))) {
>                 while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>                         BT_DBG("skb %p len %d", skb, skb->len);
> -                       hci_send_frame(hdev, skb);
> +                       hci_send_conn_frame(hdev, conn, skb);
>
>                         conn->sent++;
>                         if (conn->sent == ~0)
> @@ -3620,7 +3627,7 @@ static void hci_sched_acl_pkt(struct hci_dev *hdev)
>                         hci_conn_enter_active_mode(chan->conn,
>                                                    bt_cb(skb)->force_active);
>
> -                       hci_send_frame(hdev, skb);
> +                       hci_send_conn_frame(hdev, chan->conn, skb);
>                         hdev->acl_last_tx = jiffies;
>
>                         hdev->acl_cnt--;
> @@ -3676,7 +3683,7 @@ static void hci_sched_le(struct hci_dev *hdev)
>
>                         skb = skb_dequeue(&chan->data_q);
>
> -                       hci_send_frame(hdev, skb);
> +                       hci_send_conn_frame(hdev, chan->conn, skb);
>                         hdev->le_last_tx = jiffies;
>
>                         (*cnt)--;
> @@ -3710,7 +3717,7 @@ static void hci_sched_iso(struct hci_dev *hdev)
>         while (*cnt && (conn = hci_low_sent(hdev, ISO_LINK, &quote))) {
>                 while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
>                         BT_DBG("skb %p len %d", skb, skb->len);
> -                       hci_send_frame(hdev, skb);
> +                       hci_send_conn_frame(hdev, conn, skb);
>
>                         conn->sent++;
>                         if (conn->sent == ~0)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 2cc7a9306350..144b442180f7 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4405,6 +4405,7 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
>                 struct hci_comp_pkts_info *info = &ev->handles[i];
>                 struct hci_conn *conn;
>                 __u16  handle, count;
> +               unsigned int i;
>
>                 handle = __le16_to_cpu(info->handle);
>                 count  = __le16_to_cpu(info->count);
> @@ -4415,6 +4416,9 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
>
>                 conn->sent -= count;
>
> +               for (i = 0; i < count; ++i)
> +                       hci_conn_tx_dequeue(conn);
> +
>                 switch (conn->type) {
>                 case ACL_LINK:
>                         hdev->acl_cnt += count;
> --
> 2.48.1
>
>

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

[Pauli Virtanen]

Hi,

su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti:
> 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.

For BT ISO packets, in theory hw-provided TX timestamps exist, and we
might want both (with separate flags for enabling them). I don't really
know, last I looked Intel HW didn't support them, and it's not clear to
which degree they are useful.

> That said, +1 on adding explicit well defined measurement point
> instead.
>
> > Signed-off-by: Pauli Virtanen <pav@iki.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.

Ok. If possible, I'd like to avoid changing the behavior of the non-
Bluetooth parts of net/ here, as I'm not familiar with those.

I guess a simpler solution could be that sock_set_timestamping() checks
the type of the socket, and gives EINVAL if the flag is set for non-
Bluetooth sockets?

One could then postpone having to invent how to check the driver
support, and user would know non-supported status from setsockopt
failing.

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

If doing it per-protocol sounds ok, it could be put in bt_skb_cb
instead.

Since the completion timestamp didn't already exist, it maybe means
it's probably not that important for other parts of net/

> >  #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"?

Ok.

-- 
Pauli Virtanen

Re: [PATCH v3 3/5] Bluetooth: ISO: add TX timestamping

[Pauli Virtanen]

Hi,

ma, 2025-02-10 kello 13:19 +0800, Jason Xing kirjoitti:
> On Sun, Feb 9, 2025 at 6:39 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Add BT_SCM_ERROR socket CMSG type.
> > 
> > Support TX timestamping in ISO sockets.
> > 
> > Support MSG_ERRQUEUE in ISO recvmsg.
> > 
> > If a packet from sendmsg() is fragmented, only the first ACL fragment is
> > timestamped.
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >  include/net/bluetooth/bluetooth.h |  1 +
> >  net/bluetooth/iso.c               | 24 ++++++++++++++++++++----
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 435250c72d56..bbefde319f95 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -156,6 +156,7 @@ struct bt_voice {
> >  #define BT_PKT_STATUS           16
> > 
> >  #define BT_SCM_PKT_STATUS      0x03
> > +#define BT_SCM_ERROR           0x04
> > 
> >  #define BT_ISO_QOS             17
> > 
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index 44acddf58a0c..f497759a2af5 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -518,7 +518,8 @@ static struct bt_iso_qos *iso_sock_get_qos(struct sock *sk)
> >         return &iso_pi(sk)->qos;
> >  }
> > 
> > -static int iso_send_frame(struct sock *sk, struct sk_buff *skb)
> > +static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> > +                         const struct sockcm_cookie *sockc)
> >  {
> >         struct iso_conn *conn = iso_pi(sk)->conn;
> >         struct bt_iso_qos *qos = iso_sock_get_qos(sk);
> > @@ -538,10 +539,12 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb)
> >         hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len,
> >                                                       HCI_ISO_STATUS_VALID));
> > 
> > -       if (sk->sk_state == BT_CONNECTED)
> > +       if (sk->sk_state == BT_CONNECTED) {
> > +               hci_setup_tx_timestamp(skb, 1, sockc);
> >                 hci_send_iso(conn->hcon, skb);
> > -       else
> > +       } else {
> >                 len = -ENOTCONN;
> > +       }
> > 
> >         return len;
> >  }
> > @@ -1348,6 +1351,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> >  {
> >         struct sock *sk = sock->sk;
> >         struct sk_buff *skb, **frag;
> > +       struct sockcm_cookie sockc;
> >         size_t mtu;
> >         int err;
> > 
> > @@ -1360,6 +1364,14 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> >         if (msg->msg_flags & MSG_OOB)
> >                 return -EOPNOTSUPP;
> > 
> > +       sockcm_init(&sockc, sk);
> 
> No need to initialize other irrelevant fields since Willem started to
> clean up this kind of init phase in TCP[1].
> 
> [1]: https://lore.kernel.org/all/20250206193521.2285488-2-willemdebruijn.kernel@gmail.com/

Ok.

> > +
> > +       if (msg->msg_controllen) {
> > +               err = sock_cmsg_send(sk, msg, &sockc);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> 
> I'm not familiar with bluetooth, but I'm wondering if the above code
> snippet is supposed to be protected by the socket lock as below since
> I refer to TCP as an example? Is it possible that multiple threads
> call this sendmsg at the same time?

I think parallel sendmsg() is possible, but I understood sock_cmsg_send
itself doesn't need lock_sock(), if I read correctly e.g. udp_sendmsg()
seems to be using it via ip_cmsg_send() unlocked.

The lock_sock() that are below IIRC protect the invariant that 
sk->sk_state == BT_CONNECTED has special meaning here.
> 

> >         lock_sock(sk);
> > 
> >         if (sk->sk_state != BT_CONNECTED) {
> > @@ -1405,7 +1417,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> >         lock_sock(sk);
> > 
> >         if (sk->sk_state == BT_CONNECTED)
> > -               err = iso_send_frame(sk, skb);
> > +               err = iso_send_frame(sk, skb, &sockc);
> >         else
> >                 err = -ENOTCONN;
> > 
> > @@ -1474,6 +1486,10 @@ static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> > 
> >         BT_DBG("sk %p", sk);
> > 
> > +       if (unlikely(flags & MSG_ERRQUEUE))
> > +               return sock_recv_errqueue(sk, msg, len, SOL_BLUETOOTH,
> > +                                         BT_SCM_ERROR);
> > +
> >         if (test_and_clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
> >                 sock_hold(sk);
> >                 lock_sock(sk);
> > --
> > 2.48.1
> > 
> > 

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

[Willem de Bruijn]

Pauli Virtanen wrote:
> Hi,
> 
> su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti:
> > 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.
> 
> For BT ISO packets, in theory hw-provided TX timestamps exist, and we
> might want both (with separate flags for enabling them). I don't really
> know, last I looked Intel HW didn't support them, and it's not clear to
> which degree they are useful.

That's reason enough to separate these measurement types.

If we don't do it properly now, we won't be able to update drivers
later once users depend on requesting hw timestamps when they mean to
get completion timestamps.

> > That said, +1 on adding explicit well defined measurement point
> > instead.
> >
> > > Signed-off-by: Pauli Virtanen <pav@iki.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.
> 
> Ok. If possible, I'd like to avoid changing the behavior of the non-
> Bluetooth parts of net/ here, as I'm not familiar with those.
> 
> I guess a simpler solution could be that sock_set_timestamping() checks
> the type of the socket, and gives EINVAL if the flag is set for non-
> Bluetooth sockets?

Actually, I'd prefer to have this completion timestamp ability for all
drivers. And avoid creating subsystem private mechanisms.

I suppose we can punt on the get_ts_info control API if need be.
 
> One could then postpone having to invent how to check the driver
> support, and user would know non-supported status from setsockopt
> failing.
> 
> > >  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.
> 
> If doing it per-protocol sounds ok, it could be put in bt_skb_cb
> instead.
> 
> Since the completion timestamp didn't already exist, it maybe means
> it's probably not that important for other parts of net/

I can see its value especially for hardware that does not support
hardware timestamps, or hw timestamps at line rate.

This gives a reasonable estimation of transmission time and
measure of device delay.

It is device specific whether it will be an over- or under-estimation,
depending on whether the completion is queued to the host after or
before the data is written on the wire. But either way, it will
include the delay in processing the tx queue, which on multi-queue
NICs and with TSO may be substantial (even before considering HW
rate limiting).

> > >  #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"?
> 
> Ok.
> 
> -- 
> Pauli Virtanen

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

[Luiz Augusto von Dentz]

Hi Willem,

On Mon, Feb 10, 2025 at 1:43 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Pauli Virtanen wrote:
> > Hi,
> >
> > su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti:
> > > 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.
> >
> > For BT ISO packets, in theory hw-provided TX timestamps exist, and we
> > might want both (with separate flags for enabling them). I don't really
> > know, last I looked Intel HW didn't support them, and it's not clear to
> > which degree they are useful.
>
> That's reason enough to separate these measurement types.
>
> If we don't do it properly now, we won't be able to update drivers
> later once users depend on requesting hw timestamps when they mean to
> get completion timestamps.
>
> > > That said, +1 on adding explicit well defined measurement point
> > > instead.
> > >
> > > > Signed-off-by: Pauli Virtanen <pav@iki.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.
> >
> > Ok. If possible, I'd like to avoid changing the behavior of the non-
> > Bluetooth parts of net/ here, as I'm not familiar with those.
> >
> > I guess a simpler solution could be that sock_set_timestamping() checks
> > the type of the socket, and gives EINVAL if the flag is set for non-
> > Bluetooth sockets?
>
> Actually, I'd prefer to have this completion timestamp ability for all
> drivers. And avoid creating subsystem private mechanisms.
>
> I suppose we can punt on the get_ts_info control API if need be.

I guess that it is reasonable if we don't have to do the work for
drivers other than Bluetooth otherwise I'd say you are probably asking
too much here, also doesn't this land on the TSN space if one needs to
tightly control timings? I suspect if this sort of change was not
necessary for TSN then perhaps it wouldn't be of much value to try to
generalize this.

> > One could then postpone having to invent how to check the driver
> > support, and user would know non-supported status from setsockopt
> > failing.
> >
> > > >  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.
> >
> > If doing it per-protocol sounds ok, it could be put in bt_skb_cb
> > instead.
> >
> > Since the completion timestamp didn't already exist, it maybe means
> > it's probably not that important for other parts of net/
>
> I can see its value especially for hardware that does not support
> hardware timestamps, or hw timestamps at line rate.
>
> This gives a reasonable estimation of transmission time and
> measure of device delay.
>
> It is device specific whether it will be an over- or under-estimation,
> depending on whether the completion is queued to the host after or
> before the data is written on the wire. But either way, it will
> include the delay in processing the tx queue, which on multi-queue
> NICs and with TSO may be substantial (even before considering HW
> rate limiting).
>
> > > >  #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"?
> >
> > Ok.
> >
> > --
> > Pauli Virtanen
>
>


-- 
Luiz Augusto von Dentz

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

[Willem de Bruijn]

Luiz Augusto von Dentz wrote:
> Hi Willem,
> 
> On Mon, Feb 10, 2025 at 1:43 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Pauli Virtanen wrote:
> > > Hi,
> > >
> > > su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti:
> > > > 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.
> > >
> > > For BT ISO packets, in theory hw-provided TX timestamps exist, and we
> > > might want both (with separate flags for enabling them). I don't really
> > > know, last I looked Intel HW didn't support them, and it's not clear to
> > > which degree they are useful.
> >
> > That's reason enough to separate these measurement types.
> >
> > If we don't do it properly now, we won't be able to update drivers
> > later once users depend on requesting hw timestamps when they mean to
> > get completion timestamps.
> >
> > > > That said, +1 on adding explicit well defined measurement point
> > > > instead.
> > > >
> > > > > Signed-off-by: Pauli Virtanen <pav@iki.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.
> > >
> > > Ok. If possible, I'd like to avoid changing the behavior of the non-
> > > Bluetooth parts of net/ here, as I'm not familiar with those.
> > >
> > > I guess a simpler solution could be that sock_set_timestamping() checks
> > > the type of the socket, and gives EINVAL if the flag is set for non-
> > > Bluetooth sockets?
> >
> > Actually, I'd prefer to have this completion timestamp ability for all
> > drivers. And avoid creating subsystem private mechanisms.
> >
> > I suppose we can punt on the get_ts_info control API if need be.
> 
> I guess that it is reasonable if we don't have to do the work for
> drivers other than Bluetooth otherwise I'd say you are probably asking
> too much here,

I was mainly agreeing to Pauli's implementation in this series.
Not asking to do any work irrelevant to BT.

> also doesn't this land on the TSN space if one needs to
> tightly control timings? I suspect if this sort of change was not
> necessary for TSN then perhaps it wouldn't be of much value to try to
> generalize this.

I don't fully follow. But in general SO_TIMESTAMPING is not limited to
TSN.

> 
> > > One could then postpone having to invent how to check the driver
> > > support, and user would know non-supported status from setsockopt
> > > failing.
> > >
> > > > >  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.
> > >
> > > If doing it per-protocol sounds ok, it could be put in bt_skb_cb
> > > instead.
> > >
> > > Since the completion timestamp didn't already exist, it maybe means
> > > it's probably not that important for other parts of net/
> >
> > I can see its value especially for hardware that does not support
> > hardware timestamps, or hw timestamps at line rate.
> >
> > This gives a reasonable estimation of transmission time and
> > measure of device delay.
> >
> > It is device specific whether it will be an over- or under-estimation,
> > depending on whether the completion is queued to the host after or
> > before the data is written on the wire. But either way, it will
> > include the delay in processing the tx queue, which on multi-queue
> > NICs and with TSO may be substantial (even before considering HW
> > rate limiting).
> >
> > > > >  #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"?
> > >
> > > Ok.
> > >
> > > --
> > > Pauli Virtanen
> >
> >
> 
> 
> -- 
> Luiz Augusto von Dentz

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

[Pauli Virtanen]

Hi,

su, 2025-02-09 kello 22:29 -0500, Willem de Bruijn kirjoitti:
> Pauli Virtanen wrote:
> 
> >  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.

One alternative here could be:

Make SOF_TIMESTAMPING_TX_COMPLETION available only as a socket option,
and make it emit COMPLETION tstamp exactly for those packets that also
have SOF_TIMESTAMPING_TX_SOFTWARE enabled. User apps can then still
timestamp only selected packets via CMSG, however the choice between
SND and SND+COMPLETION is socket-wide. The API is then less uniform,
but probably usable in practice, although some use cases may not be
interested in the extra SND tstamps.

IIUC, sizeof skb_shared_info cannot be easily changed and I'm not sure
if there is room left there. So if the option of putting it into
protocol-specific skb cb is also out, then I'm not sure what the plan
here should be.

-- 
Pauli Virtanen