[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <617e2577-8de2-4fde-bbfe-2d6280c48c29@linux.dev>
Date: Mon, 15 Apr 2024 13:00:42 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Abhishek Chauhan <quic_abchauha@...cinc.com>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: kernel@...cinc.com, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Andrew Halaney <ahalaney@...hat.com>,
Martin KaFai Lau <martin.lau@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, bpf <bpf@...r.kernel.org>
Subject: Re: [RFC PATCH bpf-next v3 2/2] net: Add additional bit to support
userspace timestamp type
On 4/13/24 12:07 PM, Willem de Bruijn wrote:
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index a83a2120b57f..b6346c21c3d4 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -827,7 +827,8 @@ enum skb_tstamp_type {
>> * @tstamp_type: When set, skb->tstamp has the
>> * delivery_time in mono clock base (i.e. EDT). Otherwise, the
>> * skb->tstamp has the (rcv) timestamp at ingress and
>> - * delivery_time at egress.
>> + * delivery_time at egress or skb->tstamp defined by skb->sk->sk_clockid
>> + * coming from userspace
>> * @napi_id: id of the NAPI struct this skb came from
>> * @sender_cpu: (aka @napi_id) source CPU in XPS
>> * @alloc_cpu: CPU which did the skb allocation.
>> @@ -955,7 +956,7 @@ struct sk_buff {
>> /* private: */
>> __u8 __mono_tc_offset[0];
>> /* public: */
>> - __u8 tstamp_type:1; /* See SKB_MONO_DELIVERY_TIME_MASK */
>> + __u8 tstamp_type:2; /* See SKB_MONO_DELIVERY_TIME_MASK */
>> #ifdef CONFIG_NET_XGRESS
>> __u8 tc_at_ingress:1; /* See TC_AT_INGRESS_MASK */
>> __u8 tc_skip_classify:1;
>
> A quick pahole for a fairly standard .config that I had laying around
> shows a hole after this list of bits, so no huge concerns there from
> adding a bit:
>
> __u8 slow_gro:1; /* 3: 4 1 */
> __u8 csum_not_inet:1; /* 3: 5 1 */
>
> /* XXX 2 bits hole, try to pack */
>
> __u16 tc_index; /* 4 2 */
>
>> @@ -1090,10 +1091,10 @@ struct sk_buff {
>> */
>> #ifdef __BIG_ENDIAN_BITFIELD
>> #define SKB_MONO_DELIVERY_TIME_MASK (1 << 7)
>> -#define TC_AT_INGRESS_MASK (1 << 6)
>> +#define TC_AT_INGRESS_MASK (1 << 5)
>
> Have to be careful when adding a new 2 bit tstamp_type with both bits
> set, that this does not incorrectly get interpreted as MONO.
>
> I haven't looked closely at the BPF API, but hopefully it can be
> extensible to return the specific type. If it is hardcoded to return
> either MONO or not, then only 0x1 should match, not 0x3.
Good point. I believe it is the best to have bpf to consider both bits in
tstamp_type:2 in filter.c to avoid the 0x3 surprise in the future. The BPF API
can be extended to support SKB_CLOCK_TAI.
Regardless, in bpf_convert_tstamp_write(), it still needs to clear both bits in
tstamp_type when it is at ingress. Right now it only clears the mono bit.
Then it may as well consider both tstamp_type:2 bits in
bpf_convert_tstamp_read() and bpf_convert_tstamp_type_read(). e.g.
bpf_convert_tstamp_type_read(), it should be a pretty straight forward change
because the SKB_CLOCK_* enum value should be a 1:1 mapping to the BPF_SKB_TSTAMP_*.
>
>> #else
>> #define SKB_MONO_DELIVERY_TIME_MASK (1 << 0)
>> -#define TC_AT_INGRESS_MASK (1 << 1)
>> +#define TC_AT_INGRESS_MASK (1 << 2)
>> #endif
>> #define SKB_BF_MONO_TC_OFFSET offsetof(struct sk_buff, __mono_tc_offset)
>>
Powered by blists - more mailing lists