[RFC,bpf-next,16/20] trait: Support sk_buffs

Commit Message

Arthur Fabre March 5, 2025, 2:32 p.m. UTC

From: Arthur Fabre <[e-mail]>

Hide the space used by traits from skb_headroom(): that space isn't
actually usable.

Preserve the trait store in pskb_expand_head() by copying it ahead of
the new headroom. The struct xdp_frame at the start of the headroom
isn't needed anymore, so we can overwrite it with traits, and introduce
a new flag to indicate traits are stored at the start of the headroom.

Cloned skbs share the same packet data and headroom as the original skb,
so changes to traits in one would be reflected in the other.
Is that ok?
Are there cases where we would want a clone to have different traits?
For now, prevent clones from using traits.

Signed-off-by: Arthur Fabre <[e-mail]>
---
 include/linux/skbuff.h | 25 +++++++++++++++++++++++--
 net/core/skbuff.c      | 25 +++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

Comments

Lorenzo Bianconi March 10, 2025, 11:45 a.m. UTC | #1

> From: Arthur Fabre <[e-mail]>
> 
> Hide the space used by traits from skb_headroom(): that space isn't
> actually usable.
> 
> Preserve the trait store in pskb_expand_head() by copying it ahead of
> the new headroom. The struct xdp_frame at the start of the headroom
> isn't needed anymore, so we can overwrite it with traits, and introduce
> a new flag to indicate traits are stored at the start of the headroom.
> 
> Cloned skbs share the same packet data and headroom as the original skb,
> so changes to traits in one would be reflected in the other.
> Is that ok?
> Are there cases where we would want a clone to have different traits?
> For now, prevent clones from using traits.
> 
> Signed-off-by: Arthur Fabre <[e-mail]>
> ---
>  include/linux/skbuff.h | 25 +++++++++++++++++++++++--
>  net/core/skbuff.c      | 25 +++++++++++++++++++++++--
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index d7dfee152ebd26ce87a230222e94076aca793adc..886537508789202339c925b5613574de67b7e43c 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -39,6 +39,7 @@
>  #include <net/net_debug.h>
>  #include <net/dropreason-core.h>
>  #include <net/netmem.h>
> +#include <net/trait.h>
>  
>  /**
>   * DOC: skb checksums
> @@ -729,6 +730,8 @@ enum skb_traits_type {
>  	SKB_TRAITS_NONE,
>  	/* Trait store in headroom, offset by sizeof(struct xdp_frame) */
>  	SKB_TRAITS_AFTER_XDP,
> +	/* Trait store at start of headroom */
> +	SKB_TRAITS_AT_HEAD,
>  };
>  
>  /**
> @@ -1029,7 +1032,7 @@ struct sk_buff {
>  	__u8			csum_not_inet:1;
>  #endif
>  	__u8			unreadable:1;
> -	__u8			traits_type:1;	/* See enum skb_traits_type */
> +	__u8			traits_type:2;	/* See enum skb_traits_type */
>  #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS)
>  	__u16			tc_index;	/* traffic control index */
>  #endif
> @@ -2836,6 +2839,18 @@ static inline void *pskb_pull(struct sk_buff *skb, unsigned int len)
>  
>  void skb_condense(struct sk_buff *skb);
>  
> +static inline void *skb_traits(const struct sk_buff *skb)
> +{
> +	switch (skb->traits_type) {
> +	case SKB_TRAITS_AFTER_XDP:
> +		return skb->head + _XDP_FRAME_SIZE;
> +	case SKB_TRAITS_AT_HEAD:
> +		return skb->head;
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  /**
>   *	skb_headroom - bytes at buffer head
>   *	@skb: buffer to check
> @@ -2844,7 +2859,13 @@ void skb_condense(struct sk_buff *skb);
>   */
>  static inline unsigned int skb_headroom(const struct sk_buff *skb)
>  {
> -	return skb->data - skb->head;
> +	int trait_size = 0;
> +	void *traits = skb_traits(skb);
> +
> +	if (traits)
> +		trait_size = traits_size(traits);
> +
> +	return skb->data - skb->head - trait_size;

I am not fully aware of all possible use-cases, but do we really need to
store hw medata traits (e.g. hw rx checksum or hw rx hash) in the skb
headroom when we convert the xdp_frame/xdp_buff in the skb? All of these
fields already have dedicated fields in the skb struct. Moreover, we need
to set them in order to have a real performance improvements when we execute
XDP_PASS. Something like:

https://lore.kernel.org/bpf/[e-mail]/

Regards,
Lorenzo

>  }
>  
>  /**
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7b03b64fdcb276f68ce881d1d8da8e4c6b897efc..83f58517738e8ff12990c28b09336ed44f4be32a 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1515,6 +1515,19 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
>  	atomic_inc(&(skb_shinfo(skb)->dataref));
>  	skb->cloned = 1;
>  
> +	/* traits would end up shared with the clone,
> +	 * and edits would be reflected there.
> +	 *
> +	 * Is that ok? What if the original skb and the clone take different paths?
> +	 * Does that even happen?
> +	 *
> +	 * If that's not ok, we could copy the traits and store them in an extension header
> +	 * for clones.
> +	 *
> +	 * For now, pretend the clone doesn't have any traits.
> +	 */
> +	skb->traits_type = SKB_TRAITS_NONE;
> +
>  	return n;
>  #undef C
>  }
> @@ -2170,7 +2183,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  	unsigned int osize = skb_end_offset(skb);
>  	unsigned int size = osize + nhead + ntail;
>  	long off;
> -	u8 *data;
> +	u8 *data, *head;
>  	int i;
>  
>  	BUG_ON(nhead < 0);
> @@ -2187,10 +2200,18 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  		goto nodata;
>  	size = SKB_WITH_OVERHEAD(size);
>  
> +	head = skb->head;
> +	if (skb->traits_type != SKB_TRAITS_NONE) {
> +		head = skb_traits(skb) + traits_size(skb_traits(skb));
> +		/* struct xdp_frame isn't needed in the headroom, drop it */
> +		memcpy(data, skb_traits(skb), traits_size(skb_traits(skb)));
> +		skb->traits_type = SKB_TRAITS_AT_HEAD;
> +	}
> +
>  	/* Copy only real data... and, alas, header. This should be
>  	 * optimized for the cases when header is void.
>  	 */
> -	memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head);
> +	memcpy(data + nhead, head, skb_tail_pointer(skb) - head);
>  
>  	memcpy((struct skb_shared_info *)(data + size),
>  	       skb_shinfo(skb),
> 
> -- 
> 2.43.0
> 
>

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d7dfee152ebd26ce87a230222e94076aca793adc..886537508789202339c925b5613574de67b7e43c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -39,6 +39,7 @@ 
 #include <net/net_debug.h>
 #include <net/dropreason-core.h>
 #include <net/netmem.h>
+#include <net/trait.h>
 
 /**
  * DOC: skb checksums
@@ -729,6 +730,8 @@  enum skb_traits_type {
 	SKB_TRAITS_NONE,
 	/* Trait store in headroom, offset by sizeof(struct xdp_frame) */
 	SKB_TRAITS_AFTER_XDP,
+	/* Trait store at start of headroom */
+	SKB_TRAITS_AT_HEAD,
 };
 
 /**
@@ -1029,7 +1032,7 @@  struct sk_buff {
 	__u8			csum_not_inet:1;
 #endif
 	__u8			unreadable:1;
-	__u8			traits_type:1;	/* See enum skb_traits_type */
+	__u8			traits_type:2;	/* See enum skb_traits_type */
 #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS)
 	__u16			tc_index;	/* traffic control index */
 #endif
