[PATCH 0/3] bpf: TSTAMP_COMPLETION_CB timestamping + enable it for Bluetooth

[PATCH 0/3] bpf: TSTAMP_COMPLETION_CB timestamping + enable it for Bluetooth

[Pauli Virtanen]

Add BPF_SOCK_OPS_TSTAMP_COMPLETION_CB and emit it on completion
timestamps.

Enable that for Bluetooth.

Tests:
https://lore.kernel.org/linux-bluetooth/a74e58b9cf12bc9c64a024d18e6e58999202f853.1743336056.git.pav@iki.fi/

***

However, I don't quite see how to do the tskey management so
that BPF and socket timestamping do not interfere with each other.

The tskey counter here increments only for sendmsg() that have
timestamping turned on. IIUC this works similarly as for UDP.  I
understood the documentation so that stream sockets would do similarly,
but apparently TCP increments also for non-timestamped packets.

If BPF needs tskey while socket timestamping is off, we can't increment
sk_tskey, as that interferes with counting by user applications doing
socket timestamps.

Should the Bluetooth timestamping actually just increment the counters
for any packet, timestamped or not?

Pauli Virtanen (3):
  bpf: Add BPF_SOCK_OPS_TSTAMP_COMPLETION_CB callback
  [RFC] bpf: allow non-TCP skbs for bpf_sock_ops_enable_tx_tstamp
  [RFC] Bluetooth: enable bpf TX timestamping

 include/net/bluetooth/bluetooth.h |  1 +
 include/uapi/linux/bpf.h          |  5 +++++
 net/bluetooth/hci_conn.c          | 21 +++++++++++++++++++--
 net/core/filter.c                 | 12 ++++++++++--
 net/core/skbuff.c                 |  3 +++
 5 files changed, 38 insertions(+), 4 deletions(-)

-- 
2.49.0

[PATCH 1/3] bpf: Add BPF_SOCK_OPS_TSTAMP_COMPLETION_CB callback

[Pauli Virtanen]

Support SCM_TSTAMP_COMPLETION case for bpf timestamping.

Add a new sock_ops callback, BPF_SOCK_OPS_TSTAMP_SND_SW_CB. This
callback shall occur at the same timestamping point as the user
space's software SCM_TSTAMP_COMPLETION. The BPF program can use it to
get the same SCM_TSTAMP_COMPLETION timestamp without modifying the
user-space application.

