Message-ID: <CAL+tcoAAj0p=4h+MBYaN0v-mKQLNau43Av7crF7CVXFEnVL=LQ@mail.gmail.com>
Date: Thu, 20 Feb 2025 08:42:42 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Pauli Virtanen <pav@....fi>
Cc: linux-bluetooth@...r.kernel.org, 
	Luiz Augusto von Dentz <luiz.dentz@...il.com>, netdev@...r.kernel.org, davem@...emloft.net, 
	kuba@...nel.org, willemdebruijn.kernel@...il.com
Subject: Re: [PATCH v4 0/5] net: Bluetooth: add TX timestamping for ISO/L2CAP/SCO

On Thu, Feb 20, 2025 at 2:15 AM 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.
>
> - Better name in sof_timestamping_names
>
> - I decided to keep using sockcm_init(), to avoid open coding READ_ONCE
>   and since it's passed to sock_cmsg_send() which anyway also may init
>   such fields.

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

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

Thanks,
Jason

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

Powered by blists - more mailing lists