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

[PATCH v4 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)

Previous discussions:
https://lore.kernel.org/linux-bluetooth/cover.1739097311.git.pav@iki.fi/
https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/
https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/

Changes
=======
v4:
- Change meaning of SOF_TIMESTAMPING_TX_COMPLETION, to save a bit in
  skb_shared_info.tx_flags:

  It now enables COMPLETION only for packets that also have software SND
  enabled.  The flag can now be enabled only via a socket option, but
  coupling with SND allows user to still choose for which packets
  SND+COMPLETION should be generated.  This choice maybe is OK for
  applications which can skip SND if they're not interested.

  However, this would make the timestamping API not uniform, as the
  other TX flags can be enabled via CMSG.

  IIUC, sizeof skb_shared_info cannot be easily changed and I'm not sure
  there is somewhere else in general skb info, where one could safely
  put the extra separate flag bit for COMPLETION. So here's alternative
  suggestion.

- Better name in sof_timestamping_names

- I decided to keep using sockcm_init(), to avoid open coding READ_ONCE
  and since it's passed to sock_cmsg_send() which anyway also may init
  such fields.

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.

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.

Tests
=====

See
https://lore.kernel.org/linux-bluetooth/cover.1739026302.git.pav@iki.fi/

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/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                  | 118 ++++++++++++++++++++++
 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/core/sock.c                           |   2 +
 net/ethtool/common.c                      |   1 +
 17 files changed, 258 insertions(+), 20 deletions(-)

-- 
2.48.1

[PATCH v4 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, as hardware timestamps do not
exist in the HCI specification except for ISO packets, and the hardware
has a queue where packets may wait.  In this case the software SND
timestamp only reflects the kernel-side part of the total latency
(usually small) and queue length (usually 0 unless HW buffers
congested), whereas the completion report time is more informative of
the true latency.

It may also be useful in other cases where HW TX timestamps cannot be
obtained and user wants to estimate an upper bound to when the TX
probably happened.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
        together with SND, to save a bit in skb_shared_info.tx_flags
    
        As it then cannot be set per-skb, reject setting it via CMSG.

 Documentation/networking/timestamping.rst | 9 +++++++++
 include/uapi/linux/errqueue.h             | 1 +
 include/uapi/linux/net_tstamp.h           | 6 ++++--
 net/core/sock.c                           | 2 ++
 net/ethtool/common.c                      | 1 +
 5 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
index 61ef9da10e28..5034dfe326c0 100644
--- a/Documentation/networking/timestamping.rst
+++ b/Documentation/networking/timestamping.rst
@@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
   cumulative acknowledgment. The mechanism ignores SACK and FACK.
   This flag can be enabled via both socket options and control messages.
 
+SOF_TIMESTAMPING_TX_COMPLETION:
+  Request tx timestamps on packet tx completion, for the packets that
+  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion
+  timestamp is generated by the kernel when it receives a packet
+  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. This flag can be enabled *only*
+  via a socket option.
+
 
 1.3.2 Timestamp Reporting
 ^^^^^^^^^^^^^^^^^^^^^^^^^
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/core/sock.c b/net/core/sock.c
index a197f0a0b878..76a5d5cb1e56 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2933,6 +2933,8 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
 		tsflags = *(u32 *)CMSG_DATA(cmsg);
 		if (tsflags & ~SOF_TIMESTAMPING_TX_RECORD_MASK)
 			return -EINVAL;
+		if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION)
+			return -EINVAL;
 
 		sockc->tsflags &= ~SOF_TIMESTAMPING_TX_RECORD_MASK;
 		sockc->tsflags |= tsflags;
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 5489d0c9d13f..ed4d6a9f4d7e 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -473,6 +473,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)] = "tx-completion",
 };
 static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT);
 
-- 
2.48.1

[PATCH v4 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>
---

Notes:
    v4: coupled COMPLETION & SND

 include/net/bluetooth/hci_core.h |  13 ++++
 net/bluetooth/hci_conn.c         | 118 +++++++++++++++++++++++++++++++
 net/bluetooth/hci_core.c         |  17 +++--
 net/bluetooth/hci_event.c        |   4 ++
 4 files changed, 147 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..f09206155456 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,118 @@ 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_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_SW_TSTAMP) &&
+	    READ_ONCE(skb->sk->sk_tsflags) & SOF_TIMESTAMPING_TX_COMPLETION)
+		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 v4 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>
---

Notes:
    v4: no changes

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

Notes:
    v4: no changes

 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 fec11e576f31..56a36e872e7b 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 v4 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>
---

Notes:
    v4: no changes

 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 v4 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO

[Luiz Augusto von Dentz]

Hi Pauli,

On Wed, Feb 19, 2025 at 1:13 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> 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)
>
> Previous discussions:
> https://lore.kernel.org/linux-bluetooth/cover.1739097311.git.pav@iki.fi/
> https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/
> https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/
>
> Changes
> =======
> v4:
> - Change meaning of SOF_TIMESTAMPING_TX_COMPLETION, to save a bit in
>   skb_shared_info.tx_flags:
>
>   It now enables COMPLETION only for packets that also have software SND
>   enabled.  The flag can now be enabled only via a socket option, but
>   coupling with SND allows user to still choose for which packets
>   SND+COMPLETION should be generated.  This choice maybe is OK for
>   applications which can skip SND if they're not interested.
>
>   However, this would make the timestamping API not uniform, as the
>   other TX flags can be enabled via CMSG.
>
>   IIUC, sizeof skb_shared_info cannot be easily changed and I'm not sure
>   there is somewhere else in general skb info, where one could safely
>   put the extra separate flag bit for COMPLETION. So here's alternative
>   suggestion.

