Message-ID: <CABBYNZJQc7x-b=_UQDjGbTVnY-iKASNzg=rTFXDRXyn_O+ohNQ@mail.gmail.com>
Date: Mon, 17 Mar 2025 13:50:36 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: Pauli Virtanen <pav@....fi>, 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 7:43 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> 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
> >
> >

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

-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists