Received-SPF: pass (google.com: domain of linux-kernel+bounces-158720-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 25 Apr 2024 10:31:35 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Abhishek Chauhan <quic_abchauha@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>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.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
Message-ID: <662a69475869_1de39b29415@willemb.c.googlers.com.notmuch>
In-Reply-To: <20240424222028.1080134-2-quic_abchauha@quicinc.com>
References: <20240424222028.1080134-1-quic_abchauha@quicinc.com>
 <20240424222028.1080134-2-quic_abchauha@quicinc.com>
Subject: Re: [RFC PATCH bpf-next v5 1/2] net: Rename mono_delivery_time to
 tstamp_type for scalabilty
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

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.

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

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
field going forward? I.e., not by ignoring the second bit.