Due to cloning/dup of socket by bluetoothd wouldn't it be better to
have the completion on a per-packet basis? Not really sure if that is
what setting it via CMSG would mean, but in the other hand perhaps the
problem is that the error queue is socket wide, not per-fd, anyway it
doesn't sound very useful to notify the completion on all fd pointing
to the same socket. Or perhaps it is time for introducing a proper TX
complete queue rather than reuse the error queue? I mean we can keep
using the error queue for backwards compatibility but moving forward I
think it would be better not to mix errors with tx complete events, so
perhaps we can add something like a socket option that dissociates the
error queue from tx completion queue.

> - Better name in sof_timestamping_names
>
> - I decided to keep using sockcm_init(), to avoid open coding READ_ONCE
>   and since it's passed to sock_cmsg_send() which anyway also may init
>   such fields.
>
> 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.
>
> 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.
>
> Tests
> =====
>
> See
> https://lore.kernel.org/linux-bluetooth/cover.1739026302.git.pav@iki.fi/
>
> 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/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                  | 118 ++++++++++++++++++++++
>  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/core/sock.c                           |   2 +
>  net/ethtool/common.c                      |   1 +
>  17 files changed, 258 insertions(+), 20 deletions(-)
>
> --
> 2.48.1
>


-- 
Luiz Augusto von Dentz

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

[Pauli Virtanen]

Hi,

ke, 2025-02-19 kello 14:55 -0500, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Wed, Feb 19, 2025 at 1:13 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > 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)
> > 
> > Previous discussions:
> > https://lore.kernel.org/linux-bluetooth/cover.1739097311.git.pav@iki.fi/
> > https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/
> > https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/
> > 
> > Changes
> > =======
> > v4:
> > - Change meaning of SOF_TIMESTAMPING_TX_COMPLETION, to save a bit in
> >   skb_shared_info.tx_flags:
> > 
> >   It now enables COMPLETION only for packets that also have software SND
> >   enabled.  The flag can now be enabled only via a socket option, but
> >   coupling with SND allows user to still choose for which packets
> >   SND+COMPLETION should be generated.  This choice maybe is OK for
> >   applications which can skip SND if they're not interested.
> > 
> >   However, this would make the timestamping API not uniform, as the
> >   other TX flags can be enabled via CMSG.
> > 
> >   IIUC, sizeof skb_shared_info cannot be easily changed and I'm not sure
> >   there is somewhere else in general skb info, where one could safely
> >   put the extra separate flag bit for COMPLETION. So here's alternative
> >   suggestion.
>
> Due to cloning/dup of socket by bluetoothd wouldn't it be better to
> have the completion on a per-packet basis?

So this is about a few lines change to v3, not a big redesign.

This change is a suggestion in response to the concern on using the
last available bit in tx_flags raised here:

https://lore.kernel.org/linux-bluetooth/67a972a6e2704_14761294b0@willemb.c.googlers.com.notmuch/

So normally how it works is that based on socket flags and CMSG, skb
info is tagged with bit flags that indicate which tstamps they emit.
The v3->v4 change just avoids needing another bit for COMPLETION, by
using the existing bit for SND tstamp together with socket flag (which
in v3 instead controlled whether skbs are tagged for COMPLETION) to
decide between SND and SND+COMPLETION.

So I'm not sure if this sounds preferable over using the last free
tx_flags bit to indicate COMPLETION is requested (or if there is some
other place in skb to put a bit that I'm not seeing).

>  Not really sure if that is what setting it via CMSG would mean,

I was here referring to the CMSG timestamping API, Sec 1.3.4 in

https://www.kernel.org/doc/Documentation/networking/timestamping.rst

> but in the other hand perhaps the
> problem is that the error queue is socket wide, not per-fd, anyway it
> doesn't sound very useful to notify the completion on all fd pointing
> to the same socket. Or perhaps it is time for introducing a proper TX
> complete queue rather than reuse the error queue? I mean we can keep
> using the error queue for backwards compatibility but moving forward I
> think it would be better not to mix errors with tx complete events, so
> perhaps we can add something like a socket option that dissociates the
> error queue from tx completion queue.


Some good solution to the issue of wakeups on timestamps and multiple
errqueue consumers would be useful, but that probably requires a
separate patch series.

> > - Better name in sof_timestamping_names
> > 
> > - I decided to keep using sockcm_init(), to avoid open coding READ_ONCE
> >   and since it's passed to sock_cmsg_send() which anyway also may init
> >   such fields.
> > 
> > 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.
> > 
> > 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.
> > 
> > Tests
> > =====
> > 
> > See
> > https://lore.kernel.org/linux-bluetooth/cover.1739026302.git.pav@iki.fi/
> > 
> > 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/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                  | 118 ++++++++++++++++++++++
> >  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/core/sock.c                           |   2 +
> >  net/ethtool/common.c                      |   1 +
> >  17 files changed, 258 insertions(+), 20 deletions(-)
> > 
> > --
> > 2.48.1
> > 
> 
> 

-- 
Pauli Virtanen

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

[Jason Xing]

On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> 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)
>
> Previous discussions:
> https://lore.kernel.org/linux-bluetooth/cover.1739097311.git.pav@iki.fi/
> https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/
> https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/
>
> Changes
> =======
> v4:
> - Change meaning of SOF_TIMESTAMPING_TX_COMPLETION, to save a bit in
>   skb_shared_info.tx_flags:
>
>   It now enables COMPLETION only for packets that also have software SND
>   enabled.  The flag can now be enabled only via a socket option, but
>   coupling with SND allows user to still choose for which packets
>   SND+COMPLETION should be generated.  This choice maybe is OK for
>   applications which can skip SND if they're not interested.
>
>   However, this would make the timestamping API not uniform, as the
>   other TX flags can be enabled via CMSG.
>
>   IIUC, sizeof skb_shared_info cannot be easily changed and I'm not sure
>   there is somewhere else in general skb info, where one could safely
>   put the extra separate flag bit for COMPLETION. So here's alternative
>   suggestion.
>
> - Better name in sof_timestamping_names
>
> - I decided to keep using sockcm_init(), to avoid open coding READ_ONCE
>   and since it's passed to sock_cmsg_send() which anyway also may init
>   such fields.

