[PATCH 0/3] First pass of cleanups for pskb_expand_head

After looking over the tcp coalesing and GRO code a couple of days ago it
occurred to me that pskb_expand_head has a few flaws.  A few of which are
addressed in this patch series.

This change set takes care of some of the minor cleanup items.  One thing
that caught my eye is the fact the memmove code in the fast-path is likely
no longer doing any thing but burning cycles on a call that doesn't
actually move any memory.

The other change is a follow on to that to drop the fastpath variable which
really just means if the skb is cloned or not.

The final change in this set just adds an inline for getting the end offset
since there were multiple places where we were computing end - head to get
the offset and if we are storing it as an offset it makes more sense to
just pull the actual value.

There are a few more items that I will try to get to next week.  The big one
is the fact that pskb_expand_head can mess up the truesize since it can
allocate a new head but never updates the truesize.  I plan on adding a helper
function for the cases where we are just using it unshare the head so I can
identify the places where we are actually modifying the size.

---

Alexander Duyck (3):
      skb: Add inline helper for getting the skb end offset from head
      skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head
      skb: Drop bad code from pskb_expand_head


 drivers/atm/ambassador.c             |    2 +
 drivers/atm/idt77252.c               |    2 +
 drivers/net/wimax/i2400m/usb-rx.c    |    2 +
 drivers/staging/octeon/ethernet-tx.c |    2 +
 include/linux/skbuff.h               |   12 ++++++++-
 net/core/skbuff.c                    |   46 ++++++++++------------------------
 6 files changed, 29 insertions(+), 37 deletions(-)

-- 
Thanks,

Alex

The fast-path for pskb_expand_head contains a check where the size plus the
unaligned size of skb_shared_info is compared against the size of the data
buffer.  This code path has two issues.  First is the fact that after the
recent changes by Eric Dumazet to __alloc_skb and build_skb the shared info
is always placed in the optimal spot for a buffer size making this check
unnecessary.  The second issue is the fact that the check doesn't take into
account the aligned size of shared info.  As a result the code burns cycles
doing a memcpy with nothing actually being shifted.

Signed-off-by: Alexander Duyck <redacted>
---

 net/core/skbuff.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c199aa4..4d085d4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -951,17 +951,6 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 		fastpath = atomic_read(&skb_shinfo(skb)->dataref) == delta;
 	}
 
-	if (fastpath && !skb->head_frag &&
-	    size + sizeof(struct skb_shared_info) <= ksize(skb->head)) {
-		memmove(skb->head + size, skb_shinfo(skb),
-			offsetof(struct skb_shared_info,
-				 frags[skb_shinfo(skb)->nr_frags]));
-		memmove(skb->head + nhead, skb->head,
-			skb_tail_pointer(skb) - skb->head);
-		off = nhead;
-		goto adjust_others;
-	}
-
 	data = kmalloc(size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 		       gfp_mask);
 	if (!data)

@@ -997,7 +986,6 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 
 	skb->head     = data;
 	skb->head_frag = 0;
-adjust_others:
 	skb->data    += off;
 #ifdef NET_SKBUFF_DATA_USES_OFFSET
 	skb->end      = size;

Since there is now only one spot that actually uses "fastpath" there isn't
much point in carrying it.  Instead we can just use a check for skb_cloned
to verify if we can perform the fast-path free for the head or not.

Signed-off-by: Alexander Duyck <redacted>
---

 net/core/skbuff.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4d085d4..17e4b1e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -932,7 +932,6 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	u8 *data;
 	int size = nhead + (skb_end_pointer(skb) - skb->head) + ntail;
 	long off;
-	bool fastpath;
 
 	BUG_ON(nhead < 0);
 

@@ -941,16 +940,6 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 
 	size = SKB_DATA_ALIGN(size);
 
