[3/3,RFC] Bluetooth: enable bpf TX timestamping

Commit Message

Pauli Virtanen March 30, 2025, 12:23 p.m. UTC

Emit timestamps also for BPF timestamping.

***

The tskey management here is not quite right: see cover letter.
---
 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/hci_conn.c          | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

Comments

Martin KaFai Lau April 2, 2025, 1:34 a.m. UTC | #1

On 3/30/25 5:23 AM, Pauli Virtanen wrote:
> Emit timestamps also for BPF timestamping.
> 
> ***
> 
> The tskey management here is not quite right: see cover letter.
> ---
>   include/net/bluetooth/bluetooth.h |  1 +
>   net/bluetooth/hci_conn.c          | 21 +++++++++++++++++++--
>   2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index bbefde319f95..3b2e59cedd2d 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -383,6 +383,7 @@ struct bt_sock {
>   	struct list_head accept_q;
>   	struct sock *parent;
>   	unsigned long flags;
> +	atomic_t bpf_tskey;
>   	void (*skb_msg_name)(struct sk_buff *, void *, int *);
>   	void (*skb_put_cmsg)(struct sk_buff *, struct msghdr *, struct sock *);
>   };
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 95972fd4c784..7430df1c5822 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -28,6 +28,7 @@
>   #include <linux/export.h>
>   #include <linux/debugfs.h>
>   #include <linux/errqueue.h>
> +#include <linux/bpf-cgroup.h>
>   
>   #include <net/bluetooth/bluetooth.h>
>   #include <net/bluetooth/hci_core.h>
> @@ -3072,6 +3073,7 @@ void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
>   			    const struct sockcm_cookie *sockc)
>   {
>   	struct sock *sk = skb ? skb->sk : NULL;
> +	bool have_tskey = false;
>   
>   	/* This shall be called on a single skb of those generated by user
>   	 * sendmsg(), and only when the sendmsg() does not return error to
> @@ -3096,6 +3098,20 @@ void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
>   
>   			skb_shinfo(skb)->tskey = key - 1;
>   		}
> +		have_tskey = true;
> +	}
> +
> +	if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
> +	    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING)) {
> +		struct bt_sock *bt_sk = container_of(sk, struct bt_sock, sk);
> +		int key = atomic_inc_return(&bt_sk->bpf_tskey);

I don't think it needs to add "atomic_t bpf_tskey". Allow the bpf to decide what 
the skb_shinfo(skb)->tskey should be if it is not set by the userspace.

> +
> +		if (!have_tskey)
> +			skb_shinfo(skb)->tskey = key - 1;
> +
> +		bpf_skops_tx_timestamping(sk, skb,
> +					  BPF_SOCK_OPS_TSTAMP_SENDMSG_CB);
> +
>   	}
>   }
>

Pauli Virtanen April 2, 2025, 4:56 p.m. UTC | #2

Hi,

ti, 2025-04-01 kello 18:34 -0700, Martin KaFai Lau kirjoitti:
> On 3/30/25 5:23 AM, Pauli Virtanen wrote:
> > Emit timestamps also for BPF timestamping.
> > 
> > ***
> > 
> > The tskey management here is not quite right: see cover letter.
> > ---
> >   include/net/bluetooth/bluetooth.h |  1 +
> >   net/bluetooth/hci_conn.c          | 21 +++++++++++++++++++--
> >   2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index bbefde319f95..3b2e59cedd2d 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -383,6 +383,7 @@ struct bt_sock {
> >   	struct list_head accept_q;
> >   	struct sock *parent;
> >   	unsigned long flags;
> > +	atomic_t bpf_tskey;
> >   	void (*skb_msg_name)(struct sk_buff *, void *, int *);
> >   	void (*skb_put_cmsg)(struct sk_buff *, struct msghdr *, struct sock *);
> >   };
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 95972fd4c784..7430df1c5822 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -28,6 +28,7 @@
> >   #include <linux/export.h>
> >   #include <linux/debugfs.h>
> >   #include <linux/errqueue.h>
> > +#include <linux/bpf-cgroup.h>
> >   
> >   #include <net/bluetooth/bluetooth.h>
> >   #include <net/bluetooth/hci_core.h>
> > @@ -3072,6 +3073,7 @@ void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
> >   			    const struct sockcm_cookie *sockc)
> >   {
> >   	struct sock *sk = skb ? skb->sk : NULL;
> > +	bool have_tskey = false;
> >   
> >   	/* This shall be called on a single skb of those generated by user
> >   	 * sendmsg(), and only when the sendmsg() does not return error to
> > @@ -3096,6 +3098,20 @@ void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
> >   
> >   			skb_shinfo(skb)->tskey = key - 1;
> >   		}
> > +		have_tskey = true;
> > +	}
> > +
> > +	if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
> > +	    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING)) {
> > +		struct bt_sock *bt_sk = container_of(sk, struct bt_sock, sk);
> > +		int key = atomic_inc_return(&bt_sk->bpf_tskey);
> 
> I don't think it needs to add "atomic_t bpf_tskey". Allow the bpf to decide what 
> the skb_shinfo(skb)->tskey should be if it is not set by the userspace.

Ok. So if I understand correctly, the plan is that for UDP and
Bluetooth seqpacket sockets it works like this:

bpf_sock_ops_enable_tx_tstamp() does not set tskey.

Socket timestamping sets tskey the same way as previously.

So when both are in play, it shall work like:

* attach BPF timestamping
* setsockopt(SOF_TIMESTAMPING_SOFTWARE | SOF_TIMESTAMPING_OPT_ID)
* sendmsg() CMSG SO_TIMESTAMPING = 0
=> tskey 0 (unset)
* sendmsg() CMSG SO_TIMESTAMPING = SOF_TIMESTAMPING_TX_SOFTWARE
=> tskey 0
* sendmsg() CMSG SO_TIMESTAMPING = SOF_TIMESTAMPING_TX_SOFTWARE
=> tskey 1
* sendmsg() CMSG SO_TIMESTAMPING = 0
=> tskey 0 (unset)
* sendmsg() CMSG SO_TIMESTAMPING = 0
=> tskey 0 (unset)
* sendmsg() CMSG SO_TIMESTAMPING = 0
=> tskey 0 (unset)
* sendmsg() CMSG SO_TIMESTAMPING = SOF_TIMESTAMPING_TX_SOFTWARE
=> tskey 2

and BPF program has to handle the (unset) cases itself.

> 
> > +
> > +		if (!have_tskey)
> > +			skb_shinfo(skb)->tskey = key - 1;
> > +
> > +		bpf_skops_tx_timestamping(sk, skb,
> > +					  BPF_SOCK_OPS_TSTAMP_SENDMSG_CB);
> > +
> >   	}
> >   }
> >   
> 
>

Patch

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index bbefde319f95..3b2e59cedd2d 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -383,6 +383,7 @@  struct bt_sock {
 	struct list_head accept_q;
 	struct sock *parent;
 	unsigned long flags;
+	atomic_t bpf_tskey;
 	void (*skb_msg_name)(struct sk_buff *, void *, int *);
 	void (*skb_put_cmsg)(struct sk_buff *, struct msghdr *, struct sock *);
 };
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 95972fd4c784..7430df1c5822 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -28,6 +28,7 @@ 
 #include <linux/export.h>
 #include <linux/debugfs.h>
 #include <linux/errqueue.h>
+#include <linux/bpf-cgroup.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -3072,6 +3073,7 @@  void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
 			    const struct sockcm_cookie *sockc)
 {
 	struct sock *sk = skb ? skb->sk : NULL;
+	bool have_tskey = false;
 
 	/* This shall be called on a single skb of those generated by user
 	 * sendmsg(), and only when the sendmsg() does not return error to
@@ -3096,6 +3098,20 @@  void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
 
 			skb_shinfo(skb)->tskey = key - 1;
 		}
+		have_tskey = true;
+	}
+
+	if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
+	    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING)) {
+		struct bt_sock *bt_sk = container_of(sk, struct bt_sock, sk);
+		int key = atomic_inc_return(&bt_sk->bpf_tskey);
+
+		if (!have_tskey)
+			skb_shinfo(skb)->tskey = key - 1;
+
+		bpf_skops_tx_timestamping(sk, skb,
+					  BPF_SOCK_OPS_TSTAMP_SENDMSG_CB);
+
 	}
 }
 
@@ -3105,7 +3121,7 @@  void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
 	bool track = false;
 
 	/* Emit SND now, ie. just before sending to driver */
-	if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
+	if (skb_shinfo(skb)->tx_flags & (SKBTX_SW_TSTAMP | SKBTX_BPF))
 		__skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SND);
 
 	/* COMPLETION tstamp is emitted for tracked skb later in Number of
@@ -3127,7 +3143,8 @@  void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
 		return;
 	}
 
-	if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
+	if (skb->sk && (skb_shinfo(skb)->tx_flags &
+			(SKBTX_COMPLETION_TSTAMP | SKBTX_BPF)))
 		track = true;
 
 	/* If nothing is tracked, just count extra skbs at the queue head */