Please don't do this since the current sockcm_init() initializes more
than you need. Please take a look at what the new sockcm_init looks
like and what this series has done[1] which just got merged in recent
days :)

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=aefd232de5eb2e7

Thanks,
Jason

>
> 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.
>
> 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.
>
> Tests
> =====
>
> See
> https://lore.kernel.org/linux-bluetooth/cover.1739026302.git.pav@iki.fi/
>
> 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/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                  | 118 ++++++++++++++++++++++
>  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/core/sock.c                           |   2 +
>  net/ethtool/common.c                      |   1 +
>  17 files changed, 258 insertions(+), 20 deletions(-)
>
> --
> 2.48.1
>
>

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

[Jason Xing]

On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> when hardware reports a packet completed.
>
> Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> exist in the HCI specification except for ISO packets, and the hardware
> has a queue where packets may wait.  In this case the software SND
> timestamp only reflects the kernel-side part of the total latency
> (usually small) and queue length (usually 0 unless HW buffers
> congested), whereas the completion report time is more informative of
> the true latency.
>
> It may also be useful in other cases where HW TX timestamps cannot be
> obtained and user wants to estimate an upper bound to when the TX
> probably happened.
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
>     v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
>         together with SND, to save a bit in skb_shared_info.tx_flags
>
>         As it then cannot be set per-skb, reject setting it via CMSG.
>
>  Documentation/networking/timestamping.rst | 9 +++++++++
>  include/uapi/linux/errqueue.h             | 1 +
>  include/uapi/linux/net_tstamp.h           | 6 ++++--
>  net/core/sock.c                           | 2 ++
>  net/ethtool/common.c                      | 1 +
>  5 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 61ef9da10e28..5034dfe326c0 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
>    cumulative acknowledgment. The mechanism ignores SACK and FACK.
>    This flag can be enabled via both socket options and control messages.
>
> +SOF_TIMESTAMPING_TX_COMPLETION:
> +  Request tx timestamps on packet tx completion, for the packets that
> +  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion

Is it mandatory for other drivers that will try to use
SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled
both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be
better if you add the limitation in sock_set_timestamping() so that
the same rule can be applied to other drivers.

But may I ask why you tried to couple them so tight in the version?
Could you say more about this? It's optional, right? IIUC, you
expected the driver to have both timestamps and then calculate the
delta easily?

Thanks,
Jason


> +  timestamp is generated by the kernel when it receives a packet
> +  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. This flag can be enabled *only*
> +  via a socket option.
> +
>
>  1.3.2 Timestamp Reporting
>  ^^^^^^^^^^^^^^^^^^^^^^^^^
> 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/core/sock.c b/net/core/sock.c
> index a197f0a0b878..76a5d5cb1e56 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2933,6 +2933,8 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>                 tsflags = *(u32 *)CMSG_DATA(cmsg);
>                 if (tsflags & ~SOF_TIMESTAMPING_TX_RECORD_MASK)
>                         return -EINVAL;
> +               if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION)
> +                       return -EINVAL;
>
>                 sockc->tsflags &= ~SOF_TIMESTAMPING_TX_RECORD_MASK;
>                 sockc->tsflags |= tsflags;
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 5489d0c9d13f..ed4d6a9f4d7e 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -473,6 +473,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)] = "tx-completion",
>  };
>  static_assert(ARRAY_SIZE(sof_timestamping_names) == __SOF_TIMESTAMPING_CNT);
>
> --
> 2.48.1
>
>

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

[Willem de Bruijn]

