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.
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