Emitting BPF completion timestamps is enabled in separate patches.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/uapi/linux/bpf.h | 5 +++++
 net/core/skbuff.c        | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index defa5bb881f4..6cd918febcb3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7054,6 +7054,11 @@ enum {
 					 * sendmsg timestamp with corresponding
 					 * tskey.
 					 */
+	BPF_SOCK_OPS_TSTAMP_COMPLETION_CB,	/* Called on skb completion
+						 * report from hardware when
+						 * SK_BPF_CB_TX_TIMESTAMPING
+						 * feature is on.
+						 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 6cbf77bc61fc..9b2ff115eaf5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5552,6 +5552,9 @@ static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
 	case SCM_TSTAMP_ACK:
 		op = BPF_SOCK_OPS_TSTAMP_ACK_CB;
 		break;
+	case SCM_TSTAMP_COMPLETION:
+		op = BPF_SOCK_OPS_TSTAMP_COMPLETION_CB;
+		break;
 	default:
 		return;
 	}
-- 
2.49.0

[PATCH 2/3] [RFC] bpf: allow non-TCP skbs for bpf_sock_ops_enable_tx_tstamp

[Pauli Virtanen]

Change bpf_sock_ops_enable_tx_tstamp() kfunc to only set SKBTX_BPF flag,
so that it can be used also for non-TCP skbs.  Do not set TCP-specific
fields if the socket is not TCP.

***

Doing it this way requires a valid tskey is set by the socket family,
before BPF_SOCK_OPS_TSTAMP_SENDMSG_CB.  Alternatively, it maybe could be
hardcoded per socket type here, or some new proto_ops added.
---
 net/core/filter.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 46ae8eb7a03c..1300b0ef3620 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12127,6 +12127,7 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
 __bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops,
 					      u64 flags)
 {
+	struct sock *sk;
 	struct sk_buff *skb;
 
 	if (skops->op != BPF_SOCK_OPS_TSTAMP_SENDMSG_CB)
@@ -12135,10 +12136,17 @@ __bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops,
 	if (flags)
 		return -EINVAL;
 
+	sk = skops->sk;
+	if (!sk)
+		return -EINVAL;
+
 	skb = skops->skb;
 	skb_shinfo(skb)->tx_flags |= SKBTX_BPF;
-	TCP_SKB_CB(skb)->txstamp_ack |= TSTAMP_ACK_BPF;
-	skb_shinfo(skb)->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
+
+	if (sk_is_tcp(sk)) {
+		TCP_SKB_CB(skb)->txstamp_ack |= TSTAMP_ACK_BPF;
+		skb_shinfo(skb)->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
+	}
 
 	return 0;
 }
-- 
2.49.0

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

[Pauli Virtanen]

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);
+
+		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 */
-- 
2.49.0

Re: [PATCH 0/3] bpf: TSTAMP_COMPLETION_CB timestamping + enable it for Bluetooth

[Jason Xing]

Hi Pauli,

On Sun, Mar 30, 2025 at 8:23 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add BPF_SOCK_OPS_TSTAMP_COMPLETION_CB and emit it on completion
> timestamps.
>
> Enable that for Bluetooth.

Thanks for working on this!

It would be better if you can cc Martin in the next revision since he
is one of co-authors of BPF timestamping. Using
./scripts/get_maintainer.pl will show you which group people you're
supposed to cc.

>
> Tests:
> https://lore.kernel.org/linux-bluetooth/a74e58b9cf12bc9c64a024d18e6e58999202f853.1743336056.git.pav@iki.fi/
>
> ***
>
> However, I don't quite see how to do the tskey management so
> that BPF and socket timestamping do not interfere with each other.
>
> The tskey counter here increments only for sendmsg() that have
> timestamping turned on. IIUC this works similarly as for UDP.  I
> understood the documentation so that stream sockets would do similarly,
> but apparently TCP increments also for non-timestamped packets.

TCP increments sequence number for every skb regardless of BPF
timestamping feature. BPF timetamping uses the last byte of the last
skb to generate the tskey in tcp_tx_timestamp(). So it means the tskey
comes with the sequence number of each to-be-traced skb. It works for
both socket and BPF timestamping features.

>
> If BPF needs tskey while socket timestamping is off, we can't increment
> sk_tskey, as that interferes with counting by user applications doing
> socket timestamps.

That is the reason why in TCP we chose to implement the tskey of BPF
timestamping in the socket timestamping area. Please take a look at
tcp_tx_timestamp(). As for UDP implementation, it is a leftover that I
will make work next month.

>
> Should the Bluetooth timestamping actually just increment the counters
> for any packet, timestamped or not?

It's supposed to be the same tskey shared with socket timestamping so
that people don't need to separately take care of a new tskey
management.That is to say, if the socket timestamping and BPF
timestamping are turned on, sharing the same tskey will be consistent.

Thanks,
Jason

>
> Pauli Virtanen (3):
>   bpf: Add BPF_SOCK_OPS_TSTAMP_COMPLETION_CB callback
>   [RFC] bpf: allow non-TCP skbs for bpf_sock_ops_enable_tx_tstamp
>   [RFC] Bluetooth: enable bpf TX timestamping
>
>  include/net/bluetooth/bluetooth.h |  1 +
>  include/uapi/linux/bpf.h          |  5 +++++
>  net/bluetooth/hci_conn.c          | 21 +++++++++++++++++++--
>  net/core/filter.c                 | 12 ++++++++++--
>  net/core/skbuff.c                 |  3 +++
>  5 files changed, 38 insertions(+), 4 deletions(-)
>
> --
> 2.49.0
>

Re: [PATCH 0/3] bpf: TSTAMP_COMPLETION_CB timestamping + enable it for Bluetooth

[Pauli Virtanen]

Hi,

31. maaliskuuta 2025 3.39.46 GMT+03:00 Jason Xing <kerneljasonxing@gmail.com> kirjoitti:
>Hi Pauli,
>
>On Sun, Mar 30, 2025 at 8:23 PM Pauli Virtanen <pav@iki.fi> wrote:
>>
>> Add BPF_SOCK_OPS_TSTAMP_COMPLETION_CB and emit it on completion
>> timestamps.
>>
>> Enable that for Bluetooth.
>
>Thanks for working on this!
>
>It would be better if you can cc Martin in the next revision since he
>is one of co-authors of BPF timestamping. Using
>./scripts/get_maintainer.pl will show you which group people you're
>supposed to cc.
>
>>
>> Tests:
>> https://lore.kernel.org/linux-bluetooth/a74e58b9cf12bc9c64a024d18e6e58999202f853.1743336056.git.pav@iki.fi/
>>
>> ***
>>
>> However, I don't quite see how to do the tskey management so
>> that BPF and socket timestamping do not interfere with each other.
>>
>> The tskey counter here increments only for sendmsg() that have
>> timestamping turned on. IIUC this works similarly as for UDP.  I
>> understood the documentation so that stream sockets would do similarly,
>> but apparently TCP increments also for non-timestamped packets.
>
>TCP increments sequence number for every skb regardless of BPF
>timestamping feature. BPF timetamping uses the last byte of the last
>skb to generate the tskey in tcp_tx_timestamp(). So it means the tskey
>comes with the sequence number of each to-be-traced skb. It works for
>both socket and BPF timestamping features.
>
>>
>> If BPF needs tskey while socket timestamping is off, we can't increment
>> sk_tskey, as that interferes with counting by user applications doing
>> socket timestamps.
>
>That is the reason why in TCP we chose to implement the tskey of BPF
>timestamping in the socket timestamping area. Please take a look at
>tcp_tx_timestamp(). As for UDP implementation, it is a leftover that I
>will make work next month.

Ok, I guess I forgot tskey is reset to zero when socket timestamping is (re-)enabled. Then it's ok to increment the counter when either BPF or socket tstamps are enabled, although BPF will then have to live with tskey discontinuity when that happens.

But from the above, if BT stream socket tskey should work same as TCP (increment on every byte, also when timestamping is off) that probably should be fixed now while nobody is yet using the feature.

And for seqpacket, retain current behaviour of increment by one for each timestamped packet (and reset to 0 when stamping turned on).

>> Should the Bluetooth timestamping actually just increment the counters
>> for any packet, timestamped or not?
>
>It's supposed to be the same tskey shared with socket timestamping so
>that people don't need to separately take care of a new tskey
>management.That is to say, if the socket timestamping and BPF
>timestamping are turned on, sharing the same tskey will be consistent.
>
>Thanks,
>Jason
>
>>
>> Pauli Virtanen (3):
>>   bpf: Add BPF_SOCK_OPS_TSTAMP_COMPLETION_CB callback
>>   [RFC] bpf: allow non-TCP skbs for bpf_sock_ops_enable_tx_tstamp
>>   [RFC] Bluetooth: enable bpf TX timestamping
>>
>>  include/net/bluetooth/bluetooth.h |  1 +
>>  include/uapi/linux/bpf.h          |  5 +++++
>>  net/bluetooth/hci_conn.c          | 21 +++++++++++++++++++--
>>  net/core/filter.c                 | 12 ++++++++++--
>>  net/core/skbuff.c                 |  3 +++
>>  5 files changed, 38 insertions(+), 4 deletions(-)
>>
>> --
>> 2.49.0
>>

Re: [PATCH 3/3] [RFC] Bluetooth: enable bpf TX timestamping

[Martin KaFai Lau]

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);
> +
>   	}
>   }
>   

Re: [PATCH 3/3] [RFC] Bluetooth: enable bpf TX timestamping

[Pauli Virtanen]

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);
> > +
> >   	}
> >   }
> >   
> 
> 

-- 
Pauli Virtanen

Re: [PATCH 3/3] [RFC] Bluetooth: enable bpf TX timestamping

[Martin KaFai Lau]

On 4/2/25 9:56 AM, Pauli Virtanen wrote:
> 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.

The idea was that the bpf prog can directly set the skb_shinfo(skb)->tskey if it 
is not used by the userspace. iirc, it is where the discussion left at during 
the earlier UDP support thread.


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

The bpf_sock_ops_enable_tx_tstamp() has an used "u64 flags" argument. 
Potentially, it can use the higher 32bits to specify the 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);
>>> +
>>>    	}
>>>    }
>>>    
>>
>>
>