Jason Xing wrote:
> On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> >
> > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> > when hardware reports a packet completed.
> >
> > Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> > exist in the HCI specification except for ISO packets, and the hardware
> > has a queue where packets may wait.  In this case the software SND
> > timestamp only reflects the kernel-side part of the total latency
> > (usually small) and queue length (usually 0 unless HW buffers
> > congested), whereas the completion report time is more informative of
> > the true latency.
> >
> > It may also be useful in other cases where HW TX timestamps cannot be
> > obtained and user wants to estimate an upper bound to when the TX
> > probably happened.
> >
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >
> > Notes:
> >     v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
> >         together with SND, to save a bit in skb_shared_info.tx_flags
> >
> >         As it then cannot be set per-skb, reject setting it via CMSG.
> >
> >  Documentation/networking/timestamping.rst | 9 +++++++++
> >  include/uapi/linux/errqueue.h             | 1 +
> >  include/uapi/linux/net_tstamp.h           | 6 ++++--
> >  net/core/sock.c                           | 2 ++
> >  net/ethtool/common.c                      | 1 +
> >  5 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > index 61ef9da10e28..5034dfe326c0 100644
> > --- a/Documentation/networking/timestamping.rst
> > +++ b/Documentation/networking/timestamping.rst
> > @@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
> >    cumulative acknowledgment. The mechanism ignores SACK and FACK.
> >    This flag can be enabled via both socket options and control messages.
> >
> > +SOF_TIMESTAMPING_TX_COMPLETION:
> > +  Request tx timestamps on packet tx completion, for the packets that
> > +  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion
> 
> Is it mandatory for other drivers that will try to use
> SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled
> both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be
> better if you add the limitation in sock_set_timestamping() so that
> the same rule can be applied to other drivers.
> 
> But may I ask why you tried to couple them so tight in the version?
> Could you say more about this? It's optional, right? IIUC, you
> expected the driver to have both timestamps and then calculate the
> delta easily?

This is a workaround around the limited number of bits available in
skb_shared_info.tx_flags.

Pauli could claim last available bit 7.. but then you would need to
find another bit for SKBTX_BPF ;)

FWIW I think we could probably free up 1 or 2 bits if we look closely,
e.g., of SKBTX_HW_TSTAMP_USE_CYCLES or SKBTX_WIFI_STATUS.



But given that two uses for those bits are in review at the same time,
I doubt that this is the last such feature that is added.

This workaround is clever. Only, we're leaking implementation
limitations into the API, making it API non-uniform and thus more
complex.

On the one hand, the feature should work just like all the existing
ones, and thus be configurable independently, and maintaining a
consistent API. But, this does require a tx_flags bit. And the
same for each subsequent such feature that gets proposed.

On the other, if we can anticipate more such additional requests,
then perhaps it does make sense to use only one bit in the skb to
encode whether the skb is sampled (in this case SKBTX_SW_TSTAMP),
and use per-socket sk_tsflags to encode which types of timestamps
to generate for such sampled packets.

This is more or less how sampling in the new BPF mode works too.

It is just not how SO_TIMESTAMPING works for existing bits.


There's something to be said for either approach IMHO.

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

[Jason Xing]

On Thu, Feb 20, 2025 at 10:35 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> > > when hardware reports a packet completed.
> > >
> > > Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> > > exist in the HCI specification except for ISO packets, and the hardware
> > > has a queue where packets may wait.  In this case the software SND
> > > timestamp only reflects the kernel-side part of the total latency
> > > (usually small) and queue length (usually 0 unless HW buffers
> > > congested), whereas the completion report time is more informative of
> > > the true latency.
> > >
> > > It may also be useful in other cases where HW TX timestamps cannot be
> > > obtained and user wants to estimate an upper bound to when the TX
> > > probably happened.
> > >
> > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > ---
> > >
> > > Notes:
> > >     v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
> > >         together with SND, to save a bit in skb_shared_info.tx_flags
> > >
> > >         As it then cannot be set per-skb, reject setting it via CMSG.
> > >
> > >  Documentation/networking/timestamping.rst | 9 +++++++++
> > >  include/uapi/linux/errqueue.h             | 1 +
> > >  include/uapi/linux/net_tstamp.h           | 6 ++++--
> > >  net/core/sock.c                           | 2 ++
> > >  net/ethtool/common.c                      | 1 +
> > >  5 files changed, 17 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > index 61ef9da10e28..5034dfe326c0 100644
> > > --- a/Documentation/networking/timestamping.rst
> > > +++ b/Documentation/networking/timestamping.rst
> > > @@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
> > >    cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > >    This flag can be enabled via both socket options and control messages.
> > >
> > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > +  Request tx timestamps on packet tx completion, for the packets that
> > > +  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion
> >
> > Is it mandatory for other drivers that will try to use
> > SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled
> > both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be
> > better if you add the limitation in sock_set_timestamping() so that
> > the same rule can be applied to other drivers.
> >
> > But may I ask why you tried to couple them so tight in the version?
> > Could you say more about this? It's optional, right? IIUC, you
> > expected the driver to have both timestamps and then calculate the
> > delta easily?
>
> This is a workaround around the limited number of bits available in
> skb_shared_info.tx_flags.

Oh, I'm surprised I missed the point even though I revisited the
previous discussion.

Pauli, please add the limitation when users setsockopt in
sock_set_timestamping() :)

>
> Pauli could claim last available bit 7.. but then you would need to
> find another bit for SKBTX_BPF ;)

Right :D

>
> FWIW I think we could probably free up 1 or 2 bits if we look closely,
> e.g., of SKBTX_HW_TSTAMP_USE_CYCLES or SKBTX_WIFI_STATUS.

Good. Will you submit a patch series to do that, or...?

>
> But given that two uses for those bits are in review at the same time,
> I doubt that this is the last such feature that is added.
>
> This workaround is clever. Only, we're leaking implementation
> limitations into the API, making it API non-uniform and thus more
> complex.

