Hi Pauli, On Wed, Mar 19, 2025 at 1:43 PM Pauli Virtanen <pav@xxxxxx> 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@xxxxxx> 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
