* [PATCH 0/3] First pass of cleanups for pskb_expand_head
@ 2012-05-05 0:26 Alexander Duyck
2012-05-05 0:26 ` [PATCH 1/3] skb: Drop bad code from pskb_expand_head Alexander Duyck
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Alexander Duyck @ 2012-05-05 0:26 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, jeffrey.t.kirsher
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] skb: Drop bad code from pskb_expand_head
2012-05-05 0:26 [PATCH 0/3] First pass of cleanups for pskb_expand_head Alexander Duyck
@ 2012-05-05 0:26 ` Alexander Duyck
2012-05-05 5:35 ` Eric Dumazet
2012-05-05 0:26 ` [PATCH 2/3] skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head Alexander Duyck
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Alexander Duyck @ 2012-05-05 0:26 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, jeffrey.t.kirsher
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 <alexander.h.duyck@intel.com>
---
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;
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head
2012-05-05 0:26 [PATCH 0/3] First pass of cleanups for pskb_expand_head Alexander Duyck
2012-05-05 0:26 ` [PATCH 1/3] skb: Drop bad code from pskb_expand_head Alexander Duyck
@ 2012-05-05 0:26 ` Alexander Duyck
2012-05-05 5:37 ` Eric Dumazet
2012-05-05 0:26 ` [PATCH 3/3] skb: Add inline helper for getting the skb end offset from head Alexander Duyck
2012-05-05 5:44 ` [PATCH 0/3] First pass of cleanups for pskb_expand_head Eric Dumazet
3 siblings, 1 reply; 12+ messages in thread
From: Alexander Duyck @ 2012-05-05 0:26 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, jeffrey.t.kirsher
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 <alexander.h.duyck@intel.com>
---
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;
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] skb: Add inline helper for getting the skb end offset from head
2012-05-05 0:26 [PATCH 0/3] First pass of cleanups for pskb_expand_head Alexander Duyck
2012-05-05 0:26 ` [PATCH 1/3] skb: Drop bad code from pskb_expand_head Alexander Duyck
2012-05-05 0:26 ` [PATCH 2/3] skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head Alexander Duyck
@ 2012-05-05 0:26 ` Alexander Duyck
2012-05-05 5:39 ` Eric Dumazet
2012-05-05 5:44 ` [PATCH 0/3] First pass of cleanups for pskb_expand_head Eric Dumazet
3 siblings, 1 reply; 12+ messages in thread
From: Alexander Duyck @ 2012-05-05 0:26 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, jeffrey.t.kirsher
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 <alexander.h.duyck@intel.com>
---
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;
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] skb: Drop bad code from pskb_expand_head
2012-05-05 0:26 ` [PATCH 1/3] skb: Drop bad code from pskb_expand_head Alexander Duyck
@ 2012-05-05 5:35 ` Eric Dumazet
2012-05-06 17:13 ` David Miller
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2012-05-05 5:35 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, jeffrey.t.kirsher
On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:
> 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 <alexander.h.duyck@intel.com>
> ---
>
> 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;
>
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head
2012-05-05 0:26 ` [PATCH 2/3] skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head Alexander Duyck
@ 2012-05-05 5:37 ` Eric Dumazet
2012-05-06 17:13 ` David Miller
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2012-05-05 5:37 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, jeffrey.t.kirsher
On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:
> 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 <alexander.h.duyck@intel.com>
> ---
>
> 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;
>
>
Excellent
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] skb: Add inline helper for getting the skb end offset from head
2012-05-05 0:26 ` [PATCH 3/3] skb: Add inline helper for getting the skb end offset from head Alexander Duyck
@ 2012-05-05 5:39 ` Eric Dumazet
2012-05-06 17:13 ` David Miller
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2012-05-05 5:39 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, jeffrey.t.kirsher
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 <alexander.h.duyck@intel.com>
> ---
>
> 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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] First pass of cleanups for pskb_expand_head
2012-05-05 0:26 [PATCH 0/3] First pass of cleanups for pskb_expand_head Alexander Duyck
` (2 preceding siblings ...)
2012-05-05 0:26 ` [PATCH 3/3] skb: Add inline helper for getting the skb end offset from head Alexander Duyck
@ 2012-05-05 5:44 ` Eric Dumazet
2012-05-05 6:51 ` Alexander Duyck
3 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2012-05-05 5:44 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, jeffrey.t.kirsher
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] First pass of cleanups for pskb_expand_head
2012-05-05 5:44 ` [PATCH 0/3] First pass of cleanups for pskb_expand_head Eric Dumazet
@ 2012-05-05 6:51 ` Alexander Duyck
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Duyck @ 2012-05-05 6:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Alexander Duyck, netdev, davem, jeffrey.t.kirsher
On 5/4/2012 10:44 PM, Eric Dumazet wrote:
> 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.
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] skb: Drop bad code from pskb_expand_head
2012-05-05 5:35 ` Eric Dumazet
@ 2012-05-06 17:13 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-05-06 17:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: alexander.h.duyck, netdev, jeffrey.t.kirsher
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 05 May 2012 07:35:30 +0200
> On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:
>> 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 <alexander.h.duyck@intel.com>
...
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head
2012-05-05 5:37 ` Eric Dumazet
@ 2012-05-06 17:13 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-05-06 17:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: alexander.h.duyck, netdev, jeffrey.t.kirsher
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 05 May 2012 07:37:24 +0200
> On Fri, 2012-05-04 at 17:26 -0700, Alexander Duyck wrote:
>> 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 <alexander.h.duyck@intel.com>
...
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] skb: Add inline helper for getting the skb end offset from head
2012-05-05 5:39 ` Eric Dumazet
@ 2012-05-06 17:13 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-05-06 17:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: alexander.h.duyck, netdev, jeffrey.t.kirsher
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 05 May 2012 07:39:21 +0200
> 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 <alexander.h.duyck@intel.com>
...
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-05-06 17:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-05 0:26 [PATCH 0/3] First pass of cleanups for pskb_expand_head Alexander Duyck
2012-05-05 0:26 ` [PATCH 1/3] skb: Drop bad code from pskb_expand_head Alexander Duyck
2012-05-05 5:35 ` Eric Dumazet
2012-05-06 17:13 ` David Miller
2012-05-05 0:26 ` [PATCH 2/3] skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head Alexander Duyck
2012-05-05 5:37 ` Eric Dumazet
2012-05-06 17:13 ` David Miller
2012-05-05 0:26 ` [PATCH 3/3] skb: Add inline helper for getting the skb end offset from head Alexander Duyck
2012-05-05 5:39 ` Eric Dumazet
2012-05-06 17:13 ` David Miller
2012-05-05 5:44 ` [PATCH 0/3] First pass of cleanups for pskb_expand_head Eric Dumazet
2012-05-05 6:51 ` Alexander Duyck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).