Probably not a big deal because previously OPT_ID_TCP is also bound with OPT_ID?

>
> On the one hand, the feature should work just like all the existing
> ones, and thus be configurable independently, and maintaining a
> consistent API. But, this does require a tx_flags bit. And the
> same for each subsequent such feature that gets proposed.
>
> On the other, if we can anticipate more such additional requests,
> then perhaps it does make sense to use only one bit in the skb to
> encode whether the skb is sampled (in this case SKBTX_SW_TSTAMP),
> and use per-socket sk_tsflags to encode which types of timestamps
> to generate for such sampled packets.

Very good idea, I think.

>
> This is more or less how sampling in the new BPF mode works too.
>
> It is just not how SO_TIMESTAMPING works for existing bits.

If needed, especially when a new bit is added, I think we can refactor
this part in the future? Or can we adjust it in advance? Speaking of
the design itself, it's really good :)

Thanks,
Jason

>
>
> There's something to be said for either approach IMHO.

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

[Willem de Bruijn]

Jason Xing wrote:
> On Thu, Feb 20, 2025 at 10:35 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > >
> > > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> > > > when hardware reports a packet completed.
> > > >
> > > > Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> > > > exist in the HCI specification except for ISO packets, and the hardware
> > > > has a queue where packets may wait.  In this case the software SND
> > > > timestamp only reflects the kernel-side part of the total latency
> > > > (usually small) and queue length (usually 0 unless HW buffers
> > > > congested), whereas the completion report time is more informative of
> > > > the true latency.
> > > >
> > > > It may also be useful in other cases where HW TX timestamps cannot be
> > > > obtained and user wants to estimate an upper bound to when the TX
> > > > probably happened.
> > > >
> > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > ---
> > > >
> > > > Notes:
> > > >     v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
> > > >         together with SND, to save a bit in skb_shared_info.tx_flags
> > > >
> > > >         As it then cannot be set per-skb, reject setting it via CMSG.
> > > >
> > > >  Documentation/networking/timestamping.rst | 9 +++++++++
> > > >  include/uapi/linux/errqueue.h             | 1 +
> > > >  include/uapi/linux/net_tstamp.h           | 6 ++++--
> > > >  net/core/sock.c                           | 2 ++
> > > >  net/ethtool/common.c                      | 1 +
> > > >  5 files changed, 17 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > > index 61ef9da10e28..5034dfe326c0 100644
> > > > --- a/Documentation/networking/timestamping.rst
> > > > +++ b/Documentation/networking/timestamping.rst
> > > > @@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
> > > >    cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > > >    This flag can be enabled via both socket options and control messages.
> > > >
> > > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > > +  Request tx timestamps on packet tx completion, for the packets that
> > > > +  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion
> > >
> > > Is it mandatory for other drivers that will try to use
> > > SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled
> > > both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be
> > > better if you add the limitation in sock_set_timestamping() so that
> > > the same rule can be applied to other drivers.
> > >
> > > But may I ask why you tried to couple them so tight in the version?
> > > Could you say more about this? It's optional, right? IIUC, you
> > > expected the driver to have both timestamps and then calculate the
> > > delta easily?
> >
> > This is a workaround around the limited number of bits available in
> > skb_shared_info.tx_flags.
> 
> Oh, I'm surprised I missed the point even though I revisited the
> previous discussion.
> 
> Pauli, please add the limitation when users setsockopt in
> sock_set_timestamping() :)
> 
> >
> > Pauli could claim last available bit 7.. but then you would need to
> > find another bit for SKBTX_BPF ;)
> 
> Right :D
> 
> >
> > FWIW I think we could probably free up 1 or 2 bits if we look closely,
> > e.g., of SKBTX_HW_TSTAMP_USE_CYCLES or SKBTX_WIFI_STATUS.
> 
> Good. Will you submit a patch series to do that, or...?

Reclaiming space is really up to whoever needs it.

I'll take a quick look, just to see if there is an obvious path and
we can postpone this whole conversation to next time we need a bit.

> 
> >
> > But given that two uses for those bits are in review at the same time,
> > I doubt that this is the last such feature that is added.
> >
> > This workaround is clever. Only, we're leaking implementation
> > limitations into the API, making it API non-uniform and thus more
> > complex.
> 
> Probably not a big deal because previously OPT_ID_TCP is also bound with OPT_ID?

That is also unfortunate. Ideally we had never needed OPT_ID_TCP.
 
> >
> > On the one hand, the feature should work just like all the existing
> > ones, and thus be configurable independently, and maintaining a
> > consistent API. But, this does require a tx_flags bit. And the
> > same for each subsequent such feature that gets proposed.
> >
> > On the other, if we can anticipate more such additional requests,
> > then perhaps it does make sense to use only one bit in the skb to
> > encode whether the skb is sampled (in this case SKBTX_SW_TSTAMP),
> > and use per-socket sk_tsflags to encode which types of timestamps
> > to generate for such sampled packets.
> 
> Very good idea, I think.

After giving it some thought, I prefer the first option to maintain
the same API. Let's see if we can harvest a bit and use that for this
new completion point.

> 
> >
> > This is more or less how sampling in the new BPF mode works too.
> >
> > It is just not how SO_TIMESTAMPING works for existing bits.
> 
> If needed, especially when a new bit is added, I think we can refactor
> this part in the future? Or can we adjust it in advance? Speaking of
> the design itself, it's really good :)

