[RFC PATCH bpf-next v1 0/3] Rename mono_delivery_time to

[RFC PATCH bpf-next v1 0/3] Rename mono_delivery_time to

[Abhishek Chauhan]

Patch 1 :- This patch takes care of only renaming the mono delivery
timestamp to tstamp_type with no change in functionality of 
existing available code in kernel. 

Patch 2 :- Starts assigning tstamp_type with either mono or real and 
introduces a new enum in the skbuff.h, again no change in functionality 
of the existing available code in kernel , just making the code scalable 

Patch 3 :- Additional bit was added to support userspace timestamp to 
avoid tstamp drops in the forwarding path when testing TC-ETF. 
With this patch i am not sure what impacts it has towards BPF code. 
I need upstream BPF community to help me in adding the necessary BPF 
changes to avoid any BPF test case failures. 
I haven't changed any of the BPF functionalities and hence i need 
upstream BPF help to assist me with those changes so i can make them as 
part of this patch.    


Abhishek Chauhan (3):
  net: Rename mono_delivery_time to tstamp_type for scalibilty
  net: assign enum to skb->tstamp_type to distinguish between tstamp
  net: Add additional bit to support userspace timestamp type

 include/linux/skbuff.h                     | 40 ++++++++++++++++------
 include/net/inet_frag.h                    |  4 +--
 net/bridge/netfilter/nf_conntrack_bridge.c |  6 ++--
 net/core/dev.c                             |  2 +-
 net/core/filter.c                          |  8 ++---
 net/ipv4/inet_fragment.c                   |  2 +-
 net/ipv4/ip_fragment.c                     |  2 +-
 net/ipv4/ip_output.c                       | 10 +++---
 net/ipv4/raw.c                             |  2 +-
 net/ipv4/tcp_output.c                      | 14 ++++----
 net/ipv6/ip6_output.c                      |  8 ++---
 net/ipv6/netfilter.c                       |  6 ++--
 net/ipv6/netfilter/nf_conntrack_reasm.c    |  2 +-
 net/ipv6/raw.c                             |  2 +-
 net/ipv6/reassembly.c                      |  2 +-
 net/ipv6/tcp_ipv6.c                        |  2 +-
 net/packet/af_packet.c                     |  6 ++--
 net/sched/act_bpf.c                        |  4 +--
 net/sched/cls_bpf.c                        |  4 +--
 19 files changed, 73 insertions(+), 53 deletions(-)

-- 
2.25.1

[RFC PATCH bpf-next v1 1/3] net: Rename mono_delivery_time to tstamp_type for scalabilty

[Abhishek Chauhan]

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.

Bridge driver today has no support to forward the userspace timestamp
packets and ends up resetting the timestamp. ETF qdisc checks the
packet coming from userspace and encounters to be 0 thereby dropping
time sensitive packets. These changes will allow userspace timestamps
packets to be forwarded from the bridge to NIC drivers.

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>
---
 include/linux/skbuff.h                     | 18 +++++++++---------
 include/net/inet_frag.h                    |  4 ++--
 net/bridge/netfilter/nf_conntrack_bridge.c |  6 +++---
 net/core/dev.c                             |  2 +-
 net/core/filter.c                          |  8 ++++----
 net/ipv4/inet_fragment.c                   |  2 +-
 net/ipv4/ip_fragment.c                     |  2 +-
 net/ipv4/ip_output.c                       |  8 ++++----
 net/ipv6/ip6_output.c                      |  6 +++---
 net/ipv6/netfilter.c                       |  6 +++---
 net/ipv6/netfilter/nf_conntrack_reasm.c    |  2 +-
 net/ipv6/reassembly.c                      |  2 +-
 net/sched/act_bpf.c                        |  4 ++--
 net/sched/cls_bpf.c                        |  4 ++--
 14 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0c7c67b3a87b..8210d699d8e9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -821,7 +821,7 @@ typedef unsigned char *sk_buff_data_t;
  *	@dst_pending_confirm: need to confirm neighbour
  *	@decrypted: Decrypted SKB
  *	@slow_gro: state present at GRO time, slower prepare step required
