Received-SPF: pass (google.com: domain of linux-kernel+bounces-145857-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <617e2577-8de2-4fde-bbfe-2d6280c48c29@linux.dev>
Date: Mon, 15 Apr 2024 13:00:42 -0700
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [RFC PATCH bpf-next v3 2/2] net: Add additional bit to support
 userspace timestamp type
To: Abhishek Chauhan <quic_abchauha@quicinc.com>,
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: kernel@quicinc.com, "David S. Miller" <davem@davemloft.net>,
 Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
 Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, Andrew Halaney <ahalaney@redhat.com>,
 Martin KaFai Lau <martin.lau@kernel.org>,
 Daniel Borkmann <daniel@iogearbox.net>, bpf <bpf@vger.kernel.org>
References: <20240412210125.1780574-1-quic_abchauha@quicinc.com>
 <20240412210125.1780574-3-quic_abchauha@quicinc.com>
 <661ad7d4c65da_3be9a7294e@willemb.c.googlers.com.notmuch>
Content-Language: en-US
From: Martin KaFai Lau <martin.lau@linux.dev>
In-Reply-To: <661ad7d4c65da_3be9a7294e@willemb.c.googlers.com.notmuch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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