We cannot change existing behavior.

> Thanks,
> Jason
> 
> >
> >
> > There's something to be said for either approach IMHO.

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

[Willem de Bruijn]

Willem de Bruijn wrote:
> Jason Xing wrote:
> > On Thu, Feb 20, 2025 at 10:35 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > >
> > > > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> > > > > when hardware reports a packet completed.
> > > > >
> > > > > Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> > > > > exist in the HCI specification except for ISO packets, and the hardware
> > > > > has a queue where packets may wait.  In this case the software SND
> > > > > timestamp only reflects the kernel-side part of the total latency
> > > > > (usually small) and queue length (usually 0 unless HW buffers
> > > > > congested), whereas the completion report time is more informative of
> > > > > the true latency.
> > > > >
> > > > > It may also be useful in other cases where HW TX timestamps cannot be
> > > > > obtained and user wants to estimate an upper bound to when the TX
> > > > > probably happened.
> > > > >
> > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > >     v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
> > > > >         together with SND, to save a bit in skb_shared_info.tx_flags
> > > > >
> > > > >         As it then cannot be set per-skb, reject setting it via CMSG.
> > > > >
> > > > >  Documentation/networking/timestamping.rst | 9 +++++++++
> > > > >  include/uapi/linux/errqueue.h             | 1 +
> > > > >  include/uapi/linux/net_tstamp.h           | 6 ++++--
> > > > >  net/core/sock.c                           | 2 ++
> > > > >  net/ethtool/common.c                      | 1 +
> > > > >  5 files changed, 17 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > > > index 61ef9da10e28..5034dfe326c0 100644
> > > > > --- a/Documentation/networking/timestamping.rst
> > > > > +++ b/Documentation/networking/timestamping.rst
> > > > > @@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
> > > > >    cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > > > >    This flag can be enabled via both socket options and control messages.
> > > > >
> > > > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > > > +  Request tx timestamps on packet tx completion, for the packets that
> > > > > +  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion
> > > >
> > > > Is it mandatory for other drivers that will try to use
> > > > SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled
> > > > both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be
> > > > better if you add the limitation in sock_set_timestamping() so that
> > > > the same rule can be applied to other drivers.
> > > >
> > > > But may I ask why you tried to couple them so tight in the version?
> > > > Could you say more about this? It's optional, right? IIUC, you
> > > > expected the driver to have both timestamps and then calculate the
> > > > delta easily?
> > >
> > > This is a workaround around the limited number of bits available in
> > > skb_shared_info.tx_flags.
> > 
> > Oh, I'm surprised I missed the point even though I revisited the
> > previous discussion.
> > 
> > Pauli, please add the limitation when users setsockopt in
> > sock_set_timestamping() :)
> > 
> > >
> > > Pauli could claim last available bit 7.. but then you would need to
> > > find another bit for SKBTX_BPF ;)
> > 
> > Right :D
> > 
> > >
> > > FWIW I think we could probably free up 1 or 2 bits if we look closely,
> > > e.g., of SKBTX_HW_TSTAMP_USE_CYCLES or SKBTX_WIFI_STATUS.
> > 
> > Good. Will you submit a patch series to do that, or...?
> 
> Reclaiming space is really up to whoever needs it.
> 
> I'll take a quick look, just to see if there is an obvious path and
> we can postpone this whole conversation to next time we need a bit.

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

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

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

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

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

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

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

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

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

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

[Jason Xing]

