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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67dad8635c22c_5948294ac@willemb.c.googlers.com.notmuch>
Date: Wed, 19 Mar 2025 10:44:51 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Pauli Virtanen <pav@....fi>
Cc: linux-bluetooth@...r.kernel.org, 
 Luiz Augusto von Dentz <luiz.dentz@...il.com>, 
 netdev@...r.kernel.org, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 willemdebruijn.kernel@...il.com
Subject: Re: [PATCH v5 2/5] Bluetooth: add support for skb TX SND/COMPLETION
 timestamping

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?

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