Re: [RFC PATCH bpf-next v3 2/2] net: Add additional bit to support userspace timestamp type

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

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