On Fri, Feb 21, 2025 at 7:19 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Willem de Bruijn wrote:
> > Jason Xing wrote:
> > > On Thu, Feb 20, 2025 at 10:35 AM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > > >
> > > > > > Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> > > > > > when hardware reports a packet completed.
> > > > > >
> > > > > > Completion tstamp is useful for Bluetooth, as hardware timestamps do not
> > > > > > exist in the HCI specification except for ISO packets, and the hardware
> > > > > > has a queue where packets may wait.  In this case the software SND
> > > > > > timestamp only reflects the kernel-side part of the total latency
> > > > > > (usually small) and queue length (usually 0 unless HW buffers
> > > > > > congested), whereas the completion report time is more informative of
> > > > > > the true latency.
> > > > > >
> > > > > > It may also be useful in other cases where HW TX timestamps cannot be
> > > > > > obtained and user wants to estimate an upper bound to when the TX
> > > > > > probably happened.
> > > > > >
> > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > > ---
> > > > > >
> > > > > > Notes:
> > > > > >     v4: changed SOF_TIMESTAMPING_TX_COMPLETION to only emit COMPLETION
> > > > > >         together with SND, to save a bit in skb_shared_info.tx_flags
> > > > > >
> > > > > >         As it then cannot be set per-skb, reject setting it via CMSG.
> > > > > >
> > > > > >  Documentation/networking/timestamping.rst | 9 +++++++++
> > > > > >  include/uapi/linux/errqueue.h             | 1 +
> > > > > >  include/uapi/linux/net_tstamp.h           | 6 ++++--
> > > > > >  net/core/sock.c                           | 2 ++
> > > > > >  net/ethtool/common.c                      | 1 +
> > > > > >  5 files changed, 17 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > > > > index 61ef9da10e28..5034dfe326c0 100644
> > > > > > --- a/Documentation/networking/timestamping.rst
> > > > > > +++ b/Documentation/networking/timestamping.rst
> > > > > > @@ -140,6 +140,15 @@ SOF_TIMESTAMPING_TX_ACK:
> > > > > >    cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > > > > >    This flag can be enabled via both socket options and control messages.
> > > > > >
> > > > > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > > > > +  Request tx timestamps on packet tx completion, for the packets that
> > > > > > +  also have SOF_TIMESTAMPING_TX_SOFTWARE enabled.  The completion
> > > > >
> > > > > Is it mandatory for other drivers that will try to use
> > > > > SOF_TIMESTAMPING_TX_COMPLETION in the future? I can see you coupled
> > > > > both of them in hci_conn_tx_queue in patch [2/5]. If so, it would be
> > > > > better if you add the limitation in sock_set_timestamping() so that
> > > > > the same rule can be applied to other drivers.
> > > > >
> > > > > But may I ask why you tried to couple them so tight in the version?
> > > > > Could you say more about this? It's optional, right? IIUC, you
> > > > > expected the driver to have both timestamps and then calculate the
> > > > > delta easily?
> > > >
> > > > This is a workaround around the limited number of bits available in
> > > > skb_shared_info.tx_flags.
> > >
> > > Oh, I'm surprised I missed the point even though I revisited the
> > > previous discussion.
> > >
> > > Pauli, please add the limitation when users setsockopt in
> > > sock_set_timestamping() :)
> > >
> > > >
> > > > Pauli could claim last available bit 7.. but then you would need to
> > > > find another bit for SKBTX_BPF ;)
> > >
> > > Right :D
> > >
> > > >
> > > > FWIW I think we could probably free up 1 or 2 bits if we look closely,
> > > > e.g., of SKBTX_HW_TSTAMP_USE_CYCLES or SKBTX_WIFI_STATUS.
> > >
> > > Good. Will you submit a patch series to do that, or...?
> >
> > Reclaiming space is really up to whoever needs it.
> >
> > I'll take a quick look, just to see if there is an obvious path and
> > we can postpone this whole conversation to next time we need a bit.
>
> SKBTX_HW_TSTAMP_USE_CYCLES is only true if SOF_TIMESTAMPING_BIND_PHC.
> It cannot be set per cmsg (is not in SOF_TIMESTAMPING_TX_RECORD_MASK),
> so no need to record it per skb.

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

>
> It only has two drivers using it, which can easily be updated:
>
>         -                if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES)
>         +                if (skb->sk &&
>         +                    READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
>                                         tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;
>
> They later call skb_tstamp_tx, which does nothing if !skb->sk.
> Only cost is a higher cost of accessing the sk cacheline.
>
> SKBTX_WIFI_STATUS essentially follows the same argument. It can only
> be set in the sockopt. It has a handful more callsites that would need
> to be updated. sock_flag(sk, SOCK_WIFI_STATUS) will be tested without
> the socket lock held. But this is already the case in the UDP lockless
> fast path through ip_make_skb.
>
> SKBTX_HW_TSTAMP_NETDEV is only used on Rx. Could shadow another bit
> that is used only on Tx.
>
> SKBTX_IN_PROGRESS is only used by the driver to suppress the software
> tx timestamp from skb_tx_timestamp if a later hardware timestamp will
> be generated. Predates SOF_TIMESTAMPING_OPT_TX_SWHW.

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

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

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

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

> The first one seems most straightforward.

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

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

Thanks,
Jason

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

[Pauli Virtanen]

Hi,

to, 2025-02-20 kello 18:19 -0500, Willem de Bruijn kirjoitti:
> Willem de Bruijn wrote:
[clip]
> > 
> > Reclaiming space is really up to whoever needs it.
> > 
> > I'll take a quick look, just to see if there is an obvious path and
> > we can postpone this whole conversation to next time we need a bit.
> 
> SKBTX_HW_TSTAMP_USE_CYCLES is only true if SOF_TIMESTAMPING_BIND_PHC.
> It cannot be set per cmsg (is not in SOF_TIMESTAMPING_TX_RECORD_MASK),
> so no need to record it per skb.
> 
> It only has two drivers using it, which can easily be updated:
> 
> 	-                if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES)
> 	+                if (skb->sk &&
> 	+                    READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
> 					tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;
> 
> They later call skb_tstamp_tx, which does nothing if !skb->sk.
> Only cost is a higher cost of accessing the sk cacheline.
> 
> SKBTX_WIFI_STATUS essentially follows the same argument. It can only
> be set in the sockopt. It has a handful more callsites that would need
> to be updated. sock_flag(sk, SOCK_WIFI_STATUS) will be tested without
> the socket lock held. But this is already the case in the UDP lockless
> fast path through ip_make_skb.
> 
> SKBTX_HW_TSTAMP_NETDEV is only used on Rx. Could shadow another bit
> that is used only on Tx.
> 
> SKBTX_IN_PROGRESS is only used by the driver to suppress the software
> tx timestamp from skb_tx_timestamp if a later hardware timestamp will
> be generated. Predates SOF_TIMESTAMPING_OPT_TX_SWHW.
> 
> In short plenty of bits we can reclaim if we try.
> 
> SKBTX_BPF was just merged, so we will have to reclaim one. The first
> one seems most straightforward.

Ok, I'll go back to tx_flags bit for v5 (= v3 + the minor cleanups),
going on top of "net: skb: free up one bit in tx_flags" then.

-- 
Pauli Virtanen

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

[Luiz Augusto von Dentz]

Hi Pauli,

On Wed, Feb 19, 2025 at 7:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> >
> > 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)
> >
> > Previous discussions:
> > https://lore.kernel.org/linux-bluetooth/cover.1739097311.git.pav@iki.fi/
> > https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/
> > https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/
> >
> > Changes
> > =======
> > v4:
> > - Change meaning of SOF_TIMESTAMPING_TX_COMPLETION, to save a bit in
> >   skb_shared_info.tx_flags:
> >
> >   It now enables COMPLETION only for packets that also have software SND
> >   enabled.  The flag can now be enabled only via a socket option, but
> >   coupling with SND allows user to still choose for which packets
> >   SND+COMPLETION should be generated.  This choice maybe is OK for
> >   applications which can skip SND if they're not interested.
> >
> >   However, this would make the timestamping API not uniform, as the
> >   other TX flags can be enabled via CMSG.
> >
> >   IIUC, sizeof skb_shared_info cannot be easily changed and I'm not sure
> >   there is somewhere else in general skb info, where one could safely
> >   put the extra separate flag bit for COMPLETION. So here's alternative
> >   suggestion.
> >
> > - Better name in sof_timestamping_names
> >
> > - I decided to keep using sockcm_init(), to avoid open coding READ_ONCE
> >   and since it's passed to sock_cmsg_send() which anyway also may init
> >   such fields.
>
> Please don't do this since the current sockcm_init() initializes more
> than you need. Please take a look at what the new sockcm_init looks
> like and what this series has done[1] which just got merged in recent
> days :)
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=aefd232de5eb2e7
>
> Thanks,
> Jason
>
> >
> > 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.
> >
> > 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.
> >
> > Tests
> > =====
> >
> > See
> > https://lore.kernel.org/linux-bluetooth/cover.1739026302.git.pav@iki.fi/
> >
> > 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/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                  | 118 ++++++++++++++++++++++
> >  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/core/sock.c                           |   2 +
> >  net/ethtool/common.c                      |   1 +
> >  17 files changed, 258 insertions(+), 20 deletions(-)
> >
> > --
> > 2.48.1
> >
> >

