| Message ID | 20220620155641.GA3846@debian (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: helper function for skb_shift | expand |
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 <richardbgobert@gmail.com> 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.
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 > + */ > +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.
On Tue, Jun 21, 2022 at 10:12:40PM -0700, Jakub Kicinski wrote: > That's better, thanks. Please move the helper to skbuff.h Will do. > 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?
On Wed, 22 Jun 2022 12:25:20 +0200 Richard Gobert wrote: > On Tue, Jun 21, 2022 at 10:12:40PM -0700, Jakub Kicinski wrote: > > 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.
diff --git a/include/net/sock.h b/include/net/sock.h index a01d6c421aa2..21122a22f624 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -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; diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 30b523fa4ad2..e6b5dd69b78f 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -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; } diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c index d747166bb291..6d4194349108 100644 --- a/net/ipv4/esp4.c +++ b/net/ipv4/esp4.c @@ -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); diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 00b4bf26fd93..2c46ecc495a4 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -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;
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 <richardbgobert@gmail.com> --- 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(-)