Message-ID: <CABBYNZ+j=TYq27g-Ym7NnCm_Mhd=f8JZ=gT-Veq75BdHqzvUEw@mail.gmail.com>
Date: Wed, 19 Feb 2025 14:55:07 -0500
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Pauli Virtanen <pav@....fi>
Cc: linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org, 
	davem@...emloft.net, kuba@...nel.org, willemdebruijn.kernel@...il.com
Subject: Re: [PATCH v4 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO

Hi Pauli,

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

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

> - Better name in sof_timestamping_names
>
> - I decided to keep using sockcm_init(), to avoid open coding READ_ONCE
>   and since it's passed to sock_cmsg_send() which anyway also may init
>   such fields.
>
> v3:
> - Add new COMPLETION timestamp type, and emit it in HCI completion.
> - Emit SND instead of SCHED, when sending to driver.
> - Do not emit SCHED timestamps.
> - Don't safeguard tx_q length explicitly. Now that hci_sched_acl_blk()
>   is no more, the scheduler flow control is guaranteed to keep it
>   bounded.
> - Fix L2CAP stream sockets to use the bytestream timestamp conventions.
>
> Overview
> ========
>
> The packet flow in Bluetooth is the following. Timestamps added here
> indicated:
>
> user sendmsg() generates skbs
> |
> * skb waits in net/bluetooth queue for a free HW packet slot
> |
> * orphan skb, send to driver -> TSTAMP_SND
> |
> * driver: send packet data to transport (eg. USB)
> |
> * wait for transport completion
> |
> * driver: transport tx completion, free skb (some do this immediately)
> |
> * packet waits in HW side queue
> |
> * HCI report for packet completion -> TSTAMP_COMPLETION (for non-SCO)
>
> In addition, we may want to do the following in future (but not
> implemented in this series as we don't have ISO sequence number
> synchronization yet which is needed first, moreover e.g. Intel
> controllers return only zeros in timestamps):
>
> * if packet is ISO, send HCI LE Read ISO TX Sync
> |
> * HCI response -> hardware TSTAMP_SND for the packet the response
>   corresponds to if it was waiting for one, might not be possible
>   to get a tstamp for every packet
>
> Bluetooth does not have tx timestamps in the completion reports from
> hardware, and only for ISO packets there are HCI commands in
> specification for querying timestamps afterward.
>
> The drivers do not provide ways to get timestamps either, I'm also not
> aware if some devices would have vendor-specific commands to get them.
>
> Driver-side timestamps
> ======================
>
> Generating SND on driver side may be slightly more accurate, but that
> requires changing the BT driver API to not orphan skbs first.  In theory
> this probably won't cause problems, but it is not done in this patchset.
>
> For some of the drivers it won't gain much. E.g. btusb immediately
> submits the URB, so if one would emit SND just before submit (as
> drivers/net/usb/usbnet.c does), it is essentially identical to emitting
> before sending to driver.  btintel_pcie looks like it does synchronous
> send, so looks the same.  hci_serdev has internal queue, iiuc flushing
> as fast as data can be transferred, but it shouldn't be waiting for
> hardware slots due to HCI flow control.
>
> Unless HW buffers are full, packets mostly wait on the HW side.  E.g.
> with btusb (non-SCO) median time from sendmsg() to URB generation is
> ~0.1 ms, to USB completion ~0.5 ms, and HCI completion report at ~5 ms.
>
> The exception is SCO, for which HCI flow control is disabled, so they do
> not get completion events so it's possible to build up queues inside the
> driver. For SCO, COMPLETION needs to be generated from driver side, eg.
> for btusb maybe at URB completion.  This could be useful for SCO PCM
> modes (but which are more or less obsolete nowadays), where USB isoc
> data rate matches audio data rate, so queues on USB side may build up.
>
> Use cases
> =========
>
> In audio use cases we want to track and avoid queues building up, to
> control latency, especially in cases like ISO where the controller has a
> fixed schedule that the sending application must match.  E.g.
> application can aim to keep 1 packet in HW queue, so it has 2*7.5ms of
> slack for being woken up too late.
>
> Applications can use SND & COMPLETION timestamps to track in-kernel and
> in-HW packet queues separately.  This can matter for ISO, where the
> specification allows HW to use the timings when it gets packets to
> determine what packets are synchronized together. Applications can use
> SND to track that.
>
> Tests
> =====
>
> See
> https://lore.kernel.org/linux-bluetooth/cover.1739026302.git.pav@iki.fi/
>
> Pauli Virtanen (5):
>   net-timestamp: COMPLETION timestamp on packet tx completion
>   Bluetooth: add support for skb TX SND/COMPLETION timestamping
>   Bluetooth: ISO: add TX timestamping
>   Bluetooth: L2CAP: add TX timestamping
>   Bluetooth: SCO: add TX timestamping socket-level mechanism
>
>  Documentation/networking/timestamping.rst |   9 ++
>  include/net/bluetooth/bluetooth.h         |   1 +
>  include/net/bluetooth/hci_core.h          |  13 +++
>  include/net/bluetooth/l2cap.h             |   3 +-
>  include/uapi/linux/errqueue.h             |   1 +
>  include/uapi/linux/net_tstamp.h           |   6 +-
>  net/bluetooth/6lowpan.c                   |   2 +-
>  net/bluetooth/hci_conn.c                  | 118 ++++++++++++++++++++++
>  net/bluetooth/hci_core.c                  |  17 +++-
>  net/bluetooth/hci_event.c                 |   4 +
>  net/bluetooth/iso.c                       |  24 ++++-
>  net/bluetooth/l2cap_core.c                |  41 +++++++-
>  net/bluetooth/l2cap_sock.c                |  15 ++-
>  net/bluetooth/sco.c                       |  19 +++-
>  net/bluetooth/smp.c                       |   2 +-
>  net/core/sock.c                           |   2 +
>  net/ethtool/common.c                      |   1 +
>  17 files changed, 258 insertions(+), 20 deletions(-)
>
> --
> 2.48.1
>


-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists