Skip to contentSkip to conversation

net: helper function for skb_shift

PATCH In review Richard Gobert 2 participants 5 messages Jun 20, 2022 – Jun 22, 2022

Subsystems (5)
Networking Network drivers IPv4/IPv6 Sockets +1 more subsystems
lore
Share & tools
lore

Richard Gobert

author
#0 lore raw

PATCH v2 1/1 net: helper function for skb_shift

Move the len fields manipulation in the skbs to a helper function. There is a comment specifically requesting this and there are several other areas in the code displaying the same pattern which can be refactored.
This improves code readability.

Signed-off-by Richard Gobert <[email protected]>
---
 include/net/sock.h   | 16 +++++++++++++---
 net/core/skbuff.c    | 13 +++----------
 net/ipv4/esp4.c      |  4 +---
 net/ipv4/ip_output.c |  8 ++------
 4 files changed, 19 insertions(+), 22 deletions(-)
include/net/sock.h +13 -3
index a01d6c421aa2..21122a22f624 100644
@@ -2218,6 +2218,18 @@ static inline int skb_add_data_nocache(struct sock *sk, struct sk_buff *skb,
 	return err;
 }
 
+/**
+ * skb_update_len - Updates len fields of skb
+ * @skb: buffer to add len to
+ * @len: number of bytes to add
+ */
+static inline void skb_update_len(struct sk_buff *skb, int len)
+{
+	skb->len += len;
+	skb->data_len += len;
+	skb->truesize += len;
+}
+
 static inline int skb_copy_to_page_nocache(struct sock *sk, struct iov_iter *from,
 					   struct sk_buff *skb,
 					   struct page *page,

@@ -2230,9 +2242,7 @@ static inline int skb_copy_to_page_nocache(struct sock *sk, struct iov_iter *fro
 	if (err)
 		return err;
 
-	skb->len	     += copy;
-	skb->data_len	     += copy;
-	skb->truesize	     += copy;
+	skb_update_len(skb, copy);
 	sk_wmem_queued_add(sk, copy);
 	sk_mem_charge(sk, copy);
 	return 0;
net/core/skbuff.c +3 -10
index 30b523fa4ad2..e6b5dd69b78f 100644
@@ -3195,9 +3195,7 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
 		}
 	}
 
-	to->truesize += len + plen;
-	to->len += len + plen;
-	to->data_len += len + plen;
+	skb_update_len(to, len + plen);
 
 	if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) {
 		skb_tx_error(from);

@@ -3634,13 +3632,8 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 	tgt->ip_summed = CHECKSUM_PARTIAL;
 	skb->ip_summed = CHECKSUM_PARTIAL;
 
-	/* Yak, is it really working this way? Some helper please? */
-	skb->len -= shiftlen;
-	skb->data_len -= shiftlen;
-	skb->truesize -= shiftlen;
-	tgt->len += shiftlen;
-	tgt->data_len += shiftlen;
-	tgt->truesize += shiftlen;
+	skb_update_len(skb, -shiftlen);
+	skb_update_len(tgt, shiftlen);
 
 	return shiftlen;
 }
net/ipv4/esp4.c +1 -3
index d747166bb291..6d4194349108 100644
@@ -502,9 +502,7 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 
 			nfrags++;
 
-			skb->len += tailen;
-			skb->data_len += tailen;
-			skb->truesize += tailen;
+			skb_update_len(skb, tailen);
 			if (sk && sk_fullsock(sk))
 				refcount_add(tailen, &sk->sk_wmem_alloc);
net/ipv4/ip_output.c +2 -6
index 00b4bf26fd93..2c46ecc495a4 100644
@@ -1214,9 +1214,7 @@ static int __ip_append_data(struct sock *sk,
 
 			pfrag->offset += copy;
 			skb_frag_size_add(&skb_shinfo(skb)->frags[i - 1], copy);
-			skb->len += copy;
-			skb->data_len += copy;
-			skb->truesize += copy;
+			skb_update_len(skb, copy);
 			wmem_alloc_delta += copy;
 		} else {
 			err = skb_zerocopy_iter_dgram(skb, from, copy);

@@ -1443,9 +1441,7 @@ ssize_t	ip_append_page(struct sock *sk, struct flowi4 *fl4, struct page *page,
 			skb->csum = csum_block_add(skb->csum, csum, skb->len);
 		}
 
-		skb->len += len;
-		skb->data_len += len;
-		skb->truesize += len;
+		skb_update_len(skb, len);
 		refcount_add(len, &sk->sk_wmem_alloc);
 		offset += len;
 		size -= len;
Signature · 1 line
2.36.1
maintainer
#1 lore raw
Quoted 7 lines · On Mon, 20 Jun 2022 17:56:48 +0200 Richard Gobert wrote:

Move the len fields manipulation in the skbs to a helper function. There is a comment specifically requesting this and there are several other areas in the code displaying the same pattern which can be refactored.
This improves code readability.

Signed-off-by Richard Gobert <[email protected]>

That's better, thanks. Please move the helper to skbuff.h and consider changing the subject from talking about skb_shift() which is just one of the users now to mentioning the helper's name.

author
#2 lore raw
Quoted 1 line · Jakub Kicinski

That's better, thanks. Please move the helper to skbuff.h

Will do.

Quoted 2 lines

and consider changing the subject from talking about skb_shift() which is just one of the users now to mentioning the helper's name.

When submitting the updated patch with a new subject, should it be marked v3 or is it considered a new patch entirely?

maintainer
#3 lore raw
Quoted 6 lines · On Wed, 22 Jun 2022 12:25:20 +0200 Richard Gobert wrote:
Quoted 2 lines · Jakub Kicinski

and consider changing the subject from talking about skb_shift() which is just one of the users now to mentioning the helper's name.

When submitting the updated patch with a new subject, should it be marked v3 or is it considered a new patch entirely?

v3 is better, that way maintainers will know to look for a previous version to check if feedback has been addressed.

maintainer
#4 lore raw
In reply to Richard Gobert

include/net/sock.h static inline int skb_add_data_nocache(struct sock *sk, struct sk_buff *skb, · line 2231

Quoted 10 lines · On Mon, 20 Jun 2022 17:56:48 +0200 Richard Gobert wrote:

+ * skb_update_len - Updates len fields of skb
+ * @skb: buffer to add len to
+ * @len: number of bytes to add
+ */

include/net/sock.h +6 -0
+static inline void skb_update_len(struct sk_buff *skb, int len)
+{
+	skb->len += len;
+	skb->data_len += len;
+	skb->truesize += len;
+}

On reflection, maybe skb_len_add() would be a better name? "update" does not express what the meaning of the parameter is. It could be delta it could be new length.