-	/* Check if we can avoid taking references on fragments if we own
-	 * the last reference on skb->head. (see skb_release_data())
-	 */
-	if (!skb->cloned)
-		fastpath = true;
-	else {
-		int delta = skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1;
-		fastpath = atomic_read(&skb_shinfo(skb)->dataref) == delta;
-	}
-
 	data = kmalloc(size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 		       gfp_mask);
 	if (!data)

@@ -966,9 +955,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	       skb_shinfo(skb),
 	       offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
 
-	if (fastpath) {
-		skb_free_head(skb);
-	} else {
+	/*
+	 * if shinfo is shared we must drop the old head gracefully, but if it
+	 * is not we can just drop the old head and let the existing refcount
+	 * be since all we did is relocate the values
+	 */
+	if (skb_cloned(skb)) {
 		/* copy this zero copy skb frags */
 		if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
 			if (skb_copy_ubufs(skb, gfp_mask))

@@ -981,6 +973,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 			skb_clone_fraglist(skb);
 
 		skb_release_data(skb);
+	} else {
+		skb_free_head(skb);
 	}
 	off = (data + nhead) - skb->head;
 

With the recent changes for how we compute the skb truesize it occurs to me
we are probably going to have a lot of calls to skb_end_pointer -
skb->head.  Instead of running all over the place doing that it would make
more sense to just make it a separate inline skb_end_offset(skb) that way
we can return the correct value without having gcc having to do all the
optimization to cancel out skb->head - skb->head.

Signed-off-by: Alexander Duyck <redacted>
---

 drivers/atm/ambassador.c             |    2 +-
 drivers/atm/idt77252.c               |    2 +-
 drivers/net/wimax/i2400m/usb-rx.c    |    2 +-
 drivers/staging/octeon/ethernet-tx.c |    2 +-
 include/linux/skbuff.h               |   12 +++++++++++-
 net/core/skbuff.c                    |   12 ++++++------
 6 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
index f8f41e0..89b30f3 100644
--- a/drivers/atm/ambassador.c
+++ b/drivers/atm/ambassador.c

@@ -802,7 +802,7 @@ static void fill_rx_pool (amb_dev * dev, unsigned char pool,
     }
     // cast needed as there is no %? for pointer differences
     PRINTD (DBG_SKB, "allocated skb at %p, head %p, area %li",
-	    skb, skb->head, (long) (skb_end_pointer(skb) - skb->head));
+	    skb, skb->head, (long) skb_end_offset(skb));
     rx.handle = virt_to_bus (skb);
     rx.host_address = cpu_to_be32 (virt_to_bus (skb->data));
     if (rx_give (dev, &rx, pool))

diff --git a/drivers/atm/idt77252.c b/drivers/atm/idt77252.c
index 1c05212..8974bd2 100644
--- a/drivers/atm/idt77252.c
+++ b/drivers/atm/idt77252.c

@@ -1258,7 +1258,7 @@ idt77252_rx_raw(struct idt77252_dev *card)
 	tail = readl(SAR_REG_RAWCT);
 
 	pci_dma_sync_single_for_cpu(card->pcidev, IDT77252_PRV_PADDR(queue),
-				    skb_end_pointer(queue) - queue->head - 16,
+				    skb_end_offset(queue) - 16,
 				    PCI_DMA_FROMDEVICE);
 
 	while (head != tail) {

diff --git a/drivers/net/wimax/i2400m/usb-rx.c b/drivers/net/wimax/i2400m/usb-rx.c
index e325768..b78ee67 100644
--- a/drivers/net/wimax/i2400m/usb-rx.c
+++ b/drivers/net/wimax/i2400m/usb-rx.c

@@ -277,7 +277,7 @@ retry:
 		d_printf(1, dev, "RX: size changed to %d, received %d, "
 			 "copied %d, capacity %ld\n",
 			 rx_size, read_size, rx_skb->len,
-			 (long) (skb_end_pointer(new_skb) - new_skb->head));
+			 (long) skb_end_offset(new_skb));
 		goto retry;
 	}
 		/* In most cases, it happens due to the hardware scheduling a

diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index 56d74dc..418ed03 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c

@@ -344,7 +344,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 	if (unlikely
 	    (skb->truesize !=
-	     sizeof(*skb) + skb_end_pointer(skb) - skb->head)) {
+	     sizeof(*skb) + skb_end_offset(skb))) {
 		/*
 		   printk("TX buffer truesize has been changed\n");
 		 */

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 37f5391..91ad5e2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -645,11 +645,21 @@ static inline unsigned char *skb_end_pointer(const struct sk_buff *skb)
 {
 	return skb->head + skb->end;
 }
