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

[Martin KaFai Lau <martin.lau@linux.dev>]

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