Message-ID: <1F98E20C-2F0D-4B3F-8DBE-3F0C44FCC65D@iki.fi>
Date: Mon, 31 Mar 2025 11:37:29 +0300
From: Pauli Virtanen <pav@....fi>
To: Jason Xing <kerneljasonxing@...il.com>
CC: linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org, bpf@...r.kernel.org,
 willemdebruijn.kernel@...il.com, Martin KaFai Lau <martin.lau@...ux.dev>
Subject: Re: [PATCH 0/3] bpf: TSTAMP_COMPLETION_CB timestamping + enable it for Bluetooth

Hi,

31. maaliskuuta 2025 3.39.46 GMT+03:00 Jason Xing <kerneljasonxing@...il.com> kirjoitti:
>Hi Pauli,
>
>On Sun, Mar 30, 2025 at 8:23 PM Pauli Virtanen <pav@....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
>>

Powered by blists - more mailing lists