Received-SPF: pass (google.com: domain of linux-kernel+bounces-159053-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <a84d314a-fca4-4317-9d33-0c7d3213c612@quicinc.com>
Date: Thu, 25 Apr 2024 12:02:42 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH bpf-next v5 1/2] net: Rename mono_delivery_time to
 tstamp_type for scalabilty
To: Willem de Bruijn <willemdebruijn.kernel@gmail.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>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Daniel Borkmann <daniel@iogearbox.net>, bpf <bpf@vger.kernel.org>
CC: <kernel@quicinc.com>
References: <20240424222028.1080134-1-quic_abchauha@quicinc.com>
 <20240424222028.1080134-2-quic_abchauha@quicinc.com>
 <662a69475869_1de39b29415@willemb.c.googlers.com.notmuch>
Content-Language: en-US
From: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>
In-Reply-To: <662a69475869_1de39b29415@willemb.c.googlers.com.notmuch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 4/25/2024 7:31 AM, Willem de Bruijn wrote:
> Abhishek Chauhan wrote:
>> mono_delivery_time was added to check if skb->tstamp has delivery
>> time in mono clock base (i.e. EDT) otherwise skb->tstamp has
>> timestamp in ingress and delivery_time at egress.
>>
>> Renaming the bitfield from mono_delivery_time to tstamp_type is for
>> extensibilty for other timestamps such as userspace timestamp
>> (i.e. SO_TXTIME) set via sock opts.
>>
>> As we are renaming the mono_delivery_time to tstamp_type, it makes
>> sense to start assigning tstamp_type based on enum defined
>> in this commit.
>>
>> Earlier we used bool arg flag to check if the tstamp is mono in
>> function skb_set_delivery_time, Now the signature of the functions
>> accepts tstamp_type to distinguish between mono and real time.
>>
>> Introduce a new function to set tstamp_type based on clockid. 
>>
>> In future tstamp_type:1 can be extended to support userspace timestamp
>> by increasing the bitfield.
>>
>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> 
>> +static inline void skb_set_tstamp_type_frm_clkid(struct sk_buff *skb,
>> +						  ktime_t kt, clockid_t clockid)
>> +{
> 
> Please don't garble words to save a few characters: .._from_clockid.
> 
Noted and apologies for using garble words here. I will correct it. 
> And this is essentially skb_set_delivery_type, just taking another
> type. So skb_set_delivery_type_(by|from)_clockid.
> 
> Also, instead of reimplementing the same logic with a different
> type, could implement as a conversion function that calls the main
> function. It won't save lines. But will avoid duplicate logic that
> needs to be kept in sync whenever there are future changes (fragile).
> 

I thought about doing this but as you remember that some places in the network stack, 
we are passing tstamp_type and some places we are passing clockid. 

So in the previous patchset we decided with two variants. 
1. One that assigns the tstamp_type directly 
2. Other one which computes tstamp_type based on clockid

But i agree on the above comment that if we implement two different variants 
then in future it requires maintenance to both functions. 

I will make sure i handle both cases in one func.   


> static inline void skb_set_delivery_type_by_clockid(struct sk_buff *skb,
> 						    ktime_t kt, clockid_t clockid)
> {
> 	u8 tstamp_type = SKB_CLOCK_REAL;
> 
> 	switch(clockid) {
> 	case CLOCK_REALTIME:
> 		break;
> 	case CLOCK_MONOTONIC:
> 		tstamp_type = SKB_CLOCK_MONO;
> 		break;
> 	default:
> 		WARN_ON_ONCE(1);
> 		kt = 0;
> 	};
> 
> 	skb_set_delivery_type(skb, kt, tstamp_type);
> }
> 
> 
>> +	skb->tstamp = kt;
>> +
>> +	if (!kt) {
>> +		skb->tstamp_type = SKB_CLOCK_REALTIME;
>> +		return;
>> +	}
>> +
>> +	switch (clockid) {
>> +	case CLOCK_REALTIME:
>> +		skb->tstamp_type = SKB_CLOCK_REALTIME;
>> +		break;
>> +	case CLOCK_MONOTONIC:
>> +		skb->tstamp_type = SKB_CLOCK_MONOTONIC;
>> +		break;
>> +	}
>> +}
>> +
>>  static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
>> -					 bool mono)
>> +					  u8 tstamp_type)
> 
> Indentation change: error?
>>> @@ -9444,7 +9444,7 @@ static struct bpf_insn *bpf_convert_tstamp_read(const struct bpf_prog *prog,
>>  					TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);
>>  		*insn++ = BPF_JMP32_IMM(BPF_JNE, tmp_reg,
>>  					TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK, 2);
>> -		/* skb->tc_at_ingress && skb->mono_delivery_time,
>> +		/* skb->tc_at_ingress && skb->tstamp_type:1,
> 
> Is the :1 a stale comment after we discussed how to handle the 2-bit
This is first patch which does not add tstamp_type:2 at the moment. 
This series is divided into two patches 
1. One patchset => Just rename (So the comment is still skb->tstamp_type:1)
2. Second patchset => add another bit (comment is changed to skb->tstamp_type:2)

> field going forward? I.e., not by ignoring the second bit.
> 
>