* [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO
@ 2025-03-18 19:06 Pauli Virtanen
2025-03-18 19:06 ` [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion Pauli Virtanen
` (5 more replies)
0 siblings, 6 replies; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-18 19:06 UTC (permalink / raw)
To: linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
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.1739988644.git.pav@iki.fi/
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
=======
v5:
- Revert to v3 vs decoupled SND & COMPLETION, just use the bit in tx_flags.
- Add hci_sockcm_init() and use it.
- Emit COMPLETION also for SCO if flowctl enabled, now that we enable it
for some HW (most HW doesn't seem to support it)
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
Documentation/networking/timestamping.rst | 8 ++
include/linux/skbuff.h | 7 +-
include/net/bluetooth/bluetooth.h | 1 +
include/net/bluetooth/hci_core.h | 20 ++++
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 | 122 ++++++++++++++++++++++
net/bluetooth/hci_core.c | 15 ++-
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/skbuff.c | 2 +
net/ethtool/common.c | 1 +
net/socket.c | 3 +
19 files changed, 274 insertions(+), 22 deletions(-)
--
2.48.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-18 19:06 [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO Pauli Virtanen
@ 2025-03-18 19:06 ` Pauli Virtanen
2025-03-19 0:13 ` Jason Xing
` (2 more replies)
2025-03-18 19:06 ` [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping Pauli Virtanen
` (4 subsequent siblings)
5 siblings, 3 replies; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-18 19:06 UTC (permalink / raw)
To: linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
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:
v5:
- back to decoupled COMPLETION & SND, like in v3
- BPF reporting not implemented here
Documentation/networking/timestamping.rst | 8 ++++++++
include/linux/skbuff.h | 7 ++++---
include/uapi/linux/errqueue.h | 1 +
include/uapi/linux/net_tstamp.h | 6 ++++--
net/core/skbuff.c | 2 ++
net/ethtool/common.c | 1 +
net/socket.c | 3 +++
7 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
index 61ef9da10e28..b8fef8101176 100644
--- a/Documentation/networking/timestamping.rst
+++ b/Documentation/networking/timestamping.rst
@@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
cumulative acknowledgment. The mechanism ignores SACK and FACK.
This flag can be enabled via both socket options and control messages.
+SOF_TIMESTAMPING_TX_COMPLETION:
+ Request tx timestamps on packet tx completion. The completion
+ timestamp is generated by the kernel when it receives packet a
+ completion report from the hardware. Hardware may report multiple
+ packets at once, and completion timestamps reflect the timing of the
+ report and not actual tx time. This flag can be enabled via both
+ socket options and control messages.
+
1.3.2 Timestamp Reporting
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index cd8294cdc249..b974a277975a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -478,8 +478,8 @@ enum {
/* device driver is going to provide hardware time stamp */
SKBTX_IN_PROGRESS = 1 << 2,
- /* reserved */
- SKBTX_RESERVED = 1 << 3,
+ /* generate software time stamp on packet tx completion */
+ SKBTX_COMPLETION_TSTAMP = 1 << 3,
/* generate wifi status information (where possible) */
SKBTX_WIFI_STATUS = 1 << 4,
@@ -498,7 +498,8 @@ enum {
#define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \
SKBTX_SCHED_TSTAMP | \
- SKBTX_BPF)
+ SKBTX_BPF | \
+ SKBTX_COMPLETION_TSTAMP)
#define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \
SKBTX_ANY_SW_TSTAMP)
diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
index 3c70e8ac14b8..1ea47309d772 100644
--- a/include/uapi/linux/errqueue.h
+++ b/include/uapi/linux/errqueue.h
@@ -73,6 +73,7 @@ enum {
SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */
SCM_TSTAMP_SCHED, /* data entered the packet scheduler */
SCM_TSTAMP_ACK, /* data acknowledged by peer */
+ SCM_TSTAMP_COMPLETION, /* packet tx completion */
};
#endif /* _UAPI_LINUX_ERRQUEUE_H */
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index 55b0ab51096c..383213de612a 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -44,8 +44,9 @@ enum {
SOF_TIMESTAMPING_BIND_PHC = (1 << 15),
SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16),
SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17),
+ SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18),
- SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER,
+ SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION,
SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
SOF_TIMESTAMPING_LAST
};
@@ -58,7 +59,8 @@ enum {
#define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \
SOF_TIMESTAMPING_TX_SOFTWARE | \
SOF_TIMESTAMPING_TX_SCHED | \
- SOF_TIMESTAMPING_TX_ACK)
+ SOF_TIMESTAMPING_TX_ACK | \
+ SOF_TIMESTAMPING_TX_COMPLETION)
/**
* struct so_timestamping - SO_TIMESTAMPING parameter
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ab8acb737b93..6cbf77bc61fc 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5523,6 +5523,8 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
SKBTX_SW_TSTAMP);
case SCM_TSTAMP_ACK:
return TCP_SKB_CB(skb)->txstamp_ack & TSTAMP_ACK_SK;
+ case SCM_TSTAMP_COMPLETION:
+ return skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP;
}
return false;
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 7e3c16856c1a..0cb6da1f692a 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -476,6 +476,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);
diff --git a/net/socket.c b/net/socket.c
index b64ecf2722e7..e3d879b53278 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -689,6 +689,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
if (tsflags & SOF_TIMESTAMPING_TX_SCHED)
flags |= SKBTX_SCHED_TSTAMP;
+ if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION)
+ flags |= SKBTX_COMPLETION_TSTAMP;
+
*tx_flags = flags;
}
EXPORT_SYMBOL(__sock_tx_timestamp);
--
2.48.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-18 19:06 [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO Pauli Virtanen
2025-03-18 19:06 ` [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion Pauli Virtanen
@ 2025-03-18 19:06 ` Pauli Virtanen
2025-03-19 0:39 ` Jason Xing
2025-03-18 19:06 ` [PATCH v5 3/5] Bluetooth: ISO: add TX timestamping Pauli Virtanen
` (3 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-18 19:06 UTC (permalink / raw)
To: linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
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:
v5:
- Add hci_sockm_init()
- Back to decoupled COMPLETION & SND, like in v3
- Handle SCO flow controlled case
include/net/bluetooth/hci_core.h | 20 +++++
net/bluetooth/hci_conn.c | 122 +++++++++++++++++++++++++++++++
net/bluetooth/hci_core.c | 15 +++-
net/bluetooth/hci_event.c | 4 +
4 files changed, 157 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f78e4298e39a..5115da34f881 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;
@@ -1572,6 +1580,18 @@ 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);
+
+static inline void hci_sockcm_init(struct sockcm_cookie *sockc, struct sock *sk)
+{
+ *sockc = (struct sockcm_cookie) {
+ .tsflags = READ_ONCE(sk->sk_tsflags),
+ };
+}
+
/*
* 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..95972fd4c784 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,122 @@ 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 without flowctl (needs to be done in drivers)
+ */
+ switch (conn->type) {
+ case ISO_LINK:
+ case ACL_LINK:
+ case LE_LINK:
+ break;
+ case SCO_LINK:
+ case ESCO_LINK:
+ if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
+ return;
+ break;
+ default:
+ return;
+ }
+
+ if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
+ track = true;
+
+ /* If nothing is tracked, just count extra skbs at the queue head */
+ if (!track && !comp->tracked) {
+ comp->extra++;
+ return;
+ }
+
+ if (track) {
+ skb = skb_clone_sk(skb);
+ if (!skb)
+ goto count_only;
+
+ comp->tracked++;
+ } else {
+ skb = skb_clone(skb, GFP_KERNEL);
+ if (!skb)
+ goto count_only;
+ }
+
+ skb_queue_tail(&comp->queue, skb);
+ return;
+
+count_only:
+ /* Stop tracking skbs, and only count. This will not emit timestamps for
+ * the packets, but if we get here something is more seriously wrong.
+ */
+ comp->tracked = 0;
+ comp->extra += skb_queue_len(&comp->queue) + 1;
+ skb_queue_purge(&comp->queue);
+}
+
+void hci_conn_tx_dequeue(struct hci_conn *conn)
+{
+ struct tx_queue *comp = &conn->tx_q;
+ struct sk_buff *skb;
+
+ /* If there are tracked skbs, the counted extra go before dequeuing real
+ * skbs, to keep ordering. When nothing is tracked, the ordering doesn't
+ * matter so dequeue real skbs first to get rid of them ASAP.
+ */
+ if (comp->extra && (comp->tracked || skb_queue_empty(&comp->queue))) {
+ comp->extra--;
+ return;
+ }
+
+ skb = skb_dequeue(&comp->queue);
+ if (!skb)
+ return;
+
+ if (skb->sk) {
+ comp->tracked--;
+ __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
+ SCM_TSTAMP_COMPLETION);
+ }
+
+ kfree_skb(skb);
+}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 94d9147612da..5eb0600bbd03 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3029,6 +3029,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)
@@ -3575,7 +3582,7 @@ static void hci_sched_sco(struct hci_dev *hdev, __u8 type)
while (*cnt && (conn = hci_low_sent(hdev, type, "e))) {
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)
@@ -3618,7 +3625,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--;
@@ -3674,7 +3681,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)--;
@@ -3708,7 +3715,7 @@ static void hci_sched_iso(struct hci_dev *hdev)
while (*cnt && (conn = hci_low_sent(hdev, ISO_LINK, "e))) {
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 0df4a0e082c8..83990c975c1f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4415,6 +4415,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);
@@ -4425,6 +4426,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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 3/5] Bluetooth: ISO: add TX timestamping
2025-03-18 19:06 [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO Pauli Virtanen
2025-03-18 19:06 ` [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion Pauli Virtanen
2025-03-18 19:06 ` [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping Pauli Virtanen
@ 2025-03-18 19:06 ` Pauli Virtanen
2025-03-19 14:49 ` Willem de Bruijn
2025-03-18 19:06 ` [PATCH v5 4/5] Bluetooth: L2CAP: " Pauli Virtanen
` (2 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-18 19:06 UTC (permalink / raw)
To: linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
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:
v5:
- use sockcm_init -> hci_sockcm_init
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 0cb52a3308ba..3501a991f1c6 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;
+ hci_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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 4/5] Bluetooth: L2CAP: add TX timestamping
2025-03-18 19:06 [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO Pauli Virtanen
` (2 preceding siblings ...)
2025-03-18 19:06 ` [PATCH v5 3/5] Bluetooth: ISO: add TX timestamping Pauli Virtanen
@ 2025-03-18 19:06 ` Pauli Virtanen
2025-03-18 19:06 ` [PATCH v5 5/5] Bluetooth: SCO: " Pauli Virtanen
2025-03-20 16:10 ` [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO patchwork-bot+bluetooth
5 siblings, 0 replies; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-18 19:06 UTC (permalink / raw)
To: linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
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:
v5:
- use sockcm_init -> hci_sockcm_init
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 0bf8cb17a6e8..4bb0eaedda18 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 73530b8e1eae..f0c862091bff 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -444,7 +444,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 7b4adab353cf..c7b66b2ea9f2 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2515,7 +2515,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;
@@ -2530,6 +2556,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;
}
@@ -2553,6 +2581,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);
@@ -2574,6 +2604,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;
@@ -2597,10 +2629,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..5aa55fa69594 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;
+ hci_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 a31c6acf1df2..47f359f24d1f 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 5/5] Bluetooth: SCO: add TX timestamping
2025-03-18 19:06 [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO Pauli Virtanen
` (3 preceding siblings ...)
2025-03-18 19:06 ` [PATCH v5 4/5] Bluetooth: L2CAP: " Pauli Virtanen
@ 2025-03-18 19:06 ` Pauli Virtanen
2025-03-20 16:10 ` [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO patchwork-bot+bluetooth
5 siblings, 0 replies; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-18 19:06 UTC (permalink / raw)
To: linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
Support TX timestamping in SCO sockets.
Not available for hdevs without SCO_FLOWCTL.
Support MSG_ERRQUEUE in SCO recvmsg.
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
Notes:
v5:
- use sockcm_init -> hci_sockcm_init
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 5d1bc0d6aee0..2945d27e75dc 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -378,7 +378,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;
@@ -389,6 +390,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;
@@ -784,6 +786,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);
@@ -795,6 +798,14 @@ static int sco_sock_sendmsg(struct socket *sock, struct msghdr *msg,
if (msg->msg_flags & MSG_OOB)
return -EOPNOTSUPP;
+ hci_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);
@@ -802,7 +813,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;
@@ -868,6 +879,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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-18 19:06 ` [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion Pauli Virtanen
@ 2025-03-19 0:13 ` Jason Xing
2025-03-19 14:37 ` Willem de Bruijn
2025-03-19 15:48 ` Paul Menzel
2 siblings, 0 replies; 24+ messages in thread
From: Jason Xing @ 2025-03-19 0:13 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
On Wed, Mar 19, 2025 at 3:08 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>
Hi Pauli,
This patch overall looks good to me but it depends on the small
question in another patch that is relevant to how we use this new flag
in reality. Let's discuss a bit there.
Thanks,
Jason
> ---
>
> Notes:
> v5:
> - back to decoupled COMPLETION & SND, like in v3
> - BPF reporting not implemented here
>
> Documentation/networking/timestamping.rst | 8 ++++++++
> include/linux/skbuff.h | 7 ++++---
> include/uapi/linux/errqueue.h | 1 +
> include/uapi/linux/net_tstamp.h | 6 ++++--
> net/core/skbuff.c | 2 ++
> net/ethtool/common.c | 1 +
> net/socket.c | 3 +++
> 7 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 61ef9da10e28..b8fef8101176 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> cumulative acknowledgment. The mechanism ignores SACK and FACK.
> This flag can be enabled via both socket options and control messages.
>
> +SOF_TIMESTAMPING_TX_COMPLETION:
> + Request tx timestamps on packet tx completion. The completion
> + timestamp is generated by the kernel when it receives packet a
> + completion report from the hardware. Hardware may report multiple
> + packets at once, and completion timestamps reflect the timing of the
> + report and not actual tx time. This flag can be enabled via both
> + socket options and control messages.
> +
>
> 1.3.2 Timestamp Reporting
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index cd8294cdc249..b974a277975a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -478,8 +478,8 @@ enum {
> /* device driver is going to provide hardware time stamp */
> SKBTX_IN_PROGRESS = 1 << 2,
>
> - /* reserved */
> - SKBTX_RESERVED = 1 << 3,
> + /* generate software time stamp on packet tx completion */
> + SKBTX_COMPLETION_TSTAMP = 1 << 3,
>
> /* generate wifi status information (where possible) */
> SKBTX_WIFI_STATUS = 1 << 4,
> @@ -498,7 +498,8 @@ enum {
>
> #define SKBTX_ANY_SW_TSTAMP (SKBTX_SW_TSTAMP | \
> SKBTX_SCHED_TSTAMP | \
> - SKBTX_BPF)
> + SKBTX_BPF | \
> + SKBTX_COMPLETION_TSTAMP)
> #define SKBTX_ANY_TSTAMP (SKBTX_HW_TSTAMP | \
> SKBTX_ANY_SW_TSTAMP)
>
> diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
> index 3c70e8ac14b8..1ea47309d772 100644
> --- a/include/uapi/linux/errqueue.h
> +++ b/include/uapi/linux/errqueue.h
> @@ -73,6 +73,7 @@ enum {
> SCM_TSTAMP_SND, /* driver passed skb to NIC, or HW */
> SCM_TSTAMP_SCHED, /* data entered the packet scheduler */
> SCM_TSTAMP_ACK, /* data acknowledged by peer */
> + SCM_TSTAMP_COMPLETION, /* packet tx completion */
> };
>
> #endif /* _UAPI_LINUX_ERRQUEUE_H */
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index 55b0ab51096c..383213de612a 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -44,8 +44,9 @@ enum {
> SOF_TIMESTAMPING_BIND_PHC = (1 << 15),
> SOF_TIMESTAMPING_OPT_ID_TCP = (1 << 16),
> SOF_TIMESTAMPING_OPT_RX_FILTER = (1 << 17),
> + SOF_TIMESTAMPING_TX_COMPLETION = (1 << 18),
>
> - SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_RX_FILTER,
> + SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_TX_COMPLETION,
> SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
> SOF_TIMESTAMPING_LAST
> };
> @@ -58,7 +59,8 @@ enum {
> #define SOF_TIMESTAMPING_TX_RECORD_MASK (SOF_TIMESTAMPING_TX_HARDWARE | \
> SOF_TIMESTAMPING_TX_SOFTWARE | \
> SOF_TIMESTAMPING_TX_SCHED | \
> - SOF_TIMESTAMPING_TX_ACK)
> + SOF_TIMESTAMPING_TX_ACK | \
> + SOF_TIMESTAMPING_TX_COMPLETION)
>
> /**
> * struct so_timestamping - SO_TIMESTAMPING parameter
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ab8acb737b93..6cbf77bc61fc 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5523,6 +5523,8 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
> SKBTX_SW_TSTAMP);
> case SCM_TSTAMP_ACK:
> return TCP_SKB_CB(skb)->txstamp_ack & TSTAMP_ACK_SK;
> + case SCM_TSTAMP_COMPLETION:
> + return skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP;
> }
>
> return false;
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 7e3c16856c1a..0cb6da1f692a 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -476,6 +476,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);
>
> diff --git a/net/socket.c b/net/socket.c
> index b64ecf2722e7..e3d879b53278 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -689,6 +689,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
> if (tsflags & SOF_TIMESTAMPING_TX_SCHED)
> flags |= SKBTX_SCHED_TSTAMP;
>
> + if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION)
> + flags |= SKBTX_COMPLETION_TSTAMP;
> +
> *tx_flags = flags;
> }
> EXPORT_SYMBOL(__sock_tx_timestamp);
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-18 19:06 ` [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping Pauli Virtanen
@ 2025-03-19 0:39 ` Jason Xing
2025-03-19 14:44 ` Willem de Bruijn
2025-03-19 18:21 ` Pauli Virtanen
0 siblings, 2 replies; 24+ messages in thread
From: Jason Xing @ 2025-03-19 0:39 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Support enabling TX timestamping for some skbs, and track them until
> packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> completion report from hardware.
>
> Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> driver requires changes in the driver API, and drivers mostly are going
> to send the skb immediately.
>
> Make the default situation with no COMPLETION TX timestamping more
> efficient by only counting packets in the queue when there is nothing to
> track. When there is something to track, we need to make clones, since
> the driver may modify sent skbs.
>
> The tx_q queue length is bounded by the hdev flow control, which will
> not send new packets before it has got completion reports for old ones.
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
> v5:
> - Add hci_sockm_init()
> - Back to decoupled COMPLETION & SND, like in v3
> - Handle SCO flow controlled case
>
> include/net/bluetooth/hci_core.h | 20 +++++
> net/bluetooth/hci_conn.c | 122 +++++++++++++++++++++++++++++++
> net/bluetooth/hci_core.c | 15 +++-
> net/bluetooth/hci_event.c | 4 +
> 4 files changed, 157 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f78e4298e39a..5115da34f881 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;
> @@ -1572,6 +1580,18 @@ 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);
> +
> +static inline void hci_sockcm_init(struct sockcm_cookie *sockc, struct sock *sk)
> +{
> + *sockc = (struct sockcm_cookie) {
> + .tsflags = READ_ONCE(sk->sk_tsflags),
> + };
> +}
> +
> /*
> * 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..95972fd4c784 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,122 @@ 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);
It's a bit strange that SCM_TSTAMP_SND is under the control of
SKBTX_SW_TSTAMP. Can we use the same flag for both lines here
directly? I suppose I would use SKBTX_SW_TSTAMP then.
> +
> + /* COMPLETION tstamp is emitted for tracked skb later in Number of
> + * Completed Packets event. Available only for flow controlled cases.
> + *
> + * TODO: SCO support without flowctl (needs to be done in drivers)
> + */
> + switch (conn->type) {
> + case ISO_LINK:
> + case ACL_LINK:
> + case LE_LINK:
> + break;
> + case SCO_LINK:
> + case ESCO_LINK:
> + if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
> + return;
> + break;
> + default:
> + return;
> + }
> +
> + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> + track = true;
> +
> + /* If nothing is tracked, just count extra skbs at the queue head */
> + if (!track && !comp->tracked) {
> + comp->extra++;
> + return;
> + }
> +
> + if (track) {
> + skb = skb_clone_sk(skb);
> + if (!skb)
> + goto count_only;
> +
> + comp->tracked++;
> + } else {
> + skb = skb_clone(skb, GFP_KERNEL);
> + if (!skb)
> + goto count_only;
> + }
> +
> + skb_queue_tail(&comp->queue, skb);
> + return;
> +
> +count_only:
> + /* Stop tracking skbs, and only count. This will not emit timestamps for
> + * the packets, but if we get here something is more seriously wrong.
> + */
> + comp->tracked = 0;
> + comp->extra += skb_queue_len(&comp->queue) + 1;
> + skb_queue_purge(&comp->queue);
> +}
> +
> +void hci_conn_tx_dequeue(struct hci_conn *conn)
> +{
> + struct tx_queue *comp = &conn->tx_q;
> + struct sk_buff *skb;
> +
> + /* If there are tracked skbs, the counted extra go before dequeuing real
> + * skbs, to keep ordering. When nothing is tracked, the ordering doesn't
> + * matter so dequeue real skbs first to get rid of them ASAP.
> + */
> + if (comp->extra && (comp->tracked || skb_queue_empty(&comp->queue))) {
> + comp->extra--;
> + return;
> + }
> +
> + skb = skb_dequeue(&comp->queue);
> + if (!skb)
> + return;
> +
> + if (skb->sk) {
> + comp->tracked--;
Need an explicit if statement here?
> + __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> + SCM_TSTAMP_COMPLETION);
This is the socket timestamping, and that's right. My minor question
is: for the use of bpf timestamping (that should be easy as you've
done in patch 1, I presume), I'm not sure if you're familiar with it.
If not, I plan to implement it myself in a follow-up patch and then
let you do some tests? Of course, I will provide the bpf test script
:)
Thanks,
Jason
> + }
> +
> + kfree_skb(skb);
> +}
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 94d9147612da..5eb0600bbd03 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3029,6 +3029,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)
> @@ -3575,7 +3582,7 @@ static void hci_sched_sco(struct hci_dev *hdev, __u8 type)
> while (*cnt && (conn = hci_low_sent(hdev, type, "e))) {
> 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)
> @@ -3618,7 +3625,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--;
> @@ -3674,7 +3681,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)--;
> @@ -3708,7 +3715,7 @@ static void hci_sched_iso(struct hci_dev *hdev)
> while (*cnt && (conn = hci_low_sent(hdev, ISO_LINK, "e))) {
> 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 0df4a0e082c8..83990c975c1f 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4415,6 +4415,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);
> @@ -4425,6 +4426,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
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-18 19:06 ` [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion Pauli Virtanen
2025-03-19 0:13 ` Jason Xing
@ 2025-03-19 14:37 ` Willem de Bruijn
2025-03-19 15:48 ` Paul Menzel
2 siblings, 0 replies; 24+ messages in thread
From: Willem de Bruijn @ 2025-03-19 14:37 UTC (permalink / raw)
To: Pauli Virtanen, linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
Pauli Virtanen wrote:
> Add SOF_TIMESTAMPING_TX_COMPLETION, for requesting a software timestamp
> when hardware reports a packet completed.
>
> Completion tstamp is useful for Bluetooth, 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:
> v5:
> - back to decoupled COMPLETION & SND, like in v3
> - BPF reporting not implemented here
>
> Documentation/networking/timestamping.rst | 8 ++++++++
> include/linux/skbuff.h | 7 ++++---
> include/uapi/linux/errqueue.h | 1 +
> include/uapi/linux/net_tstamp.h | 6 ++++--
> net/core/skbuff.c | 2 ++
> net/ethtool/common.c | 1 +
> net/socket.c | 3 +++
> 7 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 61ef9da10e28..b8fef8101176 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> cumulative acknowledgment. The mechanism ignores SACK and FACK.
> This flag can be enabled via both socket options and control messages.
>
> +SOF_TIMESTAMPING_TX_COMPLETION:
> + Request tx timestamps on packet tx completion. The completion
Minor: double space above, grammar issue below "receives [packet] a".
> + timestamp is generated by the kernel when it receives packet a
> + completion report from the hardware. Hardware may report multiple
> + packets at once, and completion timestamps reflect the timing of the
> + report and not actual tx time. This flag can be enabled via both
> + socket options and control messages.
> +
Otherwise the patch LGTM.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 0:39 ` Jason Xing
@ 2025-03-19 14:44 ` Willem de Bruijn
2025-03-19 17:43 ` Pauli Virtanen
2025-03-19 18:21 ` Pauli Virtanen
1 sibling, 1 reply; 24+ messages in thread
From: Willem de Bruijn @ 2025-03-19 14:44 UTC (permalink / raw)
To: Jason Xing, Pauli Virtanen
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
Jason Xing wrote:
> On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> >
> > Support enabling TX timestamping for some skbs, and track them until
> > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > completion report from hardware.
> >
> > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > driver requires changes in the driver API, and drivers mostly are going
> > to send the skb immediately.
> >
> > Make the default situation with no COMPLETION TX timestamping more
> > efficient by only counting packets in the queue when there is nothing to
> > track. When there is something to track, we need to make clones, since
> > the driver may modify sent skbs.
Why count packets at all? And if useful separate from completions,
should that be a separate patch?
> > +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);
>
> It's a bit strange that SCM_TSTAMP_SND is under the control of
> SKBTX_SW_TSTAMP. Can we use the same flag for both lines here
> directly? I suppose I would use SKBTX_SW_TSTAMP then.
This is the established behavior.
>
> > +
> > + /* COMPLETION tstamp is emitted for tracked skb later in Number of
> > + * Completed Packets event. Available only for flow controlled cases.
> > + *
> > + * TODO: SCO support without flowctl (needs to be done in drivers)
> > + */
> > + switch (conn->type) {
> > + case ISO_LINK:
> > + case ACL_LINK:
> > + case LE_LINK:
> > + break;
> > + case SCO_LINK:
> > + case ESCO_LINK:
> > + if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
> > + return;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> > + track = true;
> > +
> > + /* If nothing is tracked, just count extra skbs at the queue head */
> > + if (!track && !comp->tracked) {
> > + comp->extra++;
> > + return;
> > + }
> > +
> > + if (track) {
> > + skb = skb_clone_sk(skb);
> > + if (!skb)
> > + goto count_only;
> > +
> > + comp->tracked++;
> > + } else {
> > + skb = skb_clone(skb, GFP_KERNEL);
> > + if (!skb)
> > + goto count_only;
> > + }
What is the difference between track and comp->tracked
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/5] Bluetooth: ISO: add TX timestamping
2025-03-18 19:06 ` [PATCH v5 3/5] Bluetooth: ISO: add TX timestamping Pauli Virtanen
@ 2025-03-19 14:49 ` Willem de Bruijn
0 siblings, 0 replies; 24+ messages in thread
From: Willem de Bruijn @ 2025-03-19 14:49 UTC (permalink / raw)
To: Pauli Virtanen, linux-bluetooth
Cc: Pauli Virtanen, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
Pauli Virtanen wrote:
> Add BT_SCM_ERROR socket CMSG type.
>
> Support TX timestamping in ISO sockets.
>
> Support MSG_ERRQUEUE in ISO recvmsg.
>
> If a packet from sendmsg() is fragmented, only the first ACL fragment is
> timestamped.
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
> v5:
> - use sockcm_init -> hci_sockcm_init
>
> 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 0cb52a3308ba..3501a991f1c6 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;
>
> + hci_sockcm_init(&sockc, sk);
> +
> + if (msg->msg_controllen) {
> + err = sock_cmsg_send(sk, msg, &sockc);
> + if (err)
> + return err;
> + }
> +
Minor: do you want to return an error if the process set unexpected
sockc fields?
If allowing to set them now, but ignoring them, it will be harder to
support them later. As then it will result in a kernel behavioral
change for the same sendmsg() call.
Though I suppose that is already the case if all cmsg are ignored
currently. So fine to keep if you don't care.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-18 19:06 ` [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion Pauli Virtanen
2025-03-19 0:13 ` Jason Xing
2025-03-19 14:37 ` Willem de Bruijn
@ 2025-03-19 15:48 ` Paul Menzel
2025-03-20 14:43 ` Luiz Augusto von Dentz
2 siblings, 1 reply; 24+ messages in thread
From: Paul Menzel @ 2025-03-19 15:48 UTC (permalink / raw)
To: Pauli Virtanen
Cc: Luiz Augusto von Dentz, linux-bluetooth, netdev, davem, kuba,
willemdebruijn.kernel
Dear Pauli,
Thank you for your patch. Two minor comments, should you resend.
You could make the summary/title a statement:
Add COMPLETION timestamp on packet tx completion
Am 18.03.25 um 20:06 schrieb 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:
> v5:
> - back to decoupled COMPLETION & SND, like in v3
> - BPF reporting not implemented here
>
> Documentation/networking/timestamping.rst | 8 ++++++++
> include/linux/skbuff.h | 7 ++++---
> include/uapi/linux/errqueue.h | 1 +
> include/uapi/linux/net_tstamp.h | 6 ++++--
> net/core/skbuff.c | 2 ++
> net/ethtool/common.c | 1 +
> net/socket.c | 3 +++
> 7 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 61ef9da10e28..b8fef8101176 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> cumulative acknowledgment. The mechanism ignores SACK and FACK.
> This flag can be enabled via both socket options and control messages.
>
> +SOF_TIMESTAMPING_TX_COMPLETION:
> + Request tx timestamps on packet tx completion. The completion
> + timestamp is generated by the kernel when it receives packet a
> + completion report from the hardware. Hardware may report multiple
… receives packate a completion … sounds strange to me, but I am a
non-native speaker.
[…]
Kind regards,
Paul
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 14:44 ` Willem de Bruijn
@ 2025-03-19 17:43 ` Pauli Virtanen
2025-03-19 20:00 ` Willem de Bruijn
2025-03-19 21:16 ` Luiz Augusto von Dentz
0 siblings, 2 replies; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-19 17:43 UTC (permalink / raw)
To: Willem de Bruijn, Jason Xing
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba
Hi,
ke, 2025-03-19 kello 10:44 -0400, Willem de Bruijn kirjoitti:
> Jason Xing wrote:
> > On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > Support enabling TX timestamping for some skbs, and track them until
> > > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > > completion report from hardware.
> > >
> > > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > > driver requires changes in the driver API, and drivers mostly are going
> > > to send the skb immediately.
> > >
> > > Make the default situation with no COMPLETION TX timestamping more
> > > efficient by only counting packets in the queue when there is nothing to
> > > track. When there is something to track, we need to make clones, since
> > > the driver may modify sent skbs.
>
> Why count packets at all? And if useful separate from completions,
> should that be a separate patch?
This paragraph was commenting on the implementation of struct tx_queue,
and maybe how it works should be explicitly explained somewhere (code
comment?). Here's some explanation of it:
1) We have to hang on (clones of) skbs until completion reports for
them arrive, in order to emit COMPLETION timestamps. There's no
existing queue that does this in net/bluetooth (drivers may just copy
data & discard skbs, and they don't know about completion reports), so
something new needs to be added.
2) It is only needed for emitting COMPLETION timestamps. So it's better
to not do any extra work (clones etc.) when there are no such
timestamps to be emitted.
3) The new queue should work correctly when timestamping is turned on
or off, or only some packets are timestamped. It should also eventually
return to a state where no extra work is done, when new skbs don't
request COMPLETION timestamps.
struct tx_queue implements such queue that only "tracks" some skbs.
Logical structure:
HEAD
<no stored skb> }
<no stored skb> } tx_queue::extra is the number of non-tracked
... } logical items at queue head
<no stored skb> }
<tracked skb> } tx_queue::queue contains mixture of
<non-tracked skb> } tracked items (skb->sk != NULL) and
<non-tracked skb> } non-tracked items (skb->sk == NULL).
<tracked skb> } These are ordered after the "extra" items.
TAIL
tx_queue::tracked is the number of tracked skbs in tx_queue::queue.
hci_conn_tx_queue() determines whether skb is tracked (= COMPLETION
timestamp shall be emitted for it) and pushes a logical item to TAIL.
hci_conn_tx_dequeue() pops a logical item from HEAD, and emits
timestamp if it corresponds to a tracked skb.
When tracked == 0, queue() can just increment tx_queue::extra, and
dequeue() can remove any skb from tx_queue::queue, or if empty then
decrement tx_queue::extra. This allows it to return to a state with
empty tx_queue::queue when new skbs no longer request timestamps.
When tracked != 0, the ordering of items in the queue needs to be
respected strictly, so queue() always pushes real skb (tracked or not)
to TAIL, and dequeue() has to decrement extra to zero, before it can
pop skb from queue head.
> > > +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);
> >
> > It's a bit strange that SCM_TSTAMP_SND is under the control of
> > SKBTX_SW_TSTAMP. Can we use the same flag for both lines here
> > directly? I suppose I would use SKBTX_SW_TSTAMP then.
>
> This is the established behavior.
> >
> > > +
> > > + /* COMPLETION tstamp is emitted for tracked skb later in Number of
> > > + * Completed Packets event. Available only for flow controlled cases.
> > > + *
> > > + * TODO: SCO support without flowctl (needs to be done in drivers)
> > > + */
> > > + switch (conn->type) {
> > > + case ISO_LINK:
> > > + case ACL_LINK:
> > > + case LE_LINK:
> > > + break;
> > > + case SCO_LINK:
> > > + case ESCO_LINK:
> > > + if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
> > > + return;
> > > + break;
> > > + default:
> > > + return;
> > > + }
> > > +
> > > + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> > > + track = true;
> > > +
> > > + /* If nothing is tracked, just count extra skbs at the queue head */
> > > + if (!track && !comp->tracked) {
> > > + comp->extra++;
> > > + return;
> > > + }
> > > +
> > > + if (track) {
> > > + skb = skb_clone_sk(skb);
> > > + if (!skb)
> > > + goto count_only;
> > > +
> > > + comp->tracked++;
> > > + } else {
> > > + skb = skb_clone(skb, GFP_KERNEL);
> > > + if (!skb)
> > > + goto count_only;
> > > + }
>
> What is the difference between track and comp->tracked
I hope the answer above is clear.
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 0:39 ` Jason Xing
2025-03-19 14:44 ` Willem de Bruijn
@ 2025-03-19 18:21 ` Pauli Virtanen
2025-03-20 0:25 ` Jason Xing
1 sibling, 1 reply; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-19 18:21 UTC (permalink / raw)
To: Jason Xing
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
Hi,
ke, 2025-03-19 kello 08:39 +0800, Jason Xing kirjoitti:
> On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> >
> > Support enabling TX timestamping for some skbs, and track them until
> > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > completion report from hardware.
> >
> > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > driver requires changes in the driver API, and drivers mostly are going
> > to send the skb immediately.
> >
> > Make the default situation with no COMPLETION TX timestamping more
> > efficient by only counting packets in the queue when there is nothing to
> > track. When there is something to track, we need to make clones, since
> > the driver may modify sent skbs.
> >
> > The tx_q queue length is bounded by the hdev flow control, which will
> > not send new packets before it has got completion reports for old ones.
> >
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >
> > Notes:
> > v5:
> > - Add hci_sockm_init()
> > - Back to decoupled COMPLETION & SND, like in v3
> > - Handle SCO flow controlled case
> >
> > include/net/bluetooth/hci_core.h | 20 +++++
> > net/bluetooth/hci_conn.c | 122 +++++++++++++++++++++++++++++++
> > net/bluetooth/hci_core.c | 15 +++-
> > net/bluetooth/hci_event.c | 4 +
> > 4 files changed, 157 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index f78e4298e39a..5115da34f881 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;
> > @@ -1572,6 +1580,18 @@ 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);
> > +
> > +static inline void hci_sockcm_init(struct sockcm_cookie *sockc, struct sock *sk)
> > +{
> > + *sockc = (struct sockcm_cookie) {
> > + .tsflags = READ_ONCE(sk->sk_tsflags),
> > + };
> > +}
> > +
> > /*
> > * 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..95972fd4c784 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,122 @@ 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);
>
> It's a bit strange that SCM_TSTAMP_SND is under the control of
> SKBTX_SW_TSTAMP. Can we use the same flag for both lines here
> directly? I suppose I would use SKBTX_SW_TSTAMP then.
This is more or less open-coded skb_tx_timestamp(), which drivers do
before sending to HW, for the Bluetooth case. AFAIK it should be done
like this.
>
> > +
> > + /* COMPLETION tstamp is emitted for tracked skb later in Number of
> > + * Completed Packets event. Available only for flow controlled cases.
> > + *
> > + * TODO: SCO support without flowctl (needs to be done in drivers)
> > + */
> > + switch (conn->type) {
> > + case ISO_LINK:
> > + case ACL_LINK:
> > + case LE_LINK:
> > + break;
> > + case SCO_LINK:
> > + case ESCO_LINK:
> > + if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
> > + return;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> > + track = true;
> > +
> > + /* If nothing is tracked, just count extra skbs at the queue head */
> > + if (!track && !comp->tracked) {
> > + comp->extra++;
> > + return;
> > + }
> > +
> > + if (track) {
> > + skb = skb_clone_sk(skb);
> > + if (!skb)
> > + goto count_only;
> > +
> > + comp->tracked++;
> > + } else {
> > + skb = skb_clone(skb, GFP_KERNEL);
> > + if (!skb)
> > + goto count_only;
> > + }
> > +
> > + skb_queue_tail(&comp->queue, skb);
> > + return;
> > +
> > +count_only:
> > + /* Stop tracking skbs, and only count. This will not emit timestamps for
> > + * the packets, but if we get here something is more seriously wrong.
> > + */
> > + comp->tracked = 0;
> > + comp->extra += skb_queue_len(&comp->queue) + 1;
> > + skb_queue_purge(&comp->queue);
> > +}
> > +
> > +void hci_conn_tx_dequeue(struct hci_conn *conn)
> > +{
> > + struct tx_queue *comp = &conn->tx_q;
> > + struct sk_buff *skb;
> > +
> > + /* If there are tracked skbs, the counted extra go before dequeuing real
> > + * skbs, to keep ordering. When nothing is tracked, the ordering doesn't
> > + * matter so dequeue real skbs first to get rid of them ASAP.
> > + */
> > + if (comp->extra && (comp->tracked || skb_queue_empty(&comp->queue))) {
> > + comp->extra--;
> > + return;
> > + }
> > +
> > + skb = skb_dequeue(&comp->queue);
> > + if (!skb)
> > + return;
> > +
> > + if (skb->sk) {
> > + comp->tracked--;
>
> Need an explicit if statement here?
I think no, see explanation of how it works in the reply to Willem:
https://lore.kernel.org/linux-bluetooth/5882af942ef8cf5c9b4ce36a348f959807a387b0.camel@iki.fi/
> > + __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> > + SCM_TSTAMP_COMPLETION);
>
> This is the socket timestamping, and that's right. My minor question
> is: for the use of bpf timestamping (that should be easy as you've
> done in patch 1, I presume), I'm not sure if you're familiar with it.
> If not, I plan to implement it myself in a follow-up patch and then
> let you do some tests? Of course, I will provide the bpf test script
I saw the BPF timestamping things, but didn't look in full detail yet.
I don't know much about BPF, but IIUC, is it just as simple as adding
BPF_SOCK_OPS_TSTAMP_COMPLETION_CB and then modifying
skb_tstamp_tx_report_bpf_timestamping() accordingly?
I think we'd want to add also the BPF tests in the Bluetooth socket
timestamping tests
https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/test-runner.rst
https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/tester.h#n91
https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/iso-tester.c#n2275
https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/sco-tester.c#n755
https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/l2cap-tester.c#n1369
If you could show an example how to setup the BPF tstamps and pass the
resulting tstamp data in some way back to the application, that could
be very helpful (and I could postpone learning BPF for a little while
longer :)
> :)
>
> Thanks,
> Jason
>
> > + }
> > +
> > + kfree_skb(skb);
> > +}
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 94d9147612da..5eb0600bbd03 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -3029,6 +3029,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)
> > @@ -3575,7 +3582,7 @@ static void hci_sched_sco(struct hci_dev *hdev, __u8 type)
> > while (*cnt && (conn = hci_low_sent(hdev, type, "e))) {
> > 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)
> > @@ -3618,7 +3625,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--;
> > @@ -3674,7 +3681,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)--;
> > @@ -3708,7 +3715,7 @@ static void hci_sched_iso(struct hci_dev *hdev)
> > while (*cnt && (conn = hci_low_sent(hdev, ISO_LINK, "e))) {
> > 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 0df4a0e082c8..83990c975c1f 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4415,6 +4415,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);
> > @@ -4425,6 +4426,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
> >
> >
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 17:43 ` Pauli Virtanen
@ 2025-03-19 20:00 ` Willem de Bruijn
2025-03-19 21:30 ` Pauli Virtanen
2025-03-19 21:16 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 24+ messages in thread
From: Willem de Bruijn @ 2025-03-19 20:00 UTC (permalink / raw)
To: Pauli Virtanen, Willem de Bruijn, Jason Xing
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba
Pauli Virtanen wrote:
> Hi,
>
> ke, 2025-03-19 kello 10:44 -0400, Willem de Bruijn kirjoitti:
> > Jason Xing wrote:
> > > On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > >
> > > > Support enabling TX timestamping for some skbs, and track them until
> > > > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > > > completion report from hardware.
> > > >
> > > > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > > > driver requires changes in the driver API, and drivers mostly are going
> > > > to send the skb immediately.
> > > >
> > > > Make the default situation with no COMPLETION TX timestamping more
> > > > efficient by only counting packets in the queue when there is nothing to
> > > > track. When there is something to track, we need to make clones, since
> > > > the driver may modify sent skbs.
> >
> > Why count packets at all? And if useful separate from completions,
> > should that be a separate patch?
>
> This paragraph was commenting on the implementation of struct tx_queue,
> and maybe how it works should be explicitly explained somewhere (code
> comment?). Here's some explanation of it:
>
> 1) We have to hang on (clones of) skbs until completion reports for
> them arrive, in order to emit COMPLETION timestamps. There's no
> existing queue that does this in net/bluetooth (drivers may just copy
> data & discard skbs, and they don't know about completion reports), so
> something new needs to be added.
>
> 2) It is only needed for emitting COMPLETION timestamps. So it's better
> to not do any extra work (clones etc.) when there are no such
> timestamps to be emitted.
>
> 3) The new queue should work correctly when timestamping is turned on
> or off, or only some packets are timestamped. It should also eventually
> return to a state where no extra work is done, when new skbs don't
> request COMPLETION timestamps.
So far, fully understood.
> struct tx_queue implements such queue that only "tracks" some skbs.
> Logical structure:
>
> HEAD
> <no stored skb> }
> <no stored skb> } tx_queue::extra is the number of non-tracked
> ... } logical items at queue head
> <no stored skb> }
> <tracked skb> } tx_queue::queue contains mixture of
> <non-tracked skb> } tracked items (skb->sk != NULL) and
> <non-tracked skb> } non-tracked items (skb->sk == NULL).
> <tracked skb> } These are ordered after the "extra" items.
> TAIL
>
> tx_queue::tracked is the number of tracked skbs in tx_queue::queue.
>
> hci_conn_tx_queue() determines whether skb is tracked (= COMPLETION
> timestamp shall be emitted for it) and pushes a logical item to TAIL.
>
> hci_conn_tx_dequeue() pops a logical item from HEAD, and emits
> timestamp if it corresponds to a tracked skb.
>
> When tracked == 0, queue() can just increment tx_queue::extra, and
> dequeue() can remove any skb from tx_queue::queue, or if empty then
> decrement tx_queue::extra. This allows it to return to a state with
> empty tx_queue::queue when new skbs no longer request timestamps.
>
> When tracked != 0, the ordering of items in the queue needs to be
> respected strictly, so queue() always pushes real skb (tracked or not)
> to TAIL, and dequeue() has to decrement extra to zero, before it can
> pop skb from queue head.
Thanks. I did not understand why you need to queue or track any
sbs aside from those that have SKBTX_COMPLETION_TSTAMP.
If I follow correctly this is to be able to associate the tx
completion with the right skb on the queue.
The usual model in Ethernet drivers is that every tx descriptor (and
completion descriptor) in the ring is associated with a pure software
ring of metadata structures, which can point to an skb (or NULL).
In a pinch, instead the skb on the queue itself could record the
descriptor id that it is associated with. But hci_conn_tx_queue is
too far removed from the HW, so has no direct access to that. And
similarly hci_conn_tx_dequeue has no such low level details.
So long story short you indeed have to track this out of band with
a separate counter. I also don't immediately see a simpler way.
Though you can perhaps replace the skb_clone (not the skb_clone_sk!)
with some sentinel value that just helps count?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 17:43 ` Pauli Virtanen
2025-03-19 20:00 ` Willem de Bruijn
@ 2025-03-19 21:16 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 24+ messages in thread
From: Luiz Augusto von Dentz @ 2025-03-19 21:16 UTC (permalink / raw)
To: Pauli Virtanen
Cc: Willem de Bruijn, Jason Xing, linux-bluetooth, netdev, davem,
kuba
Hi Pauli,
On Wed, Mar 19, 2025 at 1:43 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> ke, 2025-03-19 kello 10:44 -0400, Willem de Bruijn kirjoitti:
> > Jason Xing wrote:
> > > On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > >
> > > > Support enabling TX timestamping for some skbs, and track them until
> > > > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > > > completion report from hardware.
> > > >
> > > > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > > > driver requires changes in the driver API, and drivers mostly are going
> > > > to send the skb immediately.
> > > >
> > > > Make the default situation with no COMPLETION TX timestamping more
> > > > efficient by only counting packets in the queue when there is nothing to
> > > > track. When there is something to track, we need to make clones, since
> > > > the driver may modify sent skbs.
> >
> > Why count packets at all? And if useful separate from completions,
> > should that be a separate patch?
>
> This paragraph was commenting on the implementation of struct tx_queue,
> and maybe how it works should be explicitly explained somewhere (code
> comment?). Here's some explanation of it:
>
> 1) We have to hang on (clones of) skbs until completion reports for
> them arrive, in order to emit COMPLETION timestamps. There's no
> existing queue that does this in net/bluetooth (drivers may just copy
> data & discard skbs, and they don't know about completion reports), so
> something new needs to be added.
>
> 2) It is only needed for emitting COMPLETION timestamps. So it's better
> to not do any extra work (clones etc.) when there are no such
> timestamps to be emitted.
>
> 3) The new queue should work correctly when timestamping is turned on
> or off, or only some packets are timestamped. It should also eventually
> return to a state where no extra work is done, when new skbs don't
> request COMPLETION timestamps.
I don't think it would hurt to put some of the above text as code
comments, but I think it would actually be better if we start doing
some documentation for Bluetooth in general, or we can put as part of
the manpages in userspace though that normally cover only the
interface not the internals, but I think it is a good idea to add
documentation to the likes of l2cap.rst covering the usage of
SO_TIMESTAMPING:
https://github.com/bluez/bluez/blob/master/doc/l2cap.rst
It is on my TODO to do something similar to SCO and ISO(once it is
stable) sockets.
>
> struct tx_queue implements such queue that only "tracks" some skbs.
> Logical structure:
>
> HEAD
> <no stored skb> }
> <no stored skb> } tx_queue::extra is the number of non-tracked
> ... } logical items at queue head
> <no stored skb> }
> <tracked skb> } tx_queue::queue contains mixture of
> <non-tracked skb> } tracked items (skb->sk != NULL) and
> <non-tracked skb> } non-tracked items (skb->sk == NULL).
> <tracked skb> } These are ordered after the "extra" items.
> TAIL
>
> tx_queue::tracked is the number of tracked skbs in tx_queue::queue.
>
> hci_conn_tx_queue() determines whether skb is tracked (= COMPLETION
> timestamp shall be emitted for it) and pushes a logical item to TAIL.
>
> hci_conn_tx_dequeue() pops a logical item from HEAD, and emits
> timestamp if it corresponds to a tracked skb.
>
> When tracked == 0, queue() can just increment tx_queue::extra, and
> dequeue() can remove any skb from tx_queue::queue, or if empty then
> decrement tx_queue::extra. This allows it to return to a state with
> empty tx_queue::queue when new skbs no longer request timestamps.
>
> When tracked != 0, the ordering of items in the queue needs to be
> respected strictly, so queue() always pushes real skb (tracked or not)
> to TAIL, and dequeue() has to decrement extra to zero, before it can
> pop skb from queue head.
>
>
> > > > +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);
> > >
> > > It's a bit strange that SCM_TSTAMP_SND is under the control of
> > > SKBTX_SW_TSTAMP. Can we use the same flag for both lines here
> > > directly? I suppose I would use SKBTX_SW_TSTAMP then.
> >
> > This is the established behavior.
> > >
> > > > +
> > > > + /* COMPLETION tstamp is emitted for tracked skb later in Number of
> > > > + * Completed Packets event. Available only for flow controlled cases.
> > > > + *
> > > > + * TODO: SCO support without flowctl (needs to be done in drivers)
> > > > + */
> > > > + switch (conn->type) {
> > > > + case ISO_LINK:
> > > > + case ACL_LINK:
> > > > + case LE_LINK:
> > > > + break;
> > > > + case SCO_LINK:
> > > > + case ESCO_LINK:
> > > > + if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
> > > > + return;
> > > > + break;
> > > > + default:
> > > > + return;
> > > > + }
> > > > +
> > > > + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> > > > + track = true;
> > > > +
> > > > + /* If nothing is tracked, just count extra skbs at the queue head */
> > > > + if (!track && !comp->tracked) {
> > > > + comp->extra++;
> > > > + return;
> > > > + }
> > > > +
> > > > + if (track) {
> > > > + skb = skb_clone_sk(skb);
> > > > + if (!skb)
> > > > + goto count_only;
> > > > +
> > > > + comp->tracked++;
> > > > + } else {
> > > > + skb = skb_clone(skb, GFP_KERNEL);
> > > > + if (!skb)
> > > > + goto count_only;
> > > > + }
> >
> > What is the difference between track and comp->tracked
>
> I hope the answer above is clear.
>
> --
> Pauli Virtanen
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 20:00 ` Willem de Bruijn
@ 2025-03-19 21:30 ` Pauli Virtanen
2025-03-19 21:35 ` Willem de Bruijn
0 siblings, 1 reply; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-19 21:30 UTC (permalink / raw)
To: Willem de Bruijn, Jason Xing
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba
Hi,
ke, 2025-03-19 kello 16:00 -0400, Willem de Bruijn kirjoitti:
> Pauli Virtanen wrote:
> > ke, 2025-03-19 kello 10:44 -0400, Willem de Bruijn kirjoitti:
> > > Jason Xing wrote:
> > > > On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > >
> > > > > Support enabling TX timestamping for some skbs, and track them until
> > > > > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > > > > completion report from hardware.
> > > > >
> > > > > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > > > > driver requires changes in the driver API, and drivers mostly are going
> > > > > to send the skb immediately.
> > > > >
> > > > > Make the default situation with no COMPLETION TX timestamping more
> > > > > efficient by only counting packets in the queue when there is nothing to
> > > > > track. When there is something to track, we need to make clones, since
> > > > > the driver may modify sent skbs.
> > >
> > > Why count packets at all? And if useful separate from completions,
> > > should that be a separate patch?
> >
> > This paragraph was commenting on the implementation of struct tx_queue,
> > and maybe how it works should be explicitly explained somewhere (code
> > comment?). Here's some explanation of it:
> >
> > 1) We have to hang on (clones of) skbs until completion reports for
> > them arrive, in order to emit COMPLETION timestamps. There's no
> > existing queue that does this in net/bluetooth (drivers may just copy
> > data & discard skbs, and they don't know about completion reports), so
> > something new needs to be added.
> >
> > 2) It is only needed for emitting COMPLETION timestamps. So it's better
> > to not do any extra work (clones etc.) when there are no such
> > timestamps to be emitted.
> >
> > 3) The new queue should work correctly when timestamping is turned on
> > or off, or only some packets are timestamped. It should also eventually
> > return to a state where no extra work is done, when new skbs don't
> > request COMPLETION timestamps.
>
> So far, fully understood.
>
> > struct tx_queue implements such queue that only "tracks" some skbs.
> > Logical structure:
> >
> > HEAD
> > <no stored skb> }
> > <no stored skb> } tx_queue::extra is the number of non-tracked
> > ... } logical items at queue head
> > <no stored skb> }
> > <tracked skb> } tx_queue::queue contains mixture of
> > <non-tracked skb> } tracked items (skb->sk != NULL) and
> > <non-tracked skb> } non-tracked items (skb->sk == NULL).
> > <tracked skb> } These are ordered after the "extra" items.
> > TAIL
> >
> > tx_queue::tracked is the number of tracked skbs in tx_queue::queue.
> >
> > hci_conn_tx_queue() determines whether skb is tracked (= COMPLETION
> > timestamp shall be emitted for it) and pushes a logical item to TAIL.
> >
> > hci_conn_tx_dequeue() pops a logical item from HEAD, and emits
> > timestamp if it corresponds to a tracked skb.
> >
> > When tracked == 0, queue() can just increment tx_queue::extra, and
> > dequeue() can remove any skb from tx_queue::queue, or if empty then
> > decrement tx_queue::extra. This allows it to return to a state with
> > empty tx_queue::queue when new skbs no longer request timestamps.
> >
> > When tracked != 0, the ordering of items in the queue needs to be
> > respected strictly, so queue() always pushes real skb (tracked or not)
> > to TAIL, and dequeue() has to decrement extra to zero, before it can
> > pop skb from queue head.
>
> Thanks. I did not understand why you need to queue or track any
> sbs aside from those that have SKBTX_COMPLETION_TSTAMP.
>
> If I follow correctly this is to be able to associate the tx
> completion with the right skb on the queue.
Yes, it was done to maintain the queue/dequeue ordering.
> The usual model in Ethernet drivers is that every tx descriptor (and
> completion descriptor) in the ring is associated with a pure software
> ring of metadata structures, which can point to an skb (or NULL).
>
> In a pinch, instead the skb on the queue itself could record the
> descriptor id that it is associated with. But hci_conn_tx_queue is
> too far removed from the HW, so has no direct access to that. And
> similarly hci_conn_tx_dequeue has no such low level details.
>
> So long story short you indeed have to track this out of band with
> a separate counter. I also don't immediately see a simpler way.
>
> Though you can perhaps replace the skb_clone (not the skb_clone_sk!)
> with some sentinel value that just helps count?
It probably could be done a bit smarter, it could eg. use something
else than skb_queue. Or, I think we can clobber cb here as the clones
are only used for timestamping, so:
struct tx_queue {
unsigned int pre_items;
struct sk_buff_head queue;
};
struct tx_queue_cb {
unsigned int post_items;
};
static void hci_tx_queue_push(struct tx_queue *q, struct sk_buff *skb)
{
struct tx_queue_cb *cb;
/* HEAD
* <non-tracked item> }
* ... } tx_queue::pre_items of these
* <non-tracked item> }
* <tracked skb1> <- tx_queue::queue first item
* <non-tracked item> }
* ... } ((struct tx_queue_cb *)skb1->cb)->post_items
* <non-tracked item> }
* ...
* <tracked skbn> <- tx_queue::queue n-th item
* <non-tracked item> }
* ... } ((struct tx_queue_cb *)skbn->cb)->post_items
* <non-tracked item> }
* TAIL
*/
if (skb) {
cb = (struct tx_queue_cb *)skb->cb;
cb->post_items = 0;
skb_queue_tail(&q->queue, skb);
} else {
skb = skb_peek_tail(&q->queue);
if (skb) {
cb = (struct tx_queue_cb *)skb->cb;
cb->post_items++;
} else {
q->pre_items++;
}
}
}
static struct sk_buff *hci_tx_queue_pop(struct tx_queue *q)
{
struct sk_buff *skb;
struct tx_queue_cb *cb;
if (q->pre_items) {
q->pre_items--;
return NULL;
}
skb = skb_dequeue(&q->queue);
if (skb) {
cb = (struct tx_queue_cb *)skb->cb;
q->pre_items += cb->post_items;
}
return skb;
}
void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
{
/* 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 without flowctl (needs to be done in drivers)
*/
switch (conn->type) {
case ISO_LINK:
case ACL_LINK:
case LE_LINK:
break;
case SCO_LINK:
case ESCO_LINK:
if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
return;
break;
default:
return;
}
if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
skb = skb_clone_sk(skb);
else
skb = NULL;
hci_tx_queue_push(&conn->tx_q, skb);
return;
}
void hci_conn_tx_dequeue(struct hci_conn *conn)
{
struct sk_buff *skb = hci_tx_queue_pop(&conn->tx_q);
if (skb) {
__skb_tstamp_tx(skb, NULL, NULL, skb->sk,
SCM_TSTAMP_COMPLETION);
kfree_skb(skb);
}
}
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 21:30 ` Pauli Virtanen
@ 2025-03-19 21:35 ` Willem de Bruijn
0 siblings, 0 replies; 24+ messages in thread
From: Willem de Bruijn @ 2025-03-19 21:35 UTC (permalink / raw)
To: Pauli Virtanen, Willem de Bruijn, Jason Xing
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba
Pauli Virtanen wrote:
> Hi,
>
> ke, 2025-03-19 kello 16:00 -0400, Willem de Bruijn kirjoitti:
> > Pauli Virtanen wrote:
> > > ke, 2025-03-19 kello 10:44 -0400, Willem de Bruijn kirjoitti:
> > > > Jason Xing wrote:
> > > > > On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > > >
> > > > > > Support enabling TX timestamping for some skbs, and track them until
> > > > > > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > > > > > completion report from hardware.
> > > > > >
> > > > > > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > > > > > driver requires changes in the driver API, and drivers mostly are going
> > > > > > to send the skb immediately.
> > > > > >
> > > > > > Make the default situation with no COMPLETION TX timestamping more
> > > > > > efficient by only counting packets in the queue when there is nothing to
> > > > > > track. When there is something to track, we need to make clones, since
> > > > > > the driver may modify sent skbs.
> > > >
> > > > Why count packets at all? And if useful separate from completions,
> > > > should that be a separate patch?
> > >
> > > This paragraph was commenting on the implementation of struct tx_queue,
> > > and maybe how it works should be explicitly explained somewhere (code
> > > comment?). Here's some explanation of it:
> > >
> > > 1) We have to hang on (clones of) skbs until completion reports for
> > > them arrive, in order to emit COMPLETION timestamps. There's no
> > > existing queue that does this in net/bluetooth (drivers may just copy
> > > data & discard skbs, and they don't know about completion reports), so
> > > something new needs to be added.
> > >
> > > 2) It is only needed for emitting COMPLETION timestamps. So it's better
> > > to not do any extra work (clones etc.) when there are no such
> > > timestamps to be emitted.
> > >
> > > 3) The new queue should work correctly when timestamping is turned on
> > > or off, or only some packets are timestamped. It should also eventually
> > > return to a state where no extra work is done, when new skbs don't
> > > request COMPLETION timestamps.
> >
> > So far, fully understood.
> >
> > > struct tx_queue implements such queue that only "tracks" some skbs.
> > > Logical structure:
> > >
> > > HEAD
> > > <no stored skb> }
> > > <no stored skb> } tx_queue::extra is the number of non-tracked
> > > ... } logical items at queue head
> > > <no stored skb> }
> > > <tracked skb> } tx_queue::queue contains mixture of
> > > <non-tracked skb> } tracked items (skb->sk != NULL) and
> > > <non-tracked skb> } non-tracked items (skb->sk == NULL).
> > > <tracked skb> } These are ordered after the "extra" items.
> > > TAIL
> > >
> > > tx_queue::tracked is the number of tracked skbs in tx_queue::queue.
> > >
> > > hci_conn_tx_queue() determines whether skb is tracked (= COMPLETION
> > > timestamp shall be emitted for it) and pushes a logical item to TAIL.
> > >
> > > hci_conn_tx_dequeue() pops a logical item from HEAD, and emits
> > > timestamp if it corresponds to a tracked skb.
> > >
> > > When tracked == 0, queue() can just increment tx_queue::extra, and
> > > dequeue() can remove any skb from tx_queue::queue, or if empty then
> > > decrement tx_queue::extra. This allows it to return to a state with
> > > empty tx_queue::queue when new skbs no longer request timestamps.
> > >
> > > When tracked != 0, the ordering of items in the queue needs to be
> > > respected strictly, so queue() always pushes real skb (tracked or not)
> > > to TAIL, and dequeue() has to decrement extra to zero, before it can
> > > pop skb from queue head.
> >
> > Thanks. I did not understand why you need to queue or track any
> > sbs aside from those that have SKBTX_COMPLETION_TSTAMP.
> >
> > If I follow correctly this is to be able to associate the tx
> > completion with the right skb on the queue.
>
> Yes, it was done to maintain the queue/dequeue ordering.
>
> > The usual model in Ethernet drivers is that every tx descriptor (and
> > completion descriptor) in the ring is associated with a pure software
> > ring of metadata structures, which can point to an skb (or NULL).
> >
> > In a pinch, instead the skb on the queue itself could record the
> > descriptor id that it is associated with. But hci_conn_tx_queue is
> > too far removed from the HW, so has no direct access to that. And
> > similarly hci_conn_tx_dequeue has no such low level details.
> >
> > So long story short you indeed have to track this out of band with
> > a separate counter. I also don't immediately see a simpler way.
> >
> > Though you can perhaps replace the skb_clone (not the skb_clone_sk!)
> > with some sentinel value that just helps count?
>
> It probably could be done a bit smarter, it could eg. use something
> else than skb_queue. Or, I think we can clobber cb here as the clones
> are only used for timestamping, so:
>
>
> struct tx_queue {
> unsigned int pre_items;
> struct sk_buff_head queue;
> };
>
> struct tx_queue_cb {
> unsigned int post_items;
> };
>
> static void hci_tx_queue_push(struct tx_queue *q, struct sk_buff *skb)
> {
> struct tx_queue_cb *cb;
>
> /* HEAD
> * <non-tracked item> }
> * ... } tx_queue::pre_items of these
> * <non-tracked item> }
> * <tracked skb1> <- tx_queue::queue first item
> * <non-tracked item> }
> * ... } ((struct tx_queue_cb *)skb1->cb)->post_items
> * <non-tracked item> }
> * ...
> * <tracked skbn> <- tx_queue::queue n-th item
> * <non-tracked item> }
> * ... } ((struct tx_queue_cb *)skbn->cb)->post_items
> * <non-tracked item> }
> * TAIL
> */
> if (skb) {
> cb = (struct tx_queue_cb *)skb->cb;
> cb->post_items = 0;
> skb_queue_tail(&q->queue, skb);
> } else {
> skb = skb_peek_tail(&q->queue);
> if (skb) {
> cb = (struct tx_queue_cb *)skb->cb;
> cb->post_items++;
> } else {
> q->pre_items++;
> }
> }
> }
>
> static struct sk_buff *hci_tx_queue_pop(struct tx_queue *q)
> {
> struct sk_buff *skb;
> struct tx_queue_cb *cb;
>
> if (q->pre_items) {
> q->pre_items--;
> return NULL;
> }
>
> skb = skb_dequeue(&q->queue);
> if (skb) {
> cb = (struct tx_queue_cb *)skb->cb;
> q->pre_items += cb->post_items;
> }
>
> return skb;
> }
>
> void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
> {
> /* 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 without flowctl (needs to be done in drivers)
> */
> switch (conn->type) {
> case ISO_LINK:
> case ACL_LINK:
> case LE_LINK:
> break;
> case SCO_LINK:
> case ESCO_LINK:
> if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
> return;
> break;
> default:
> return;
> }
>
> if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> skb = skb_clone_sk(skb);
> else
> skb = NULL;
>
> hci_tx_queue_push(&conn->tx_q, skb);
> return;
> }
>
> void hci_conn_tx_dequeue(struct hci_conn *conn)
> {
> struct sk_buff *skb = hci_tx_queue_pop(&conn->tx_q);
>
> if (skb) {
> __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> SCM_TSTAMP_COMPLETION);
> kfree_skb(skb);
> }
> }
Neat. To be clear, your call. Just if the expectation is that
timestamped packets are rare even when enabled (e.g., due to
sampling, or enabling only for one of many sockets), then avoiding
the skb_clone in the common case may be worthwhile.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
2025-03-19 18:21 ` Pauli Virtanen
@ 2025-03-20 0:25 ` Jason Xing
0 siblings, 0 replies; 24+ messages in thread
From: Jason Xing @ 2025-03-20 0:25 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, Luiz Augusto von Dentz, netdev, davem, kuba,
willemdebruijn.kernel
On Thu, Mar 20, 2025 at 2:21 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> ke, 2025-03-19 kello 08:39 +0800, Jason Xing kirjoitti:
> > On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > Support enabling TX timestamping for some skbs, and track them until
> > > packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> > > completion report from hardware.
> > >
> > > Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> > > driver requires changes in the driver API, and drivers mostly are going
> > > to send the skb immediately.
> > >
> > > Make the default situation with no COMPLETION TX timestamping more
> > > efficient by only counting packets in the queue when there is nothing to
> > > track. When there is something to track, we need to make clones, since
> > > the driver may modify sent skbs.
> > >
> > > The tx_q queue length is bounded by the hdev flow control, which will
> > > not send new packets before it has got completion reports for old ones.
> > >
> > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > ---
> > >
> > > Notes:
> > > v5:
> > > - Add hci_sockm_init()
> > > - Back to decoupled COMPLETION & SND, like in v3
> > > - Handle SCO flow controlled case
> > >
> > > include/net/bluetooth/hci_core.h | 20 +++++
> > > net/bluetooth/hci_conn.c | 122 +++++++++++++++++++++++++++++++
> > > net/bluetooth/hci_core.c | 15 +++-
> > > net/bluetooth/hci_event.c | 4 +
> > > 4 files changed, 157 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > index f78e4298e39a..5115da34f881 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;
> > > @@ -1572,6 +1580,18 @@ 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);
> > > +
> > > +static inline void hci_sockcm_init(struct sockcm_cookie *sockc, struct sock *sk)
> > > +{
> > > + *sockc = (struct sockcm_cookie) {
> > > + .tsflags = READ_ONCE(sk->sk_tsflags),
> > > + };
> > > +}
> > > +
> > > /*
> > > * 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..95972fd4c784 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,122 @@ 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);
> >
> > It's a bit strange that SCM_TSTAMP_SND is under the control of
> > SKBTX_SW_TSTAMP. Can we use the same flag for both lines here
> > directly? I suppose I would use SKBTX_SW_TSTAMP then.
>
> This is more or less open-coded skb_tx_timestamp(), which drivers do
> before sending to HW, for the Bluetooth case. AFAIK it should be done
> like this.
You're right. Sorry for misreading this part yesterday somehow...
>
> >
> > > +
> > > + /* COMPLETION tstamp is emitted for tracked skb later in Number of
> > > + * Completed Packets event. Available only for flow controlled cases.
> > > + *
> > > + * TODO: SCO support without flowctl (needs to be done in drivers)
> > > + */
> > > + switch (conn->type) {
> > > + case ISO_LINK:
> > > + case ACL_LINK:
> > > + case LE_LINK:
> > > + break;
> > > + case SCO_LINK:
> > > + case ESCO_LINK:
> > > + if (!hci_dev_test_flag(conn->hdev, HCI_SCO_FLOWCTL))
> > > + return;
> > > + break;
> > > + default:
> > > + return;
> > > + }
> > > +
> > > + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> > > + track = true;
> > > +
> > > + /* If nothing is tracked, just count extra skbs at the queue head */
> > > + if (!track && !comp->tracked) {
> > > + comp->extra++;
> > > + return;
> > > + }
> > > +
> > > + if (track) {
> > > + skb = skb_clone_sk(skb);
> > > + if (!skb)
> > > + goto count_only;
> > > +
> > > + comp->tracked++;
> > > + } else {
> > > + skb = skb_clone(skb, GFP_KERNEL);
> > > + if (!skb)
> > > + goto count_only;
> > > + }
> > > +
> > > + skb_queue_tail(&comp->queue, skb);
> > > + return;
> > > +
> > > +count_only:
> > > + /* Stop tracking skbs, and only count. This will not emit timestamps for
> > > + * the packets, but if we get here something is more seriously wrong.
> > > + */
> > > + comp->tracked = 0;
> > > + comp->extra += skb_queue_len(&comp->queue) + 1;
> > > + skb_queue_purge(&comp->queue);
> > > +}
> > > +
> > > +void hci_conn_tx_dequeue(struct hci_conn *conn)
> > > +{
> > > + struct tx_queue *comp = &conn->tx_q;
> > > + struct sk_buff *skb;
> > > +
> > > + /* If there are tracked skbs, the counted extra go before dequeuing real
> > > + * skbs, to keep ordering. When nothing is tracked, the ordering doesn't
> > > + * matter so dequeue real skbs first to get rid of them ASAP.
> > > + */
> > > + if (comp->extra && (comp->tracked || skb_queue_empty(&comp->queue))) {
> > > + comp->extra--;
> > > + return;
> > > + }
> > > +
> > > + skb = skb_dequeue(&comp->queue);
> > > + if (!skb)
> > > + return;
> > > +
> > > + if (skb->sk) {
> > > + comp->tracked--;
> >
> > Need an explicit if statement here?
>
> I think no, see explanation of how it works in the reply to Willem:
> https://lore.kernel.org/linux-bluetooth/5882af942ef8cf5c9b4ce36a348f959807a387b0.camel@iki.fi/
>
> > > + __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> > > + SCM_TSTAMP_COMPLETION);
> >
> > This is the socket timestamping, and that's right. My minor question
> > is: for the use of bpf timestamping (that should be easy as you've
> > done in patch 1, I presume), I'm not sure if you're familiar with it.
> > If not, I plan to implement it myself in a follow-up patch and then
> > let you do some tests? Of course, I will provide the bpf test script
>
> I saw the BPF timestamping things, but didn't look in full detail yet.
> I don't know much about BPF, but IIUC, is it just as simple as adding
> BPF_SOCK_OPS_TSTAMP_COMPLETION_CB and then modifying
> skb_tstamp_tx_report_bpf_timestamping() accordingly?
>
> I think we'd want to add also the BPF tests in the Bluetooth socket
> timestamping tests
Good to know that!
>
> https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/test-runner.rst
> https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/tester.h#n91
> https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/iso-tester.c#n2275
> https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/sco-tester.c#n755
> https://web.git.kernel.org/pub/scm/bluetooth/bluez.git/tree/tools/l2cap-tester.c#n1369
>
> If you could show an example how to setup the BPF tstamps and pass the
> resulting tstamp data in some way back to the application, that could
> be very helpful (and I could postpone learning BPF for a little while
> longer :)
Sure, I can leave it to you as long as you want to take over it :)
A simple case where I re-support the bpf extension for socket
timestamping goes like this commit [1]. And the selftest can be seen
here[2].
A rough thought is that letting it pass in
skb_tstamp_tx_report_bpf_timestamping() and adding corresponding BPF
flag as you mentioned before are the key to do it on top of the
current series. It should not be that hard. If you have any questions
on how we move on about this point, please feel free to reach out to
me anytime.
[1]: https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=6b98ec7e882af1c3088a88757e2226d06c8514f9
[2]: https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=f4924aec58dd9e14779f4bc11a6bf3a830a42a6c
Note: you will compile tools/testing/selftests/bpf/ and run
./test_progs -t net_timestamp to see how it works.
Thanks,
Jason
>
> > :)
> >
> > Thanks,
> > Jason
> >
> > > + }
> > > +
> > > + kfree_skb(skb);
> > > +}
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index 94d9147612da..5eb0600bbd03 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -3029,6 +3029,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)
> > > @@ -3575,7 +3582,7 @@ static void hci_sched_sco(struct hci_dev *hdev, __u8 type)
> > > while (*cnt && (conn = hci_low_sent(hdev, type, "e))) {
> > > 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)
> > > @@ -3618,7 +3625,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--;
> > > @@ -3674,7 +3681,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)--;
> > > @@ -3708,7 +3715,7 @@ static void hci_sched_iso(struct hci_dev *hdev)
> > > while (*cnt && (conn = hci_low_sent(hdev, ISO_LINK, "e))) {
> > > 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 0df4a0e082c8..83990c975c1f 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -4415,6 +4415,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);
> > > @@ -4425,6 +4426,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
> > >
> > >
>
> --
> Pauli Virtanen
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-19 15:48 ` Paul Menzel
@ 2025-03-20 14:43 ` Luiz Augusto von Dentz
2025-03-20 14:49 ` Willem de Bruijn
2025-03-20 17:12 ` Pauli Virtanen
0 siblings, 2 replies; 24+ messages in thread
From: Luiz Augusto von Dentz @ 2025-03-20 14:43 UTC (permalink / raw)
To: Paul Menzel
Cc: Pauli Virtanen, linux-bluetooth, netdev, davem, kuba,
willemdebruijn.kernel
Hi Pauli, Willem, Jason,
On Wed, Mar 19, 2025 at 11:48 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Pauli,
>
>
> Thank you for your patch. Two minor comments, should you resend.
>
> You could make the summary/title a statement:
>
> Add COMPLETION timestamp on packet tx completion
>
> Am 18.03.25 um 20:06 schrieb 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:
> > v5:
> > - back to decoupled COMPLETION & SND, like in v3
> > - BPF reporting not implemented here
> >
> > Documentation/networking/timestamping.rst | 8 ++++++++
> > include/linux/skbuff.h | 7 ++++---
> > include/uapi/linux/errqueue.h | 1 +
> > include/uapi/linux/net_tstamp.h | 6 ++++--
> > net/core/skbuff.c | 2 ++
> > net/ethtool/common.c | 1 +
> > net/socket.c | 3 +++
> > 7 files changed, 23 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > index 61ef9da10e28..b8fef8101176 100644
> > --- a/Documentation/networking/timestamping.rst
> > +++ b/Documentation/networking/timestamping.rst
> > @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> > cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > This flag can be enabled via both socket options and control messages.
> >
> > +SOF_TIMESTAMPING_TX_COMPLETION:
> > + Request tx timestamps on packet tx completion. The completion
> > + timestamp is generated by the kernel when it receives packet a
> > + completion report from the hardware. Hardware may report multiple
>
> … receives packate a completion … sounds strange to me, but I am a
> non-native speaker.
>
> […]
>
>
> Kind regards,
>
> Paul
Is v5 considered good enough to be merged into bluetooth-next and can
this be send to in this merge window or you think it is best to leave
for the next? In my opinion it could go in so we use the RC period to
stabilize it.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-20 14:43 ` Luiz Augusto von Dentz
@ 2025-03-20 14:49 ` Willem de Bruijn
2025-03-20 17:12 ` Pauli Virtanen
1 sibling, 0 replies; 24+ messages in thread
From: Willem de Bruijn @ 2025-03-20 14:49 UTC (permalink / raw)
To: Luiz Augusto von Dentz, Paul Menzel
Cc: Pauli Virtanen, linux-bluetooth, netdev, davem, kuba,
willemdebruijn.kernel
Luiz Augusto von Dentz wrote:
> Hi Pauli, Willem, Jason,
>
> On Wed, Mar 19, 2025 at 11:48 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> > Dear Pauli,
> >
> >
> > Thank you for your patch. Two minor comments, should you resend.
> >
> > You could make the summary/title a statement:
> >
> > Add COMPLETION timestamp on packet tx completion
> >
> > Am 18.03.25 um 20:06 schrieb 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:
> > > v5:
> > > - back to decoupled COMPLETION & SND, like in v3
> > > - BPF reporting not implemented here
> > >
> > > Documentation/networking/timestamping.rst | 8 ++++++++
> > > include/linux/skbuff.h | 7 ++++---
> > > include/uapi/linux/errqueue.h | 1 +
> > > include/uapi/linux/net_tstamp.h | 6 ++++--
> > > net/core/skbuff.c | 2 ++
> > > net/ethtool/common.c | 1 +
> > > net/socket.c | 3 +++
> > > 7 files changed, 23 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > index 61ef9da10e28..b8fef8101176 100644
> > > --- a/Documentation/networking/timestamping.rst
> > > +++ b/Documentation/networking/timestamping.rst
> > > @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> > > cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > > This flag can be enabled via both socket options and control messages.
> > >
> > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > + Request tx timestamps on packet tx completion. The completion
> > > + timestamp is generated by the kernel when it receives packet a
> > > + completion report from the hardware. Hardware may report multiple
> >
> > … receives packate a completion … sounds strange to me, but I am a
> > non-native speaker.
> >
> > […]
> >
> >
> > Kind regards,
> >
> > Paul
>
> Is v5 considered good enough to be merged into bluetooth-next and can
> this be send to in this merge window or you think it is best to leave
> for the next? In my opinion it could go in so we use the RC period to
> stabilize it.
I'm fine with merging as is.
Most of my comments were design points for consideration. If Pauli
prefers as is, no objections from me.
For the series:
Reviewed-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO
2025-03-18 19:06 [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO Pauli Virtanen
` (4 preceding siblings ...)
2025-03-18 19:06 ` [PATCH v5 5/5] Bluetooth: SCO: " Pauli Virtanen
@ 2025-03-20 16:10 ` patchwork-bot+bluetooth
5 siblings, 0 replies; 24+ messages in thread
From: patchwork-bot+bluetooth @ 2025-03-20 16:10 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, luiz.dentz, netdev, davem, kuba,
willemdebruijn.kernel
Hello:
This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
On Tue, 18 Mar 2025 21:06:41 +0200 you 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.1739988644.git.pav@iki.fi/
> 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/
>
> [...]
Here is the summary with links:
- [v5,1/5] net-timestamp: COMPLETION timestamp on packet tx completion
https://git.kernel.org/bluetooth/bluetooth-next/c/de4f56cf6cfd
- [v5,2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
https://git.kernel.org/bluetooth/bluetooth-next/c/2a1b83b8a4b2
- [v5,3/5] Bluetooth: ISO: add TX timestamping
https://git.kernel.org/bluetooth/bluetooth-next/c/6a536085b5e1
- [v5,4/5] Bluetooth: L2CAP: add TX timestamping
https://git.kernel.org/bluetooth/bluetooth-next/c/f698693b9664
- [v5,5/5] Bluetooth: SCO: add TX timestamping
https://git.kernel.org/bluetooth/bluetooth-next/c/01172ed6ff82
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-20 14:43 ` Luiz Augusto von Dentz
2025-03-20 14:49 ` Willem de Bruijn
@ 2025-03-20 17:12 ` Pauli Virtanen
2025-03-20 17:51 ` Jason Xing
1 sibling, 1 reply; 24+ messages in thread
From: Pauli Virtanen @ 2025-03-20 17:12 UTC (permalink / raw)
To: Luiz Augusto von Dentz, Paul Menzel
Cc: linux-bluetooth, netdev, davem, kuba, willemdebruijn.kernel
Hi,
to, 2025-03-20 kello 10:43 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli, Willem, Jason,
>
> On Wed, Mar 19, 2025 at 11:48 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> > Dear Pauli,
> >
> >
> > Thank you for your patch. Two minor comments, should you resend.
> >
> > You could make the summary/title a statement:
> >
> > Add COMPLETION timestamp on packet tx completion
> >
> > Am 18.03.25 um 20:06 schrieb 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:
> > > v5:
> > > - back to decoupled COMPLETION & SND, like in v3
> > > - BPF reporting not implemented here
> > >
> > > Documentation/networking/timestamping.rst | 8 ++++++++
> > > include/linux/skbuff.h | 7 ++++---
> > > include/uapi/linux/errqueue.h | 1 +
> > > include/uapi/linux/net_tstamp.h | 6 ++++--
> > > net/core/skbuff.c | 2 ++
> > > net/ethtool/common.c | 1 +
> > > net/socket.c | 3 +++
> > > 7 files changed, 23 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > index 61ef9da10e28..b8fef8101176 100644
> > > --- a/Documentation/networking/timestamping.rst
> > > +++ b/Documentation/networking/timestamping.rst
> > > @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> > > cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > > This flag can be enabled via both socket options and control messages.
> > >
> > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > + Request tx timestamps on packet tx completion. The completion
> > > + timestamp is generated by the kernel when it receives packet a
> > > + completion report from the hardware. Hardware may report multiple
> >
> > … receives packate a completion … sounds strange to me, but I am a
> > non-native speaker.
> >
> > […]
> >
> >
> > Kind regards,
> >
> > Paul
>
> Is v5 considered good enough to be merged into bluetooth-next and can
> this be send to in this merge window or you think it is best to leave
> for the next? In my opinion it could go in so we use the RC period to
> stabilize it.
From my side v5 should be good enough, if we want it now.
The remaining things were:
- Typo in documentation
- Better tx_queue implementation: probably not highly important for
these use cases, as queues are likely just a few packets so there
will be only that amount of non-timestamped skbs cloned.
In future, we might consider emitting SCM_TSTAMP_ACK timestamps
eg. for L2CAP LE credit-based flow control, and there this would
matter more as you'd need to hang on to the packets for a longer
time.
- I'd leave handling of unsupported sockcm fields as it is in
v5, as BT sockets have also previously just silently ignored
them so no change in behavior here.
- BPF: separate patch series, also need the tests for that
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion
2025-03-20 17:12 ` Pauli Virtanen
@ 2025-03-20 17:51 ` Jason Xing
0 siblings, 0 replies; 24+ messages in thread
From: Jason Xing @ 2025-03-20 17:51 UTC (permalink / raw)
To: Pauli Virtanen
Cc: Luiz Augusto von Dentz, Paul Menzel, linux-bluetooth, netdev,
davem, kuba, willemdebruijn.kernel
Hi,
On Fri, Mar 21, 2025 at 12:12 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> to, 2025-03-20 kello 10:43 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli, Willem, Jason,
> >
> > On Wed, Mar 19, 2025 at 11:48 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > >
> > > Dear Pauli,
> > >
> > >
> > > Thank you for your patch. Two minor comments, should you resend.
> > >
> > > You could make the summary/title a statement:
> > >
> > > Add COMPLETION timestamp on packet tx completion
> > >
> > > Am 18.03.25 um 20:06 schrieb 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:
> > > > v5:
> > > > - back to decoupled COMPLETION & SND, like in v3
> > > > - BPF reporting not implemented here
> > > >
> > > > Documentation/networking/timestamping.rst | 8 ++++++++
> > > > include/linux/skbuff.h | 7 ++++---
> > > > include/uapi/linux/errqueue.h | 1 +
> > > > include/uapi/linux/net_tstamp.h | 6 ++++--
> > > > net/core/skbuff.c | 2 ++
> > > > net/ethtool/common.c | 1 +
> > > > net/socket.c | 3 +++
> > > > 7 files changed, 23 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > > > index 61ef9da10e28..b8fef8101176 100644
> > > > --- a/Documentation/networking/timestamping.rst
> > > > +++ b/Documentation/networking/timestamping.rst
> > > > @@ -140,6 +140,14 @@ SOF_TIMESTAMPING_TX_ACK:
> > > > cumulative acknowledgment. The mechanism ignores SACK and FACK.
> > > > This flag can be enabled via both socket options and control messages.
> > > >
> > > > +SOF_TIMESTAMPING_TX_COMPLETION:
> > > > + Request tx timestamps on packet tx completion. The completion
> > > > + timestamp is generated by the kernel when it receives packet a
> > > > + completion report from the hardware. Hardware may report multiple
> > >
> > > … receives packate a completion … sounds strange to me, but I am a
> > > non-native speaker.
> > >
> > > […]
> > >
> > >
> > > Kind regards,
> > >
> > > Paul
> >
> > Is v5 considered good enough to be merged into bluetooth-next and can
> > this be send to in this merge window or you think it is best to leave
> > for the next? In my opinion it could go in so we use the RC period to
> > stabilize it.
>
> From my side v5 should be good enough, if we want it now.
Sorry for seeing this too late, I think I miss adding the reviewed-by
tags. Anyway,
Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
Thanks for working on this!!
>
> The remaining things were:
>
> - Typo in documentation
>
> - Better tx_queue implementation: probably not highly important for
> these use cases, as queues are likely just a few packets so there
> will be only that amount of non-timestamped skbs cloned.
>
> In future, we might consider emitting SCM_TSTAMP_ACK timestamps
> eg. for L2CAP LE credit-based flow control, and there this would
> matter more as you'd need to hang on to the packets for a longer
> time.
>
> - I'd leave handling of unsupported sockcm fields as it is in
> v5, as BT sockets have also previously just silently ignored
> them so no change in behavior here.
>
> - BPF: separate patch series, also need the tests for that
Right, I agree :)
Thanks,
Jason
>
> --
> Pauli Virtanen
>
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2025-03-20 17:52 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-18 19:06 [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO Pauli Virtanen
2025-03-18 19:06 ` [PATCH v5 1/5] net-timestamp: COMPLETION timestamp on packet tx completion Pauli Virtanen
2025-03-19 0:13 ` Jason Xing
2025-03-19 14:37 ` Willem de Bruijn
2025-03-19 15:48 ` Paul Menzel
2025-03-20 14:43 ` Luiz Augusto von Dentz
2025-03-20 14:49 ` Willem de Bruijn
2025-03-20 17:12 ` Pauli Virtanen
2025-03-20 17:51 ` Jason Xing
2025-03-18 19:06 ` [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping Pauli Virtanen
2025-03-19 0:39 ` Jason Xing
2025-03-19 14:44 ` Willem de Bruijn
2025-03-19 17:43 ` Pauli Virtanen
2025-03-19 20:00 ` Willem de Bruijn
2025-03-19 21:30 ` Pauli Virtanen
2025-03-19 21:35 ` Willem de Bruijn
2025-03-19 21:16 ` Luiz Augusto von Dentz
2025-03-19 18:21 ` Pauli Virtanen
2025-03-20 0:25 ` Jason Xing
2025-03-18 19:06 ` [PATCH v5 3/5] Bluetooth: ISO: add TX timestamping Pauli Virtanen
2025-03-19 14:49 ` Willem de Bruijn
2025-03-18 19:06 ` [PATCH v5 4/5] Bluetooth: L2CAP: " Pauli Virtanen
2025-03-18 19:06 ` [PATCH v5 5/5] Bluetooth: SCO: " Pauli Virtanen
2025-03-20 16:10 ` [PATCH v5 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO patchwork-bot+bluetooth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).