Hi,
ke, 2025-03-19 kello 16:00 -0400, Willem de Bruijn kirjoitti:
> Pauli Virtanen wrote:
> > 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.
>
> So far, fully understood.
>
> > 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.
>
> Thanks. I did not understand why you need to queue or track any
> sbs aside from those that have SKBTX_COMPLETION_TSTAMP.
>
> If I follow correctly this is to be able to associate the tx
> completion with the right skb on the queue.
Yes, it was done to maintain the queue/dequeue ordering.
> The usual model in Ethernet drivers is that every tx descriptor (and
> completion descriptor) in the ring is associated with a pure software
> ring of metadata structures, which can point to an skb (or NULL).
>
> In a pinch, instead the skb on the queue itself could record the
> descriptor id that it is associated with. But hci_conn_tx_queue is
> too far removed from the HW, so has no direct access to that. And
> similarly hci_conn_tx_dequeue has no such low level details.
>
> So long story short you indeed have to track this out of band with
> a separate counter. I also don't immediately see a simpler way.
>
> Though you can perhaps replace the skb_clone (not the skb_clone_sk!)
> with some sentinel value that just helps count?
It probably could be done a bit smarter, it could eg. use something
else than skb_queue. Or, I think we can clobber cb here as the clones
are only used for timestamping, so:
struct tx_queue {
unsigned int pre_items;
struct sk_buff_head queue;
};
struct tx_queue_cb {
unsigned int post_items;
};
static void hci_tx_queue_push(struct tx_queue *q, struct sk_buff *skb)
{
struct tx_queue_cb *cb;
/* HEAD
* <non-tracked item> }
* ... } tx_queue::pre_items of these
* <non-tracked item> }
* <tracked skb1> <- tx_queue::queue first item
* <non-tracked item> }
* ... } ((struct tx_queue_cb *)skb1->cb)->post_items
* <non-tracked item> }
* ...
* <tracked skbn> <- tx_queue::queue n-th item
* <non-tracked item> }
* ... } ((struct tx_queue_cb *)skbn->cb)->post_items
* <non-tracked item> }
* TAIL
*/
if (skb) {
cb = (struct tx_queue_cb *)skb->cb;
cb->post_items = 0;
skb_queue_tail(&q->queue, skb);
} else {
skb = skb_peek_tail(&q->queue);
if (skb) {
cb = (struct tx_queue_cb *)skb->cb;
cb->post_items++;
} else {
q->pre_items++;
}
}
}
static struct sk_buff *hci_tx_queue_pop(struct tx_queue *q)
{
struct sk_buff *skb;
struct tx_queue_cb *cb;
if (q->pre_items) {
q->pre_items--;
return NULL;
}
skb = skb_dequeue(&q->queue);
if (skb) {
cb = (struct tx_queue_cb *)skb->cb;
q->pre_items += cb->post_items;
}
return skb;
}
void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
{
/* 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);
/* 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))
skb = skb_clone_sk(skb);
else
skb = NULL;
hci_tx_queue_push(&conn->tx_q, skb);
return;
}
void hci_conn_tx_dequeue(struct hci_conn *conn)
{
struct sk_buff *skb = hci_tx_queue_pop(&conn->tx_q);
if (skb) {
__skb_tstamp_tx(skb, NULL, NULL, skb->sk,
SCM_TSTAMP_COMPLETION);
kfree_skb(skb);
}
}
--
Pauli Virtanen