- *	@mono_delivery_time: When set, skb->tstamp has the
+ *	@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.
@@ -955,7 +955,7 @@ struct sk_buff {
 	/* private: */
 	__u8			__mono_tc_offset[0];
 	/* public: */
-	__u8			mono_delivery_time:1;	/* See SKB_MONO_DELIVERY_TIME_MASK */
+	__u8			tstamp_type:1;	/* 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;
@@ -4257,7 +4257,7 @@ static inline void skb_get_new_timestampns(const struct sk_buff *skb,
 static inline void __net_timestamp(struct sk_buff *skb)
 {
 	skb->tstamp = ktime_get_real();
-	skb->mono_delivery_time = 0;
+	skb->tstamp_type = 0;
 }
 
 static inline ktime_t net_timedelta(ktime_t t)
@@ -4269,7 +4269,7 @@ static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
 					 bool mono)
 {
 	skb->tstamp = kt;
-	skb->mono_delivery_time = kt && mono;
+	skb->tstamp_type = kt && mono;
 }
 
 DECLARE_STATIC_KEY_FALSE(netstamp_needed_key);
@@ -4279,8 +4279,8 @@ DECLARE_STATIC_KEY_FALSE(netstamp_needed_key);
  */
 static inline void skb_clear_delivery_time(struct sk_buff *skb)
 {
-	if (skb->mono_delivery_time) {
-		skb->mono_delivery_time = 0;
+	if (skb->tstamp_type) {
+		skb->tstamp_type = 0;
 		if (static_branch_unlikely(&netstamp_needed_key))
 			skb->tstamp = ktime_get_real();
 		else
@@ -4290,7 +4290,7 @@ static inline void skb_clear_delivery_time(struct sk_buff *skb)
 
 static inline void skb_clear_tstamp(struct sk_buff *skb)
 {
-	if (skb->mono_delivery_time)
+	if (skb->tstamp_type)
 		return;
 
 	skb->tstamp = 0;
@@ -4298,7 +4298,7 @@ static inline void skb_clear_tstamp(struct sk_buff *skb)
 
 static inline ktime_t skb_tstamp(const struct sk_buff *skb)
 {
-	if (skb->mono_delivery_time)
+	if (skb->tstamp_type)
 		return 0;
 
 	return skb->tstamp;
@@ -4306,7 +4306,7 @@ static inline ktime_t skb_tstamp(const struct sk_buff *skb)
 
 static inline ktime_t skb_tstamp_cond(const struct sk_buff *skb, bool cond)
 {
-	if (!skb->mono_delivery_time && skb->tstamp)
+	if (!skb->tstamp_type && skb->tstamp)
 		return skb->tstamp;
 
 	if (static_branch_unlikely(&netstamp_needed_key) || cond)
diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 153960663ce4..5af6eb14c5db 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -76,7 +76,7 @@ struct frag_v6_compare_key {
  * @stamp: timestamp of the last received fragment
  * @len: total length of the original datagram
  * @meat: length of received fragments so far
- * @mono_delivery_time: stamp has a mono delivery time (EDT)
+ * @tstamp_type: stamp has a mono delivery time (EDT)
  * @flags: fragment queue flags
  * @max_size: maximum received fragment size
  * @fqdir: pointer to struct fqdir
@@ -97,7 +97,7 @@ struct inet_frag_queue {
 	ktime_t			stamp;
 	int			len;
 	int			meat;
-	u8			mono_delivery_time;
+	u8			tstamp_type;
 	__u8			flags;
 	u16			max_size;
 	struct fqdir		*fqdir;
diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index 6f877e31709b..989435bd1690 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -32,7 +32,7 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
 					   struct sk_buff *))
 {
 	int frag_max_size = BR_INPUT_SKB_CB(skb)->frag_max_size;
-	bool mono_delivery_time = skb->mono_delivery_time;
+	bool tstamp_type = skb->tstamp_type;
 	unsigned int hlen, ll_rs, mtu;
 	ktime_t tstamp = skb->tstamp;
 	struct ip_frag_state state;
@@ -82,7 +82,7 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
 			if (iter.frag)
 				ip_fraglist_prepare(skb, &iter);
 
-			skb_set_delivery_time(skb, tstamp, mono_delivery_time);
+			skb_set_delivery_time(skb, tstamp, tstamp_type);
 			err = output(net, sk, data, skb);
 			if (err || !iter.frag)
 				break;
@@ -113,7 +113,7 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
 			goto blackhole;
 		}
 
-		skb_set_delivery_time(skb2, tstamp, mono_delivery_time);
+		skb_set_delivery_time(skb2, tstamp, tstamp_type);
 		err = output(net, sk, data, skb2);
 		if (err)
 			goto blackhole;
diff --git a/net/core/dev.c b/net/core/dev.c
index 303a6ff46e4e..8b88f8118052 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2113,7 +2113,7 @@ EXPORT_SYMBOL(net_disable_timestamp);
 static inline void net_timestamp_set(struct sk_buff *skb)
 {
 	skb->tstamp = 0;
-	skb->mono_delivery_time = 0;
+	skb->tstamp_type = 0;
 	if (static_branch_unlikely(&netstamp_needed_key))
 		skb->tstamp = ktime_get_real();
 }
diff --git a/net/core/filter.c b/net/core/filter.c
index 8adf95765cdd..0f535defdd2c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7698,13 +7698,13 @@ BPF_CALL_3(bpf_skb_set_tstamp, struct sk_buff *, skb,
 		if (!tstamp)
 			return -EINVAL;
 		skb->tstamp = tstamp;
-		skb->mono_delivery_time = 1;
+		skb->tstamp_type = 1;
 		break;
 	case BPF_SKB_TSTAMP_UNSPEC:
 		if (tstamp)
 			return -EINVAL;
 		skb->tstamp = 0;
-		skb->mono_delivery_time = 0;
+		skb->tstamp_type = 0;
 		break;
 	default:
 		return -EINVAL;
@@ -9413,7 +9413,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,
 		 * read 0 as the (rcv) timestamp.
 		 */
 		*insn++ = BPF_MOV64_IMM(value_reg, 0);
@@ -9438,7 +9438,7 @@ static struct bpf_insn *bpf_convert_tstamp_write(const struct bpf_prog *prog,
 	 * the bpf prog is aware the tstamp could have delivery time.
 	 * Thus, write skb->tstamp as is if tstamp_type_access is true.
 	 * Otherwise, writing at ingress will have to clear the
-	 * mono_delivery_time bit also.
+	 * mono_delivery_time (skb->tstamp_type:1)bit also.
 	 */
 	if (!prog->tstamp_type_access) {
 		__u8 tmp_reg = BPF_REG_AX;
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 7072fc0783ef..25f8ee14ea73 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -578,7 +578,7 @@ void inet_frag_reasm_finish(struct inet_frag_queue *q, struct sk_buff *head,
 	skb_mark_not_on_list(head);
 	head->prev = NULL;
 	head->tstamp = q->stamp;
-	head->mono_delivery_time = q->mono_delivery_time;
+	head->tstamp_type = q->tstamp_type;
 }
 EXPORT_SYMBOL(inet_frag_reasm_finish);
 
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index a4941f53b523..1edba0d0ae90 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -355,7 +355,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
 		qp->iif = dev->ifindex;
 
 	qp->q.stamp = skb->tstamp;
-	qp->q.mono_delivery_time = skb->mono_delivery_time;
+	qp->q.tstamp_type = skb->tstamp_type;
 	qp->q.meat += skb->len;
 	qp->ecn |= ecn;
 	add_frag_mem_limit(qp->q.fqdir, skb->truesize);
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 1fe794967211..e8ec7e8ae2e0 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -764,7 +764,7 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 {
 	struct iphdr *iph;
 	struct sk_buff *skb2;
-	bool mono_delivery_time = skb->mono_delivery_time;
+	bool tstamp_type = skb->tstamp_type;
 	struct rtable *rt = skb_rtable(skb);
 	unsigned int mtu, hlen, ll_rs;
 	struct ip_fraglist_iter iter;
@@ -856,7 +856,7 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 				}
 			}
 
-			skb_set_delivery_time(skb, tstamp, mono_delivery_time);
+			skb_set_delivery_time(skb, tstamp, tstamp_type);
 			err = output(net, sk, skb);
 
 			if (!err)
@@ -912,7 +912,7 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 		/*
 		 *	Put this fragment into the sending queue.
 		 */
-		skb_set_delivery_time(skb2, tstamp, mono_delivery_time);
+		skb_set_delivery_time(skb2, tstamp, tstamp_type);
 		err = output(net, sk, skb2);
 		if (err)
 			goto fail;
@@ -1649,7 +1649,7 @@ void ip_send_unicast_reply(struct sock *sk, struct sk_buff *skb,
 			  arg->csumoffset) = csum_fold(csum_add(nskb->csum,
 								arg->csum));
 		nskb->ip_summed = CHECKSUM_NONE;
-		nskb->mono_delivery_time = !!transmit_time;
+		nskb->tstamp_type = !!transmit_time;
 		if (txhash)
 			skb_set_hash(nskb, txhash, PKT_HASH_TYPE_L4);
 		ip_push_pending_frames(sk, &fl4);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index b9dd3a66e423..61ddc9549160 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -859,7 +859,7 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 	struct rt6_info *rt = (struct rt6_info *)skb_dst(skb);
 	struct ipv6_pinfo *np = skb->sk && !dev_recursion_level() ?
 				inet6_sk(skb->sk) : NULL;
-	bool mono_delivery_time = skb->mono_delivery_time;
+	bool tstamp_type = skb->tstamp_type;
 	struct ip6_frag_state state;
 	unsigned int mtu, hlen, nexthdr_offset;
 	ktime_t tstamp = skb->tstamp;
@@ -955,7 +955,7 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 			if (iter.frag)
 				ip6_fraglist_prepare(skb, &iter);
 
-			skb_set_delivery_time(skb, tstamp, mono_delivery_time);
+			skb_set_delivery_time(skb, tstamp, tstamp_type);
 			err = output(net, sk, skb);
 			if (!err)
 				IP6_INC_STATS(net, ip6_dst_idev(&rt->dst),
@@ -1016,7 +1016,7 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 		/*
 		 *	Put this fragment into the sending queue.
 		 */
-		skb_set_delivery_time(frag, tstamp, mono_delivery_time);
+		skb_set_delivery_time(frag, tstamp, tstamp_type);
 		err = output(net, sk, frag);
 		if (err)
 			goto fail;
diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
index 53d255838e6a..61f28b9a2d34 100644
--- a/net/ipv6/netfilter.c
+++ b/net/ipv6/netfilter.c
@@ -126,7 +126,7 @@ int br_ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 				  struct sk_buff *))
 {
 	int frag_max_size = BR_INPUT_SKB_CB(skb)->frag_max_size;
-	bool mono_delivery_time = skb->mono_delivery_time;
+	bool tstamp_type = skb->tstamp_type;
 	ktime_t tstamp = skb->tstamp;
 	struct ip6_frag_state state;
 	u8 *prevhdr, nexthdr = 0;
@@ -192,7 +192,7 @@ int br_ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 			if (iter.frag)
 				ip6_fraglist_prepare(skb, &iter);
 
-			skb_set_delivery_time(skb, tstamp, mono_delivery_time);
+			skb_set_delivery_time(skb, tstamp, tstamp_type);
 			err = output(net, sk, data, skb);
 			if (err || !iter.frag)
 				break;
@@ -225,7 +225,7 @@ int br_ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 			goto blackhole;
 		}
 
-		skb_set_delivery_time(skb2, tstamp, mono_delivery_time);
+		skb_set_delivery_time(skb2, tstamp, tstamp_type);
 		err = output(net, sk, data, skb2);
 		if (err)
 			goto blackhole;
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 1a51a44571c3..6376555de1ae 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -264,7 +264,7 @@ static int nf_ct_frag6_queue(struct frag_queue *fq, struct sk_buff *skb,
 		fq->iif = dev->ifindex;
 
 	fq->q.stamp = skb->tstamp;
-	fq->q.mono_delivery_time = skb->mono_delivery_time;
+	fq->q.tstamp_type = skb->tstamp_type;
 	fq->q.meat += skb->len;
 	fq->ecn |= ecn;
 	if (payload_len > fq->q.max_size)
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index acb4f119e11f..ea724ff558b4 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -198,7 +198,7 @@ static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
 		fq->iif = dev->ifindex;
 
 	fq->q.stamp = skb->tstamp;
-	fq->q.mono_delivery_time = skb->mono_delivery_time;
+	fq->q.tstamp_type = skb->tstamp_type;
 	fq->q.meat += skb->len;
 	fq->ecn |= ecn;
 	add_frag_mem_limit(fq->q.fqdir, skb->truesize);
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 0e3cf11ae5fc..d62edd36b455 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -54,8 +54,8 @@ TC_INDIRECT_SCOPE int tcf_bpf_act(struct sk_buff *skb,
 		bpf_compute_data_pointers(skb);
 		filter_res = bpf_prog_run(filter, skb);
 	}
-	if (unlikely(!skb->tstamp && skb->mono_delivery_time))
-		skb->mono_delivery_time = 0;
+	if (unlikely(!skb->tstamp && skb->tstamp_type))
+		skb->tstamp_type = 0;
 	if (skb_sk_is_prefetched(skb) && filter_res != TC_ACT_OK)
 		skb_orphan(skb);
 
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 5e83e890f6a4..f9cb4378c754 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -104,8 +104,8 @@ TC_INDIRECT_SCOPE int cls_bpf_classify(struct sk_buff *skb,
 			bpf_compute_data_pointers(skb);
 			filter_res = bpf_prog_run(prog->filter, skb);
 		}
-		if (unlikely(!skb->tstamp && skb->mono_delivery_time))
-			skb->mono_delivery_time = 0;
+		if (unlikely(!skb->tstamp && skb->tstamp_type))
+			skb->tstamp_type = 0;
 
 		if (prog->exts_integrated) {
 			res->class   = 0;
-- 
2.25.1

Re: [RFC PATCH bpf-next v1 1/3] net: Rename mono_delivery_time to tstamp_type for scalabilty

[Willem de Bruijn]

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.
> 
> Bridge driver today has no support to forward the userspace timestamp
> packets and ends up resetting the timestamp. ETF qdisc checks the
> packet coming from userspace and encounters to be 0 thereby dropping
> time sensitive packets. These changes will allow userspace timestamps
> packets to be forwarded from the bridge to NIC drivers.
> 
> 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>
> ---
>  include/linux/skbuff.h                     | 18 +++++++++---------
>  include/net/inet_frag.h                    |  4 ++--
>  net/bridge/netfilter/nf_conntrack_bridge.c |  6 +++---
>  net/core/dev.c                             |  2 +-
>  net/core/filter.c                          |  8 ++++----
>  net/ipv4/inet_fragment.c                   |  2 +-
>  net/ipv4/ip_fragment.c                     |  2 +-
>  net/ipv4/ip_output.c                       |  8 ++++----
>  net/ipv6/ip6_output.c                      |  6 +++---
>  net/ipv6/netfilter.c                       |  6 +++---
>  net/ipv6/netfilter/nf_conntrack_reasm.c    |  2 +-
>  net/ipv6/reassembly.c                      |  2 +-
>  net/sched/act_bpf.c                        |  4 ++--
>  net/sched/cls_bpf.c                        |  4 ++--
>  14 files changed, 37 insertions(+), 37 deletions(-)

Since the next patch against touches many of the same lines, probably
can just squash the two.

Re: [RFC PATCH bpf-next v1 1/3] net: Rename mono_delivery_time to tstamp_type for scalabilty

[Abhishek Chauhan (ABC)]

On 4/10/2024 8:30 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.
>>
>> Bridge driver today has no support to forward the userspace timestamp
>> packets and ends up resetting the timestamp. ETF qdisc checks the
>> packet coming from userspace and encounters to be 0 thereby dropping
>> time sensitive packets. These changes will allow userspace timestamps
>> packets to be forwarded from the bridge to NIC drivers.
>>
>> 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>
>> ---
>>  include/linux/skbuff.h                     | 18 +++++++++---------
>>  include/net/inet_frag.h                    |  4 ++--
>>  net/bridge/netfilter/nf_conntrack_bridge.c |  6 +++---
>>  net/core/dev.c                             |  2 +-
>>  net/core/filter.c                          |  8 ++++----
>>  net/ipv4/inet_fragment.c                   |  2 +-
>>  net/ipv4/ip_fragment.c                     |  2 +-
>>  net/ipv4/ip_output.c                       |  8 ++++----
>>  net/ipv6/ip6_output.c                      |  6 +++---
>>  net/ipv6/netfilter.c                       |  6 +++---
>>  net/ipv6/netfilter/nf_conntrack_reasm.c    |  2 +-
>>  net/ipv6/reassembly.c                      |  2 +-
>>  net/sched/act_bpf.c                        |  4 ++--
>>  net/sched/cls_bpf.c                        |  4 ++--
>>  14 files changed, 37 insertions(+), 37 deletions(-)
> 
> Since the next patch against touches many of the same lines, probably
> can just squash the two.

Sounds good i can do that. 
Make only 2 patches 
1. rename + assign tstamp_type
2. introduce another bit for clock_id base time 

[RFC PATCH bpf-next v1 2/3] net: assign enum to skb->tstamp_type to distinguish between tstamp

[Abhishek Chauhan]

As we are renaming the mono_delivery_time to tstamp_type, it makes
sense to start assigning tstamp_type based out if enum defined as
part of 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 enum to distinguish between mono and real time.

Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
---
 include/linux/skbuff.h                     | 13 +++++++++----
 net/bridge/netfilter/nf_conntrack_bridge.c |  2 +-
 net/core/dev.c                             |  2 +-
 net/core/filter.c                          |  4 ++--
 net/ipv4/ip_output.c                       |  2 +-
 net/ipv4/tcp_output.c                      | 14 +++++++-------
 net/ipv6/ip6_output.c                      |  2 +-
 net/ipv6/tcp_ipv6.c                        |  2 +-
 net/sched/act_bpf.c                        |  2 +-
 net/sched/cls_bpf.c                        |  2 +-
 10 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8210d699d8e9..6160185f0fe0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -701,6 +701,11 @@ typedef unsigned int sk_buff_data_t;
 #else
 typedef unsigned char *sk_buff_data_t;
 #endif
+


+enum skb_tstamp_type {
+	SKB_TSTAMP_TYPE_RX_REAL = 0,    /* A RX (receive) time in real */
+	SKB_TSTAMP_TYPE_TX_MONO = 1,    /* A TX (delivery) time in mono */
+};
 
 /**
  * DOC: Basic sk_buff geometry
@@ -4257,7 +4262,7 @@ static inline void skb_get_new_timestampns(const struct sk_buff *skb,
 static inline void __net_timestamp(struct sk_buff *skb)
 {
 	skb->tstamp = ktime_get_real();
-	skb->tstamp_type = 0;
+	skb->tstamp_type = SKB_TSTAMP_TYPE_RX_REAL;
 }
 
 static inline ktime_t net_timedelta(ktime_t t)
@@ -4266,10 +4271,10 @@ static inline ktime_t net_timedelta(ktime_t t)
 }
 
 static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
-					 bool mono)
+					enum skb_tstamp_type tstamp_type)
 {
 	skb->tstamp = kt;
-	skb->tstamp_type = kt && mono;
+	skb->tstamp_type = kt && tstamp_type;
 }
 
 DECLARE_STATIC_KEY_FALSE(netstamp_needed_key);
@@ -4280,7 +4285,7 @@ DECLARE_STATIC_KEY_FALSE(netstamp_needed_key);
 static inline void skb_clear_delivery_time(struct sk_buff *skb)
 {
 	if (skb->tstamp_type) {
-		skb->tstamp_type = 0;
+		skb->tstamp_type = SKB_TSTAMP_TYPE_RX_REAL;
 		if (static_branch_unlikely(&netstamp_needed_key))
 			skb->tstamp = ktime_get_real();
 		else
diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index 989435bd1690..b970ab2279cf 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -32,7 +32,7 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
 					   struct sk_buff *))
 {
 	int frag_max_size = BR_INPUT_SKB_CB(skb)->frag_max_size;
-	bool tstamp_type = skb->tstamp_type;
+	u8 tstamp_type = skb->tstamp_type;
 	unsigned int hlen, ll_rs, mtu;
 	ktime_t tstamp = skb->tstamp;
 	struct ip_frag_state state;
diff --git a/net/core/dev.c b/net/core/dev.c
index 8b88f8118052..9a84156fab3c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2113,7 +2113,7 @@ EXPORT_SYMBOL(net_disable_timestamp);
 static inline void net_timestamp_set(struct sk_buff *skb)
 {
 	skb->tstamp = 0;
-	skb->tstamp_type = 0;
+	skb->tstamp_type = SKB_TSTAMP_TYPE_RX_REAL;
 	if (static_branch_unlikely(&netstamp_needed_key))
 		skb->tstamp = ktime_get_real();
 }
diff --git a/net/core/filter.c b/net/core/filter.c
index 0f535defdd2c..1c943a165c30 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7698,13 +7698,13 @@ BPF_CALL_3(bpf_skb_set_tstamp, struct sk_buff *, skb,
 		if (!tstamp)
 			return -EINVAL;
 		skb->tstamp = tstamp;
-		skb->tstamp_type = 1;
+		skb->tstamp_type = SKB_TSTAMP_TYPE_TX_MONO;
 		break;
 	case BPF_SKB_TSTAMP_UNSPEC:
 		if (tstamp)
 			return -EINVAL;
 		skb->tstamp = 0;
-		skb->tstamp_type = 0;
+		skb->tstamp_type = SKB_TSTAMP_TYPE_RX_REAL;
 		break;
 	default:
 		return -EINVAL;
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index e8ec7e8ae2e0..62e457f7c02c 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -764,7 +764,7 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 {
 	struct iphdr *iph;
 	struct sk_buff *skb2;
-	bool tstamp_type = skb->tstamp_type;
+	u8 tstamp_type = skb->tstamp_type;
 	struct rtable *rt = skb_rtable(skb);
 	unsigned int mtu, hlen, ll_rs;
 	struct ip_fraglist_iter iter;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e3167ad96567..071fe377747a 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1297,7 +1297,7 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb,
 	tp = tcp_sk(sk);
 	prior_wstamp = tp->tcp_wstamp_ns;
 	tp->tcp_wstamp_ns = max(tp->tcp_wstamp_ns, tp->tcp_clock_cache);
-	skb_set_delivery_time(skb, tp->tcp_wstamp_ns, true);
+	skb_set_delivery_time(skb, tp->tcp_wstamp_ns, SKB_TSTAMP_TYPE_TX_MONO);
 	if (clone_it) {
 		oskb = skb;
 
@@ -1647,7 +1647,7 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
 
 	skb_split(skb, buff, len);
 
-	skb_set_delivery_time(buff, skb->tstamp, true);
+	skb_set_delivery_time(buff, skb->tstamp, SKB_TSTAMP_TYPE_TX_MONO);
 	tcp_fragment_tstamp(skb, buff);
 
 	old_factor = tcp_skb_pcount(skb);
@@ -2728,7 +2728,7 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
 		if (unlikely(tp->repair) && tp->repair_queue == TCP_SEND_QUEUE) {
 			/* "skb_mstamp_ns" is used as a start point for the retransmit timer */
 			tp->tcp_wstamp_ns = tp->tcp_clock_cache;
-			skb_set_delivery_time(skb, tp->tcp_wstamp_ns, true);
+			skb_set_delivery_time(skb, tp->tcp_wstamp_ns, SKB_TSTAMP_TYPE_TX_MONO);
 			list_move_tail(&skb->tcp_tsorted_anchor, &tp->tsorted_sent_queue);
 			tcp_init_tso_segs(skb, mss_now);
 			goto repair; /* Skip network transmission */
@@ -3711,11 +3711,11 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
 #ifdef CONFIG_SYN_COOKIES
 	if (unlikely(synack_type == TCP_SYNACK_COOKIE && ireq->tstamp_ok))
 		skb_set_delivery_time(skb, cookie_init_timestamp(req, now),
-				      true);
+				      SKB_TSTAMP_TYPE_TX_MONO);
 	else
 #endif
 	{
-		skb_set_delivery_time(skb, now, true);
+		skb_set_delivery_time(skb, now, SKB_TSTAMP_TYPE_TX_MONO);
 		if (!tcp_rsk(req)->snt_synack) /* Timestamp first SYNACK */
 			tcp_rsk(req)->snt_synack = tcp_skb_timestamp_us(skb);
 	}
@@ -3802,7 +3802,7 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
 	bpf_skops_write_hdr_opt((struct sock *)sk, skb, req, syn_skb,
 				synack_type, &opts);
 
-	skb_set_delivery_time(skb, now, true);
+	skb_set_delivery_time(skb, now, SKB_TSTAMP_TYPE_TX_MONO);
 	tcp_add_tx_delay(skb, tp);
 
 	return skb;
@@ -3986,7 +3986,7 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
 
 	err = tcp_transmit_skb(sk, syn_data, 1, sk->sk_allocation);
 
-	skb_set_delivery_time(syn, syn_data->skb_mstamp_ns, true);
+	skb_set_delivery_time(syn, syn_data->skb_mstamp_ns, SKB_TSTAMP_TYPE_TX_MONO);
 
 	/* Now full SYN+DATA was cloned and sent (or not),
 	 * remove the SYN from the original skb (syn_data)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 61ddc9549160..a9e819115622 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -859,7 +859,7 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 	struct rt6_info *rt = (struct rt6_info *)skb_dst(skb);
 	struct ipv6_pinfo *np = skb->sk && !dev_recursion_level() ?
 				inet6_sk(skb->sk) : NULL;
-	bool tstamp_type = skb->tstamp_type;
+	u8 tstamp_type = skb->tstamp_type;
 	struct ip6_frag_state state;
 	unsigned int mtu, hlen, nexthdr_offset;
 	ktime_t tstamp = skb->tstamp;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 3f4cba49e9ee..a9bf9c630582 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -973,7 +973,7 @@ static void tcp_v6_send_response(const struct sock *sk, struct sk_buff *skb, u32
 			mark = inet_twsk(sk)->tw_mark;
 		else
 			mark = READ_ONCE(sk->sk_mark);
-		skb_set_delivery_time(buff, tcp_transmit_time(sk), true);
+		skb_set_delivery_time(buff, tcp_transmit_time(sk), SKB_TSTAMP_TYPE_TX_MONO);
 	}
 	if (txhash) {
 		/* autoflowlabel/skb_get_hash_flowi6 rely on buff->hash */
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index d62edd36b455..6f64e867a5e9 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -55,7 +55,7 @@ TC_INDIRECT_SCOPE int tcf_bpf_act(struct sk_buff *skb,
 		filter_res = bpf_prog_run(filter, skb);
 	}
 	if (unlikely(!skb->tstamp && skb->tstamp_type))
-		skb->tstamp_type = 0;
+		skb->tstamp_type = SKB_TSTAMP_TYPE_RX_REAL;
 	if (skb_sk_is_prefetched(skb) && filter_res != TC_ACT_OK)
 		skb_orphan(skb);
 
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index f9cb4378c754..7ee73618c438 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -105,7 +105,7 @@ TC_INDIRECT_SCOPE int cls_bpf_classify(struct sk_buff *skb,
 			filter_res = bpf_prog_run(prog->filter, skb);
 		}
 		if (unlikely(!skb->tstamp && skb->tstamp_type))
-			skb->tstamp_type = 0;
+			skb->tstamp_type = SKB_TSTAMP_TYPE_RX_REAL;

 		if (prog->exts_integrated) {
 			res->class   = 0;
-- 
2.25.1

Re: [RFC PATCH bpf-next v1 2/3] net: assign enum to skb->tstamp_type to distinguish between tstamp

[Willem de Bruijn]

Abhishek Chauhan wrote:
> As we are renaming the mono_delivery_time to tstamp_type, it makes
> sense to start assigning tstamp_type based out if enum defined as
> part of 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 enum to distinguish between mono and real time.
> 
> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> ---
>  include/linux/skbuff.h                     | 13 +++++++++----
>  net/bridge/netfilter/nf_conntrack_bridge.c |  2 +-
>  net/core/dev.c                             |  2 +-
>  net/core/filter.c                          |  4 ++--
>  net/ipv4/ip_output.c                       |  2 +-
>  net/ipv4/tcp_output.c                      | 14 +++++++-------
>  net/ipv6/ip6_output.c                      |  2 +-
>  net/ipv6/tcp_ipv6.c                        |  2 +-
>  net/sched/act_bpf.c                        |  2 +-
>  net/sched/cls_bpf.c                        |  2 +-
>  10 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 8210d699d8e9..6160185f0fe0 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -701,6 +701,11 @@ typedef unsigned int sk_buff_data_t;
>  #else
>  typedef unsigned char *sk_buff_data_t;
>  #endif
> +
> 
> 
> +enum skb_tstamp_type {
> +	SKB_TSTAMP_TYPE_RX_REAL = 0,    /* A RX (receive) time in real */
> +	SKB_TSTAMP_TYPE_TX_MONO = 1,    /* A TX (delivery) time in mono */
> +};

I'd drop the RX_/TX_. This is just a version of clockid_t, compressed
to minimize space taken in sk_buff. Simpler to keep to the CLOCK_..
types. Where a clock was set (TX vs RX) is not relevant to the code
that later references skb->tstamp.

>  static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
> -					 bool mono)
> +					enum skb_tstamp_type tstamp_type)
>  {
>  	skb->tstamp = kt;
> -	skb->tstamp_type = kt && mono;
> +	skb->tstamp_type = kt && tstamp_type;

Already introduce a switch here?

Re: [RFC PATCH bpf-next v1 2/3] net: assign enum to skb->tstamp_type to distinguish between tstamp

[Abhishek Chauhan (ABC)]

On 4/10/2024 8:35 AM, Willem de Bruijn wrote:
> Abhishek Chauhan wrote:
>> As we are renaming the mono_delivery_time to tstamp_type, it makes
>> sense to start assigning tstamp_type based out if enum defined as
>> part of 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 enum to distinguish between mono and real time.
>>
>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>> ---
>>  include/linux/skbuff.h                     | 13 +++++++++----
>>  net/bridge/netfilter/nf_conntrack_bridge.c |  2 +-
>>  net/core/dev.c                             |  2 +-
>>  net/core/filter.c                          |  4 ++--
>>  net/ipv4/ip_output.c                       |  2 +-
>>  net/ipv4/tcp_output.c                      | 14 +++++++-------
>>  net/ipv6/ip6_output.c                      |  2 +-
>>  net/ipv6/tcp_ipv6.c                        |  2 +-
>>  net/sched/act_bpf.c                        |  2 +-
>>  net/sched/cls_bpf.c                        |  2 +-
>>  10 files changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 8210d699d8e9..6160185f0fe0 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -701,6 +701,11 @@ typedef unsigned int sk_buff_data_t;
>>  #else
>>  typedef unsigned char *sk_buff_data_t;
>>  #endif
>> +
>>
>>
>> +enum skb_tstamp_type {
>> +	SKB_TSTAMP_TYPE_RX_REAL = 0,    /* A RX (receive) time in real */
>> +	SKB_TSTAMP_TYPE_TX_MONO = 1,    /* A TX (delivery) time in mono */
>> +};
> 
> I'd drop the RX_/TX_. This is just a version of clockid_t, compressed
> to minimize space taken in sk_buff. Simpler to keep to the CLOCK_..
> types. Where a clock was set (TX vs RX) is not relevant to the code
> that later references skb->tstamp.
> 
Make sense. tstamp can be either mono, real , tai ... etc . Directionality doesnt matter
Let me check this and update. 

>>  static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
>> -					 bool mono)
>> +					enum skb_tstamp_type tstamp_type)
>>  {
>>  	skb->tstamp = kt;
>> -	skb->tstamp_type = kt && mono;
>> +	skb->tstamp_type = kt && tstamp_type;
> 
> Already introduce a switch here?
> 
I will introduce a switch here based on tstamp_type passed. 

> 

[RFC PATCH bpf-next v1 3/3] net: Add additional bit to support userspace timestamp type

[Abhishek Chauhan]

tstamp_type can be real, mono or userspace timestamp.

This commit adds userspace timestamp and sets it if there is
valid transmit_time available in socket coming from userspace.

To make the design scalable for future needs this commit bring in
the change to extend the tstamp_type:1 to tstamp_type:2 to support
userspace timestamp.

Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
---
 include/linux/skbuff.h | 19 +++++++++++++++++--
 net/ipv4/ip_output.c   |  2 +-
 net/ipv4/raw.c         |  2 +-
 net/ipv6/ip6_output.c  |  2 +-
 net/ipv6/raw.c         |  2 +-
 net/packet/af_packet.c |  6 +++---
 6 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6160185f0fe0..2f91a8a2157a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -705,6 +705,9 @@ typedef unsigned char *sk_buff_data_t;
 enum skb_tstamp_type {
 	SKB_TSTAMP_TYPE_RX_REAL = 0,    /* A RX (receive) time in real */
 	SKB_TSTAMP_TYPE_TX_MONO = 1,    /* A TX (delivery) time in mono */
+	SKB_TSTAMP_TYPE_TX_USER = 2,    /* A TX (delivery) time and its clock
+									 * is in skb->sk->sk_clockid.
+									 */
 };
 
 /**
@@ -830,6 +833,9 @@ enum skb_tstamp_type {
  *		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 in mono clock base (i.e., EDT) or a clock base chosen
+ *		by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
+ *		ingress.
  *	@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.
@@ -960,7 +966,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;
@@ -4274,7 +4280,16 @@ static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
 					enum skb_tstamp_type tstamp_type)
 {
 	skb->tstamp = kt;
-	skb->tstamp_type = kt && tstamp_type;
+
+	if (skb->tstamp_type)
+		return;
+
+	if (kt && tstamp_type == SKB_TSTAMP_TYPE_TX_MONO)
+		skb->tstamp_type = SKB_TSTAMP_TYPE_TX_MONO;
+
+	if (kt && tstamp_type == SKB_TSTAMP_TYPE_TX_USER)
+		skb->tstamp_type = SKB_TSTAMP_TYPE_TX_USER;
+
 }
 
 DECLARE_STATIC_KEY_FALSE(netstamp_needed_key);
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 62e457f7c02c..9aea6e810f52 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1457,7 +1457,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
 
 	skb->priority = (cork->tos != -1) ? cork->priority: READ_ONCE(sk->sk_priority);
 	skb->mark = cork->mark;
-	skb->tstamp = cork->transmit_time;
+	skb_set_delivery_time(skb, cork->transmit_time, SKB_TSTAMP_TYPE_TX_USER);
 	/*
 	 * Steal rt from cork.dst to avoid a pair of atomic_inc/atomic_dec
 	 * on dst refcount
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index dcb11f22cbf2..d8f52bc06ed3 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -360,7 +360,7 @@ static int raw_send_hdrinc(struct sock *sk, struct flowi4 *fl4,
 	skb->protocol = htons(ETH_P_IP);
 	skb->priority = READ_ONCE(sk->sk_priority);
 	skb->mark = sockc->mark;
-	skb->tstamp = sockc->transmit_time;
+	skb_set_delivery_time(skb, sockc->transmit_time, SKB_TSTAMP_TYPE_TX_USER);
 	skb_dst_set(skb, &rt->dst);
 	*rtp = NULL;
 
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a9e819115622..2beb9fc8c0b1 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1924,7 +1924,7 @@ struct sk_buff *__ip6_make_skb(struct sock *sk,
 
 	skb->priority = READ_ONCE(sk->sk_priority);
 	skb->mark = cork->base.mark;
-	skb->tstamp = cork->base.transmit_time;
+	skb_set_delivery_time(skb, cork->base.transmit_time, SKB_TSTAMP_TYPE_TX_USER);
 
 	ip6_cork_steal_dst(skb, cork);
 	IP6_INC_STATS(net, rt->rt6i_idev, IPSTATS_MIB_OUTREQUESTS);
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 0d896ca7b589..3a68ca80bf83 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -621,7 +621,7 @@ static int rawv6_send_hdrinc(struct sock *sk, struct msghdr *msg, int length,
 	skb->protocol = htons(ETH_P_IPV6);
 	skb->priority = READ_ONCE(sk->sk_priority);
 	skb->mark = sockc->mark;
-	skb->tstamp = sockc->transmit_time;
+	skb_set_delivery_time(skb, sockc->transmit_time, SKB_TSTAMP_TYPE_TX_USER);
 
 	skb_put(skb, length);
 	skb_reset_network_header(skb);
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 18f616f487ea..27ea972dfc56 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2056,7 +2056,7 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
 	skb->dev = dev;
 	skb->priority = READ_ONCE(sk->sk_priority);
 	skb->mark = READ_ONCE(sk->sk_mark);
-	skb->tstamp = sockc.transmit_time;
+	skb_set_delivery_time(skb, sockc.transmit_time, SKB_TSTAMP_TYPE_TX_USER);
 
 	skb_setup_tx_timestamp(skb, sockc.tsflags);
 
@@ -2585,7 +2585,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 	skb->dev = dev;
 	skb->priority = READ_ONCE(po->sk.sk_priority);
 	skb->mark = READ_ONCE(po->sk.sk_mark);
-	skb->tstamp = sockc->transmit_time;
+	skb_set_delivery_time(skb, sockc->transmit_time, SKB_TSTAMP_TYPE_TX_USER);
 	skb_setup_tx_timestamp(skb, sockc->tsflags);
 	skb_zcopy_set_nouarg(skb, ph.raw);
 
@@ -3063,7 +3063,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	skb->dev = dev;
 	skb->priority = READ_ONCE(sk->sk_priority);
 	skb->mark = sockc.mark;
-	skb->tstamp = sockc.transmit_time;
+	skb_set_delivery_time(skb, sockc.transmit_time, SKB_TSTAMP_TYPE_TX_USER);
 
 	if (unlikely(extra_len == 4))
 		skb->no_fcs = 1;
-- 
2.25.1

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

[Willem de Bruijn]

Abhishek Chauhan wrote:
> tstamp_type can be real, mono or userspace timestamp.
> 
> This commit adds userspace timestamp and sets it if there is
> valid transmit_time available in socket coming from userspace.
> 
> To make the design scalable for future needs this commit bring in
> the change to extend the tstamp_type:1 to tstamp_type:2 to support
> userspace timestamp.
> 
> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> ---
>  include/linux/skbuff.h | 19 +++++++++++++++++--
>  net/ipv4/ip_output.c   |  2 +-
>  net/ipv4/raw.c         |  2 +-
>  net/ipv6/ip6_output.c  |  2 +-
>  net/ipv6/raw.c         |  2 +-
>  net/packet/af_packet.c |  6 +++---
>  6 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6160185f0fe0..2f91a8a2157a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -705,6 +705,9 @@ typedef unsigned char *sk_buff_data_t;
>  enum skb_tstamp_type {
>  	SKB_TSTAMP_TYPE_RX_REAL = 0,    /* A RX (receive) time in real */
>  	SKB_TSTAMP_TYPE_TX_MONO = 1,    /* A TX (delivery) time in mono */
> +	SKB_TSTAMP_TYPE_TX_USER = 2,    /* A TX (delivery) time and its clock
> +									 * is in skb->sk->sk_clockid.
> +									 */

Weird indentation?

More fundamentally: instead of defining a type TX_USER, can we use a
real clockid (e.g., CLOCK_TAI) based on skb->sk->sk_clockid? Rather
than store an id that means "go look at sk_clockid".

>  };
>  
>  /**
> @@ -830,6 +833,9 @@ enum skb_tstamp_type {
>   *		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 in mono clock base (i.e., EDT) or a clock base chosen
> + *		by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
> + *		ingress.
>   *	@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.
> @@ -960,7 +966,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;

With pahole, does this have an effect on sk_buff layout?

> @@ -4274,7 +4280,16 @@ static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
>  					enum skb_tstamp_type tstamp_type)
>  {
>  	skb->tstamp = kt;
> -	skb->tstamp_type = kt && tstamp_type;
> +
> +	if (skb->tstamp_type)
> +		return;
> +

Why bail if a type is already set? And what if
skb->tstamp_type != tstamp_type? Should skb->tstamp then not be
written to (i.e., the test moved up), and perhaps a rate limited
warning.

> +	if (kt && tstamp_type == SKB_TSTAMP_TYPE_TX_MONO)
> +		skb->tstamp_type = SKB_TSTAMP_TYPE_TX_MONO;
> +
> +	if (kt && tstamp_type == SKB_TSTAMP_TYPE_TX_USER)
> +		skb->tstamp_type = SKB_TSTAMP_TYPE_TX_USER;

Simpler

    if (kt)
        skb->tstamp_type = tstamp_type;

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

[Abhishek Chauhan (ABC)]

On 4/10/2024 8:42 AM, Willem de Bruijn wrote:
> Abhishek Chauhan wrote:
>> tstamp_type can be real, mono or userspace timestamp.
>>
>> This commit adds userspace timestamp and sets it if there is
>> valid transmit_time available in socket coming from userspace.
>>
>> To make the design scalable for future needs this commit bring in
>> the change to extend the tstamp_type:1 to tstamp_type:2 to support
>> userspace timestamp.
>>
>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>> ---
>>  include/linux/skbuff.h | 19 +++++++++++++++++--
>>  net/ipv4/ip_output.c   |  2 +-
>>  net/ipv4/raw.c         |  2 +-
>>  net/ipv6/ip6_output.c  |  2 +-
>>  net/ipv6/raw.c         |  2 +-
>>  net/packet/af_packet.c |  6 +++---
>>  6 files changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 6160185f0fe0..2f91a8a2157a 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -705,6 +705,9 @@ typedef unsigned char *sk_buff_data_t;
>>  enum skb_tstamp_type {
>>  	SKB_TSTAMP_TYPE_RX_REAL = 0,    /* A RX (receive) time in real */
>>  	SKB_TSTAMP_TYPE_TX_MONO = 1,    /* A TX (delivery) time in mono */
>> +	SKB_TSTAMP_TYPE_TX_USER = 2,    /* A TX (delivery) time and its clock
>> +									 * is in skb->sk->sk_clockid.
>> +									 */
> 
> Weird indentation?
> 
I will correct it. 

> More fundamentally: instead of defining a type TX_USER, can we use a
> real clockid (e.g., CLOCK_TAI) based on skb->sk->sk_clockid? Rather
> than store an id that means "go look at sk_clockid".
> 
>>  };
>>  
>>  /**
>> @@ -830,6 +833,9 @@ enum skb_tstamp_type {
>>   *		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 in mono clock base (i.e., EDT) or a clock base chosen
>> + *		by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
>> + *		ingress.
>>   *	@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.
>> @@ -960,7 +966,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;
> 
> With pahole, does this have an effect on sk_buff layout?
> 
I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC. 
That being said i am actually trying to understand/learn BPF instructions to know things better. 
I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK


#ifdef __BIG_ENDIAN_BITFIELD
#define SKB_MONO_DELIVERY_TIME_MASK	(1 << 7) //Suspecting changes here too
#define TC_AT_INGRESS_MASK		(1 << 6) // and here 
#else
#define SKB_MONO_DELIVERY_TIME_MASK	(1 << 0)
#define TC_AT_INGRESS_MASK		(1 << 1) (this might have to change to 1<<2 )
#endif
#define SKB_BF_MONO_TC_OFFSET		offsetof(struct sk_buff, __mono_tc_offset)

Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c 
I am trying to figure out what this part of the code is doing.
https://lore.kernel.org/all/20230321014115.997841-4-kuba@kernel.org/

Please correct me if i am wrong here. 

>> @@ -4274,7 +4280,16 @@ static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
>>  					enum skb_tstamp_type tstamp_type)
>>  {
>>  	skb->tstamp = kt;
>> -	skb->tstamp_type = kt && tstamp_type;
>> +
>> +	if (skb->tstamp_type)
>> +		return;
>> +
> 
I can put a warn on here incase if both MONO and TAI are set. 
OR 
Rather make it simple as you have mentioned below. 
> Why bail if a type is already set? And what if
> skb->tstamp_type != tstamp_type? Should skb->tstamp then not be
> written to (i.e., the test moved up), and perhaps a rate limited
> warning.
> 
>> +	if (kt && tstamp_type == SKB_TSTAMP_TYPE_TX_MONO)
>> +		skb->tstamp_type = SKB_TSTAMP_TYPE_TX_MONO;
>> +
>> +	if (kt && tstamp_type == SKB_TSTAMP_TYPE_TX_USER)
>> +		skb->tstamp_type = SKB_TSTAMP_TYPE_TX_USER;
> 
> Simpler
> 
>     if (kt)
>         skb->tstamp_type = tstamp_type;

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

[Willem de Bruijn]

Abhishek Chauhan (ABC) wrote:
> 
> 
> On 4/10/2024 8:42 AM, Willem de Bruijn wrote:
> > Abhishek Chauhan wrote:
> >> tstamp_type can be real, mono or userspace timestamp.
> >>
> >> This commit adds userspace timestamp and sets it if there is
> >> valid transmit_time available in socket coming from userspace.
> >>
> >> To make the design scalable for future needs this commit bring in
> >> the change to extend the tstamp_type:1 to tstamp_type:2 to support
> >> userspace timestamp.
> >>
> >> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
> >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>

> > With pahole, does this have an effect on sk_buff layout?
> > 
> I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
> changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC. 
> That being said i am actually trying to understand/learn BPF instructions to know things better. 
> I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK
> 
> 
> #ifdef __BIG_ENDIAN_BITFIELD
> #define SKB_MONO_DELIVERY_TIME_MASK	(1 << 7) //Suspecting changes here too
> #define TC_AT_INGRESS_MASK		(1 << 6) // and here 
> #else
> #define SKB_MONO_DELIVERY_TIME_MASK	(1 << 0)
> #define TC_AT_INGRESS_MASK		(1 << 1) (this might have to change to 1<<2 )
> #endif
> #define SKB_BF_MONO_TC_OFFSET		offsetof(struct sk_buff, __mono_tc_offset)
> 
> Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c 
> I am trying to figure out what this part of the code is doing.
> https://lore.kernel.org/all/20230321014115.997841-4-kuba@kernel.org/
> 
> Please correct me if i am wrong here.

I broadly agree. We should convert all references to
SKB_MONO_DELIVERY_TIME_MASK to an skb_tstamp_type equivalent.

> 
> >> @@ -4274,7 +4280,16 @@ static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
> >>  					enum skb_tstamp_type tstamp_type)
> >>  {
> >>  	skb->tstamp = kt;
> >> -	skb->tstamp_type = kt && tstamp_type;
> >> +
> >> +	if (skb->tstamp_type)
> >> +		return;
> >> +
> > 
> I can put a warn on here incase if both MONO and TAI are set. 
> OR 
> Rather make it simple as you have mentioned below.

When might skb->tstamp_type already be non-zero when
skb_set_deliver_type is called?

In most cases, this is called for a fresh skb.

In br_ip6_fragment, it is called with a previous value. But this is
the value of skb->tstamp_type. It just clears it if kt is 0.

If skb->tstamp_type != tstamp_type is not a condition that can be
forced by an unprivileged user, then we can warn.

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

[Martin KaFai Lau]

On 4/10/24 1:25 PM, Abhishek Chauhan (ABC) wrote:
>>> @@ -830,6 +833,9 @@ enum skb_tstamp_type {
>>>    *		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 in mono clock base (i.e., EDT) or a clock base chosen
>>> + *		by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
>>> + *		ingress.
>>>    *	@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.
>>> @@ -960,7 +966,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 */

The above "tstamp_type:2" change shifted the tc_at_ingress bit.
TC_AT_INGRESS_MASK needs to be adjusted.

>>>   	__u8			tc_skip_classify:1;
>>
>> With pahole, does this have an effect on sk_buff layout?
>>
> I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
> changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC.
> That being said i am actually trying to understand/learn BPF instructions to know things better.
> I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK
> 
> 
> #ifdef __BIG_ENDIAN_BITFIELD
> #define SKB_MONO_DELIVERY_TIME_MASK	(1 << 7) //Suspecting changes here too
> #define TC_AT_INGRESS_MASK		(1 << 6) // and here
> #else
> #define SKB_MONO_DELIVERY_TIME_MASK	(1 << 0)
> #define TC_AT_INGRESS_MASK		(1 << 1) (this might have to change to 1<<2 )

This should be (1 << 2) now. Similar adjustment for the big endian.

> #endif
> #define SKB_BF_MONO_TC_OFFSET		offsetof(struct sk_buff, __mono_tc_offset)
> 
> Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c

ctx_rewrite.c tests the bpf ctx rewrite code. In this particular case, it tests
the bpf_convert_tstamp_read() and bpf_convert_tstamp_write() generate the
correct bpf instructions.
e.g. "w11 &= 3;" is testing the following in bpf_convert_tstamp_read():
		*insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
	 				TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);

The existing "TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK" is 0x3
and it should become 0x5 if my hand counts correctly.

The patch set cannot be applied to the bpf-next:
https://patchwork.kernel.org/project/netdevbpf/patch/20240409210547.3815806-4-quic_abchauha@quicinc.com/
, so bpf CI cannot run to reproduce the issue.

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

[Abhishek Chauhan (ABC)]

On 4/10/2024 4:25 PM, Martin KaFai Lau wrote:
> On 4/10/24 1:25 PM, Abhishek Chauhan (ABC) wrote:
>>>> @@ -830,6 +833,9 @@ enum skb_tstamp_type {
>>>>    *        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 in mono clock base (i.e., EDT) or a clock base chosen
>>>> + *        by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
>>>> + *        ingress.
>>>>    *    @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.
>>>> @@ -960,7 +966,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 */
> 
> The above "tstamp_type:2" change shifted the tc_at_ingress bit.
> TC_AT_INGRESS_MASK needs to be adjusted.
> 
>>>>       __u8            tc_skip_classify:1;
>>>
>>> With pahole, does this have an effect on sk_buff layout?
>>>
>> I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
>> changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC.
>> That being said i am actually trying to understand/learn BPF instructions to know things better.
>> I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK
>>
>>
>> #ifdef __BIG_ENDIAN_BITFIELD
>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 7) //Suspecting changes here too
>> #define TC_AT_INGRESS_MASK        (1 << 6) // and here
>> #else
>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 0)
>> #define TC_AT_INGRESS_MASK        (1 << 1) (this might have to change to 1<<2 )
> 
> This should be (1 << 2) now. Similar adjustment for the big endian.
> 
>> #endif
>> #define SKB_BF_MONO_TC_OFFSET        offsetof(struct sk_buff, __mono_tc_offset)
>>
>> Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c
> 
> ctx_rewrite.c tests the bpf ctx rewrite code. In this particular case, it tests
> the bpf_convert_tstamp_read() and bpf_convert_tstamp_write() generate the
> correct bpf instructions.
> e.g. "w11 &= 3;" is testing the following in bpf_convert_tstamp_read():
>         *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
>                      TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);
> 
> The existing "TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK" is 0x3
> and it should become 0x5 if my hand counts correctly.
> 

so the changes will be as follows (Martin correct me if am wrong)

		//w11 is checked againt 0x5 (Binary = 101)
		N(SCHED_CLS, struct __sk_buff, tstamp),
		.read  = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
			 "w11 &= 5;" <== here 
			 "if w11 != 0x5  goto pc+2;" <==here
			 "$dst = 0;"
			 "goto pc+1;"
			 "$dst = *(u64 *)($ctx + sk_buff::tstamp);",

		//w11 is checked againt 0x4 (100) 
		.write = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
			 "if w11 & 0x4 goto pc+1;" <== here
			 "goto pc+2;"
			 "w11 &= -4;" <==here
			 "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;"
			 "*(u64 *)($ctx + sk_buff::tstamp) = $src;",


> The patch set cannot be applied to the bpf-next:
> https://patchwork.kernel.org/project/netdevbpf/patch/20240409210547.3815806-4-quic_abchauha@quicinc.com/
> , so bpf CI cannot run to reproduce the issue.
> 

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

[Abhishek Chauhan (ABC)]

On 4/10/2024 4:39 PM, Abhishek Chauhan (ABC) wrote:
> 
> 
> On 4/10/2024 4:25 PM, Martin KaFai Lau wrote:
>> On 4/10/24 1:25 PM, Abhishek Chauhan (ABC) wrote:
>>>>> @@ -830,6 +833,9 @@ enum skb_tstamp_type {
>>>>>    *        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 in mono clock base (i.e., EDT) or a clock base chosen
>>>>> + *        by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
>>>>> + *        ingress.
>>>>>    *    @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.
>>>>> @@ -960,7 +966,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 */
>>
>> The above "tstamp_type:2" change shifted the tc_at_ingress bit.
>> TC_AT_INGRESS_MASK needs to be adjusted.
>>
>>>>>       __u8            tc_skip_classify:1;
>>>>
>>>> With pahole, does this have an effect on sk_buff layout?
>>>>
>>> I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
>>> changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC.
>>> That being said i am actually trying to understand/learn BPF instructions to know things better.
>>> I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK
>>>
>>>
>>> #ifdef __BIG_ENDIAN_BITFIELD
>>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 7) //Suspecting changes here too
>>> #define TC_AT_INGRESS_MASK        (1 << 6) // and here
>>> #else
>>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 0)
>>> #define TC_AT_INGRESS_MASK        (1 << 1) (this might have to change to 1<<2 )
>>
>> This should be (1 << 2) now. Similar adjustment for the big endian.
>>
>>> #endif
>>> #define SKB_BF_MONO_TC_OFFSET        offsetof(struct sk_buff, __mono_tc_offset)
>>>
>>> Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c
>>
>> ctx_rewrite.c tests the bpf ctx rewrite code. In this particular case, it tests
>> the bpf_convert_tstamp_read() and bpf_convert_tstamp_write() generate the
>> correct bpf instructions.
>> e.g. "w11 &= 3;" is testing the following in bpf_convert_tstamp_read():
>>         *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
>>                      TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);
>>
>> The existing "TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK" is 0x3
>> and it should become 0x5 if my hand counts correctly.
>>
> 
> so the changes will be as follows (Martin correct me if am wrong)
> 
> 		//w11 is checked againt 0x5 (Binary = 101)
> 		N(SCHED_CLS, struct __sk_buff, tstamp),
> 		.read  = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
> 			 "w11 &= 5;" <== here 
> 			 "if w11 != 0x5  goto pc+2;" <==here
> 			 "$dst = 0;"
> 			 "goto pc+1;"
> 			 "$dst = *(u64 *)($ctx + sk_buff::tstamp);",
> 
> 		//w11 is checked againt 0x4 (100) 
> 		.write = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
> 			 "if w11 & 0x4 goto pc+1;" <== here
> 			 "goto pc+2;"
> 			 "w11 &= -4;" <==here
> 			 "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;"
> 			 "*(u64 *)($ctx + sk_buff::tstamp) = $src;",
> 
>
Martin and Willem,
After the above changes, patchset v3 of these changes passed BPF test cases . Looks like we are good to go with final review now. If you have any further comments
Thank you for all the comments and design discussion that we had as part of this patch set series. 

Testing :- 
1. https://patchwork.kernel.org/project/netdevbpf/patch/20240412210125.1780574-2-quic_abchauha@quicinc.com/
2. https://patchwork.kernel.org/project/netdevbpf/patch/20240412210125.1780574-3-quic_abchauha@quicinc.com/
 
>> The patch set cannot be applied to the bpf-next:
>> https://patchwork.kernel.org/project/netdevbpf/patch/20240409210547.3815806-4-quic_abchauha@quicinc.com/
>> , so bpf CI cannot run to reproduce the issue.
>>