Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jason Xing wrote:
> On Wed, Mar 19, 2025 at 3:10 AM Pauli Virtanen <pav@iki.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?

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