+
+static inline unsigned int skb_end_offset(const struct sk_buff *skb)
+{
+	return skb->end;
+}
 #else
 static inline unsigned char *skb_end_pointer(const struct sk_buff *skb)
 {
 	return skb->end;
 }
+
+static inline unsigned int skb_end_offset(const struct sk_buff *skb)
+{
+	return skb->end - skb->head;
+}
 #endif
 
 /* Internal */

@@ -2558,7 +2568,7 @@ static inline bool skb_is_recycleable(const struct sk_buff *skb, int skb_size)
 		return false;
 
 	skb_size = SKB_DATA_ALIGN(skb_size + NET_SKB_PAD);
-	if (skb_end_pointer(skb) - skb->head < skb_size)
+	if (skb_end_offset(skb) < skb_size)
 		return false;
 
 	if (skb_shared(skb) || skb_cloned(skb))

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 17e4b1e..2c35da8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -829,7 +829,7 @@ static void copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
 {
 	int headerlen = skb_headroom(skb);
-	unsigned int size = (skb_end_pointer(skb) - skb->head) + skb->data_len;
+	unsigned int size = skb_end_offset(skb) + skb->data_len;
 	struct sk_buff *n = alloc_skb(size, gfp_mask);
 
 	if (!n)

@@ -930,7 +930,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 {
 	int i;
 	u8 *data;
-	int size = nhead + (skb_end_pointer(skb) - skb->head) + ntail;
+	int size = nhead + skb_end_offset(skb) + ntail;
 	long off;
 
 	BUG_ON(nhead < 0);

@@ -2727,14 +2727,13 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
 			if (unlikely(!nskb))
 				goto err;
 
-			hsize = skb_end_pointer(nskb) - nskb->head;
+			hsize = skb_end_offset(nskb);
 			if (skb_cow_head(nskb, doffset + headroom)) {
 				kfree_skb(nskb);
 				goto err;
 			}
 
-			nskb->truesize += skb_end_pointer(nskb) - nskb->head -
-					  hsize;
+			nskb->truesize += skb_end_offset(nskb) - hsize;
 			skb_release_head_state(nskb);
 			__skb_push(nskb, doffset);
 		} else {

@@ -2883,7 +2882,8 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
 		skb_frag_size_sub(frag, offset);
 
 		/* all fragments truesize : remove (head size + sk_buff) */
-		delta_truesize = skb->truesize - SKB_TRUESIZE(skb_end_pointer(skb) - skb->head);
+		delta_truesize = skb->truesize -
+				 SKB_TRUESIZE(skb_end_offset(skb));
 
 		skb->truesize -= skb->data_len;
 		skb->len -= skb->data_len;

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

I totally agree this code is no longer needed, we already have the
skb_shared_info at the end of the buffer.

Acked-by: Eric Dumazet <edumazet@google.com>

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

Excellent

Acked-by: Eric Dumazet <edumazet@google.com>

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

With the recent changes for how we compute the skb truesize it occurs to me
we are probably going to have a lot of calls to skb_end_pointer -
skb->head.  Instead of running all over the place doing that it would make
more sense to just make it a separate inline skb_end_offset(skb) that way
we can return the correct value without having gcc having to do all the
optimization to cancel out skb->head - skb->head.

Signed-off-by: Alexander Duyck <redacted>
---

 drivers/atm/ambassador.c             |    2 +-
 drivers/atm/idt77252.c               |    2 +-
 drivers/net/wimax/i2400m/usb-rx.c    |    2 +-
 drivers/staging/octeon/ethernet-tx.c |    2 +-
 include/linux/skbuff.h               |   12 +++++++++++-
 net/core/skbuff.c                    |   12 ++++++------
 6 files changed, 21 insertions(+), 11 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

pull the actual value.

There are a few more items that I will try to get to next week.  The big one
is the fact that pskb_expand_head can mess up the truesize since it can
allocate a new head but never updates the truesize.  I plan on adding a helper
function for the cases where we are just using it unshare the head so I can
identify the places where we are actually modifying the size.

In the old days, truesize adjustements were done after
pskb_expand_head() calls. (Mabye because some contexts didnt care of
truesize for ephemeral skbs, not charged to a socket)

So it will be a nice cleanup for sure.

On 5/4/2012 10:44 PM, Eric Dumazet wrote:

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

quoted

pull the actual value.

There are a few more items that I will try to get to next week.  The big one
is the fact that pskb_expand_head can mess up the truesize since it can
allocate a new head but never updates the truesize.  I plan on adding a helper
function for the cases where we are just using it unshare the head so I can
identify the places where we are actually modifying the size.

In the old days, truesize adjustements were done after
pskb_expand_head() calls. (Mabye because some contexts didnt care of
truesize for ephemeral skbs, not charged to a socket)

So it will be a nice cleanup for sure.

I suspect the reason for no truesize adjustment is because this function 
gets called in the transmit path, and we probably should be adjusting 
truesize while there is still a desctructor in place that will turn 
around and subtract the truesize from the socket memory.  I'm still 
thinking about what would be the best solution to that, but in the 
meantime I figure I can at least add a helper function to handle all the 
pskb_expand_head(skb, 0, 0, GFP_ATOMIC) cases and just replace them with 
something like skb_unshare_head(skb).  That way I will have a better 
idea of the few cases where we might actually impact truesize.

From: Eric Dumazet <redacted>
Date: Sat, 05 May 2012 07:35:30 +0200

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

quoted

The fast-path for pskb_expand_head contains a check where the size plus the
unaligned size of skb_shared_info is compared against the size of the data
buffer.  This code path has two issues.  First is the fact that after the
recent changes by Eric Dumazet to __alloc_skb and build_skb the shared info
is always placed in the optimal spot for a buffer size making this check
unnecessary.  The second issue is the fact that the check doesn't take into
account the aligned size of shared info.  As a result the code burns cycles
doing a memcpy with nothing actually being shifted.

Signed-off-by: Alexander Duyck <redacted>

 ...

Acked-by: Eric Dumazet <edumazet@google.com>

Applied.

From: Eric Dumazet <redacted>
Date: Sat, 05 May 2012 07:37:24 +0200

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

quoted

Since there is now only one spot that actually uses "fastpath" there isn't
much point in carrying it.  Instead we can just use a check for skb_cloned
to verify if we can perform the fast-path free for the head or not.

Signed-off-by: Alexander Duyck <redacted>

 ...

Acked-by: Eric Dumazet <edumazet@google.com>

Applied.

From: Eric Dumazet <redacted>
Date: Sat, 05 May 2012 07:39:21 +0200

On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:

quoted

With the recent changes for how we compute the skb truesize it occurs to me
we are probably going to have a lot of calls to skb_end_pointer -
skb->head.  Instead of running all over the place doing that it would make
more sense to just make it a separate inline skb_end_offset(skb) that way
we can return the correct value without having gcc having to do all the
optimization to cancel out skb->head - skb->head.

Signed-off-by: Alexander Duyck <redacted>

 ...

Acked-by: Eric Dumazet <edumazet@google.com>

Applied.