We are sort of running out of time, if we want to be able to merge by
the next merge window then it must be done this week.

-- 
Luiz Augusto von Dentz

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

[Pauli Virtanen]

ma, 2025-03-17 kello 13:50 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Wed, Feb 19, 2025 at 7:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > 
> > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > 
> > > 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)
> > > 
> > > Previous discussions:
> > > https://lore.kernel.org/linux-bluetooth/cover.1739097311.git.pav@iki.fi/
> > > https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/
> > > https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/
[clip]
>
> We are sort of running out of time, if we want to be able to merge by
> the next merge window then it must be done this week.

It took a bit of time to get back to it, but v5 is sent now.

Note that it does not apply cleanly on bluetooth-next/master, as the
first commit is based on net-next since there have been some changes
there that need to be taken into account here:

commit e6116fc605574bb58c2016938ff24a7fbafe6e2a
Author: Willem de Bruijn <willemb@google.com>
Date:   Tue Feb 25 04:33:55 2025

    net: skb: free up one bit in tx_flags

commit aa290f93a4af662b8d2d9e9df65798f9f24cecf3
Author: Jason Xing <kerneljasonxing@gmail.com>
Date:   Thu Feb 20 09:29:33 2025

    net-timestamp: Prepare for isolating two modes of SO_TIMESTAMPING

-- 
Pauli Virtanen

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

[Luiz Augusto von Dentz]

Hi Pauli,

On Tue, Mar 18, 2025 at 3:19 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> ma, 2025-03-17 kello 13:50 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Wed, Feb 19, 2025 at 7:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > On Thu, Feb 20, 2025 at 2:15 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > >
> > > > 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)
> > > >
> > > > Previous discussions:
> > > > https://lore.kernel.org/linux-bluetooth/cover.1739097311.git.pav@iki.fi/
> > > > https://lore.kernel.org/all/6642c7f3427b5_20539c2949a@willemb.c.googlers.com.notmuch/
> > > > https://lore.kernel.org/all/cover.1710440392.git.pav@iki.fi/
> [clip]
> >
> > We are sort of running out of time, if we want to be able to merge by
> > the next merge window then it must be done this week.
>
> It took a bit of time to get back to it, but v5 is sent now.
>
> Note that it does not apply cleanly on bluetooth-next/master, as the
> first commit is based on net-next since there have been some changes
> there that need to be taken into account here:
>
> commit e6116fc605574bb58c2016938ff24a7fbafe6e2a
> Author: Willem de Bruijn <willemb@google.com>
> Date:   Tue Feb 25 04:33:55 2025
>
>     net: skb: free up one bit in tx_flags
>
> commit aa290f93a4af662b8d2d9e9df65798f9f24cecf3
> Author: Jason Xing <kerneljasonxing@gmail.com>
> Date:   Thu Feb 20 09:29:33 2025
>
>     net-timestamp: Prepare for isolating two modes of SO_TIMESTAMPING

Got it, I will rebase to net-next to have these also available in
bluetooth-next.

-- 
Luiz Augusto von Dentz