* [PATCH 8/9] net: Don't keep around original SKB when we software segment GSO frames.
@ 2014-09-01 22:25 David Miller
2014-09-02 1:28 ` Tom Herbert
0 siblings, 1 reply; 4+ messages in thread
From: David Miller @ 2014-09-01 22:25 UTC (permalink / raw)
To: netdev
Just maintain the list properly by returning the head of the remaining
SKB list from dev_hard_start_xmit().
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/linux/netdevice.h | 4 +--
net/core/dev.c | 79 +++++++++--------------------------------------
net/sched/sch_generic.c | 2 +-
3 files changed, 17 insertions(+), 68 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 47c49ba..202c25a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2828,8 +2828,8 @@ int dev_change_carrier(struct net_device *, bool new_carrier);
int dev_get_phys_port_id(struct net_device *dev,
struct netdev_phys_port_id *ppid);
struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev);
-int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
- struct netdev_queue *txq);
+struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
+ struct netdev_queue *txq, int *ret);
int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
bool is_skb_forwardable(struct net_device *dev, struct sk_buff *skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 75bc5b0..c89da4f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2485,52 +2485,6 @@ static int illegal_highdma(struct net_device *dev, struct sk_buff *skb)
return 0;
}
-struct dev_gso_cb {
- void (*destructor)(struct sk_buff *skb);
-};
-
-#define DEV_GSO_CB(skb) ((struct dev_gso_cb *)(skb)->cb)
-
-static void dev_gso_skb_destructor(struct sk_buff *skb)
-{
- struct dev_gso_cb *cb;
-
- kfree_skb_list(skb->next);
- skb->next = NULL;
-
- cb = DEV_GSO_CB(skb);
- if (cb->destructor)
- cb->destructor(skb);
-}
-
-/**
- * dev_gso_segment - Perform emulated hardware segmentation on skb.
- * @skb: buffer to segment
- * @features: device features as applicable to this skb
- *
- * This function segments the given skb and stores the list of segments
- * in skb->next.
- */
-static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features)
-{
- struct sk_buff *segs;
-
- segs = skb_gso_segment(skb, features);
-
- /* Verifying header integrity only. */
- if (!segs)
- return 0;
-
- if (IS_ERR(segs))
- return PTR_ERR(segs);
-
- skb->next = segs;
- DEV_GSO_CB(skb)->destructor = skb->destructor;
- skb->destructor = dev_gso_skb_destructor;
-
- return 0;
-}
-
/* If MPLS offload request, verify we are testing hardware MPLS features
* instead of standard features for the netdev.
*/
@@ -2682,8 +2636,13 @@ struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
features &= dev->hw_enc_features;
if (netif_needs_gso(skb, features)) {
- if (unlikely(dev_gso_segment(skb, features)))
- goto out_kfree_skb;
+ struct sk_buff *segs;
+
+ segs = skb_gso_segment(skb, features);
+ kfree_skb(skb);
+ if (IS_ERR(segs))
+ segs = NULL;
+ skb = segs;
} else {
if (skb_needs_linearize(skb, features) &&
__skb_linearize(skb))
@@ -2714,26 +2673,16 @@ out_null:
return NULL;
}
-int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
- struct netdev_queue *txq)
+struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
+ struct netdev_queue *txq, int *ret)
{
- int rc = NETDEV_TX_OK;
-
- if (likely(!skb->next))
- return xmit_one(skb, dev, txq, false);
-
- skb->next = xmit_list(skb->next, dev, txq, &rc);
- if (likely(skb->next == NULL)) {
- skb->destructor = DEV_GSO_CB(skb)->destructor;
- consume_skb(skb);
- return rc;
+ if (likely(!skb->next)) {
+ *ret = xmit_one(skb, dev, txq, false);
+ return skb;
}
- kfree_skb(skb);
-
- return rc;
+ return xmit_list(skb, dev, txq, ret);
}
-EXPORT_SYMBOL_GPL(dev_hard_start_xmit);
static void qdisc_pkt_len_init(struct sk_buff *skb)
{
@@ -2945,7 +2894,7 @@ static int __dev_queue_xmit(struct sk_buff *skb, void *accel_priv)
if (!netif_xmit_stopped(txq)) {
__this_cpu_inc(xmit_recursion);
- rc = dev_hard_start_xmit(skb, dev, txq);
+ skb = dev_hard_start_xmit(skb, dev, txq, &rc);
__this_cpu_dec(xmit_recursion);
if (dev_xmit_complete(rc)) {
HARD_TX_UNLOCK(dev, txq);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index f178798..a8bf9f9 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -129,7 +129,7 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
HARD_TX_LOCK(dev, txq, smp_processor_id());
if (!netif_xmit_frozen_or_stopped(txq))
- ret = dev_hard_start_xmit(skb, dev, txq);
+ skb = dev_hard_start_xmit(skb, dev, txq, &ret);
HARD_TX_UNLOCK(dev, txq);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 8/9] net: Don't keep around original SKB when we software segment GSO frames.
2014-09-01 22:25 [PATCH 8/9] net: Don't keep around original SKB when we software segment GSO frames David Miller
@ 2014-09-02 1:28 ` Tom Herbert
2014-09-02 1:42 ` David Miller
2014-09-02 8:49 ` David Laight
0 siblings, 2 replies; 4+ messages in thread
From: Tom Herbert @ 2014-09-02 1:28 UTC (permalink / raw)
To: David Miller; +Cc: Linux Netdev List
On Mon, Sep 1, 2014 at 3:25 PM, David Miller <davem@davemloft.net> wrote:
>
> Just maintain the list properly by returning the head of the remaining
> SKB list from dev_hard_start_xmit().
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> include/linux/netdevice.h | 4 +--
> net/core/dev.c | 79 +++++++++--------------------------------------
> net/sched/sch_generic.c | 2 +-
> 3 files changed, 17 insertions(+), 68 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 47c49ba..202c25a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2828,8 +2828,8 @@ int dev_change_carrier(struct net_device *, bool new_carrier);
> int dev_get_phys_port_id(struct net_device *dev,
> struct netdev_phys_port_id *ppid);
> struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev);
> -int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> - struct netdev_queue *txq);
> +struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> + struct netdev_queue *txq, int *ret);
Might be slightly better to still return int but pass struct sk_buff
**skb. One less argument and doesn't change return type.
> int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> bool is_skb_forwardable(struct net_device *dev, struct sk_buff *skb);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 75bc5b0..c89da4f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2485,52 +2485,6 @@ static int illegal_highdma(struct net_device *dev, struct sk_buff *skb)
> return 0;
> }
>
> -struct dev_gso_cb {
> - void (*destructor)(struct sk_buff *skb);
> -};
> -
> -#define DEV_GSO_CB(skb) ((struct dev_gso_cb *)(skb)->cb)
> -
> -static void dev_gso_skb_destructor(struct sk_buff *skb)
> -{
> - struct dev_gso_cb *cb;
> -
> - kfree_skb_list(skb->next);
> - skb->next = NULL;
> -
> - cb = DEV_GSO_CB(skb);
> - if (cb->destructor)
> - cb->destructor(skb);
> -}
> -
> -/**
> - * dev_gso_segment - Perform emulated hardware segmentation on skb.
> - * @skb: buffer to segment
> - * @features: device features as applicable to this skb
> - *
> - * This function segments the given skb and stores the list of segments
> - * in skb->next.
> - */
> -static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features)
> -{
> - struct sk_buff *segs;
> -
> - segs = skb_gso_segment(skb, features);
> -
> - /* Verifying header integrity only. */
> - if (!segs)
> - return 0;
> -
> - if (IS_ERR(segs))
> - return PTR_ERR(segs);
> -
> - skb->next = segs;
> - DEV_GSO_CB(skb)->destructor = skb->destructor;
> - skb->destructor = dev_gso_skb_destructor;
> -
> - return 0;
> -}
> -
> /* If MPLS offload request, verify we are testing hardware MPLS features
> * instead of standard features for the netdev.
> */
> @@ -2682,8 +2636,13 @@ struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
> features &= dev->hw_enc_features;
>
> if (netif_needs_gso(skb, features)) {
> - if (unlikely(dev_gso_segment(skb, features)))
> - goto out_kfree_skb;
> + struct sk_buff *segs;
> +
> + segs = skb_gso_segment(skb, features);
> + kfree_skb(skb);
> + if (IS_ERR(segs))
> + segs = NULL;
> + skb = segs;
> } else {
> if (skb_needs_linearize(skb, features) &&
> __skb_linearize(skb))
> @@ -2714,26 +2673,16 @@ out_null:
> return NULL;
> }
>
> -int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> - struct netdev_queue *txq)
> +struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> + struct netdev_queue *txq, int *ret)
> {
> - int rc = NETDEV_TX_OK;
> -
> - if (likely(!skb->next))
> - return xmit_one(skb, dev, txq, false);
> -
> - skb->next = xmit_list(skb->next, dev, txq, &rc);
> - if (likely(skb->next == NULL)) {
> - skb->destructor = DEV_GSO_CB(skb)->destructor;
> - consume_skb(skb);
> - return rc;
> + if (likely(!skb->next)) {
> + *ret = xmit_one(skb, dev, txq, false);
> + return skb;
> }
>
> - kfree_skb(skb);
> -
> - return rc;
> + return xmit_list(skb, dev, txq, ret);
> }
> -EXPORT_SYMBOL_GPL(dev_hard_start_xmit);
>
> static void qdisc_pkt_len_init(struct sk_buff *skb)
> {
> @@ -2945,7 +2894,7 @@ static int __dev_queue_xmit(struct sk_buff *skb, void *accel_priv)
>
> if (!netif_xmit_stopped(txq)) {
> __this_cpu_inc(xmit_recursion);
> - rc = dev_hard_start_xmit(skb, dev, txq);
> + skb = dev_hard_start_xmit(skb, dev, txq, &rc);
> __this_cpu_dec(xmit_recursion);
> if (dev_xmit_complete(rc)) {
> HARD_TX_UNLOCK(dev, txq);
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index f178798..a8bf9f9 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -129,7 +129,7 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>
> HARD_TX_LOCK(dev, txq, smp_processor_id());
> if (!netif_xmit_frozen_or_stopped(txq))
> - ret = dev_hard_start_xmit(skb, dev, txq);
> + skb = dev_hard_start_xmit(skb, dev, txq, &ret);
>
> HARD_TX_UNLOCK(dev, txq);
>
> --
> 1.7.11.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 8/9] net: Don't keep around original SKB when we software segment GSO frames.
2014-09-02 1:28 ` Tom Herbert
@ 2014-09-02 1:42 ` David Miller
2014-09-02 8:49 ` David Laight
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2014-09-02 1:42 UTC (permalink / raw)
To: therbert; +Cc: netdev
From: Tom Herbert <therbert@google.com>
Date: Mon, 1 Sep 2014 18:28:18 -0700
> On Mon, Sep 1, 2014 at 3:25 PM, David Miller <davem@davemloft.net> wrote:
>> struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev);
>> -int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>> - struct netdev_queue *txq);
>> +struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>> + struct netdev_queue *txq, int *ret);
>
> Might be slightly better to still return int but pass struct sk_buff
> **skb. One less argument and doesn't change return type.
Sure we could do that, I don't have any preference either way and
besides there is only one call site.
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 8/9] net: Don't keep around original SKB when we software segment GSO frames.
2014-09-02 1:28 ` Tom Herbert
2014-09-02 1:42 ` David Miller
@ 2014-09-02 8:49 ` David Laight
1 sibling, 0 replies; 4+ messages in thread
From: David Laight @ 2014-09-02 8:49 UTC (permalink / raw)
To: 'Tom Herbert', David Miller; +Cc: Linux Netdev List
From: Tom Herbert
> On Mon, Sep 1, 2014 at 3:25 PM, David Miller <davem@davemloft.net> wrote:
> >
> > Just maintain the list properly by returning the head of the remaining
> > SKB list from dev_hard_start_xmit().
> >
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > ---
> > include/linux/netdevice.h | 4 +--
> > net/core/dev.c | 79 +++++++++--------------------------------------
> > net/sched/sch_generic.c | 2 +-
> > 3 files changed, 17 insertions(+), 68 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 47c49ba..202c25a 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -2828,8 +2828,8 @@ int dev_change_carrier(struct net_device *, bool new_carrier);
> > int dev_get_phys_port_id(struct net_device *dev,
> > struct netdev_phys_port_id *ppid);
> > struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev);
> > -int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> > - struct netdev_queue *txq);
> > +struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> > + struct netdev_queue *txq, int *ret);
>
> Might be slightly better to still return int but pass struct sk_buff
> **skb. One less argument and doesn't change return type.
It makes a difference to how the compiler can compile the calling code.
Whichever variable you pass by reference has to be assumed to be changeable
by every following function call - so can't be placed in a callee saved register.
Ideally you want both values returned in registers - difficult to do portably.
David
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-02 8:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-01 22:25 [PATCH 8/9] net: Don't keep around original SKB when we software segment GSO frames David Miller
2014-09-02 1:28 ` Tom Herbert
2014-09-02 1:42 ` David Miller
2014-09-02 8:49 ` David Laight
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).