Message-ID: <c49167b08b7af73bc633e1b195a30e0dd23735d7.camel@iki.fi>
Date: Wed, 19 Mar 2025 23:30:38 +0200
From: Pauli Virtanen <pav@....fi>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, Jason Xing
	 <kerneljasonxing@...il.com>
Cc: linux-bluetooth@...r.kernel.org, Luiz Augusto von Dentz
	 <luiz.dentz@...il.com>, netdev@...r.kernel.org, davem@...emloft.net, 
	kuba@...nel.org
Subject: Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION
 timestamping

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

Powered by blists - more mailing lists