@@ -2836,6 +2839,18 @@  static inline void *pskb_pull(struct sk_buff *skb, unsigned int len)
 
 void skb_condense(struct sk_buff *skb);
 
+static inline void *skb_traits(const struct sk_buff *skb)
+{
+	switch (skb->traits_type) {
+	case SKB_TRAITS_AFTER_XDP:
+		return skb->head + _XDP_FRAME_SIZE;
+	case SKB_TRAITS_AT_HEAD:
+		return skb->head;
+	default:
+		return NULL;
+	}
+}
+
 /**
  *	skb_headroom - bytes at buffer head
  *	@skb: buffer to check
@@ -2844,7 +2859,13 @@  void skb_condense(struct sk_buff *skb);
  */
 static inline unsigned int skb_headroom(const struct sk_buff *skb)
 {
-	return skb->data - skb->head;
+	int trait_size = 0;
+	void *traits = skb_traits(skb);
+
+	if (traits)
+		trait_size = traits_size(traits);
+
+	return skb->data - skb->head - trait_size;
 }
 
 /**
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7b03b64fdcb276f68ce881d1d8da8e4c6b897efc..83f58517738e8ff12990c28b09336ed44f4be32a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1515,6 +1515,19 @@  static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 	atomic_inc(&(skb_shinfo(skb)->dataref));
 	skb->cloned = 1;
 
+	/* traits would end up shared with the clone,
+	 * and edits would be reflected there.
+	 *
+	 * Is that ok? What if the original skb and the clone take different paths?
+	 * Does that even happen?
+	 *
+	 * If that's not ok, we could copy the traits and store them in an extension header
+	 * for clones.
+	 *
+	 * For now, pretend the clone doesn't have any traits.
+	 */
+	skb->traits_type = SKB_TRAITS_NONE;
+
 	return n;
 #undef C
 }
@@ -2170,7 +2183,7 @@  int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	unsigned int osize = skb_end_offset(skb);
 	unsigned int size = osize + nhead + ntail;
 	long off;
-	u8 *data;
+	u8 *data, *head;
 	int i;
 
 	BUG_ON(nhead < 0);
@@ -2187,10 +2200,18 @@  int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 		goto nodata;
 	size = SKB_WITH_OVERHEAD(size);
 
+	head = skb->head;
+	if (skb->traits_type != SKB_TRAITS_NONE) {
+		head = skb_traits(skb) + traits_size(skb_traits(skb));
+		/* struct xdp_frame isn't needed in the headroom, drop it */
+		memcpy(data, skb_traits(skb), traits_size(skb_traits(skb)));
+		skb->traits_type = SKB_TRAITS_AT_HEAD;
+	}
+
 	/* Copy only real data... and, alas, header. This should be
 	 * optimized for the cases when header is void.
 	 */
-	memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head);
+	memcpy(data + nhead, head, skb_tail_pointer(skb) - head);
 
 	memcpy((struct skb_shared_info *)(data + size),
 	       skb_shinfo(skb),