| Message ID | 20140901.152450.815155429225102019.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, On Mon, 01 Sep 2014 15:24:50 -0700 (PDT) David Miller <davem@davemloft.net> wrote: > @@ -2681,25 +2709,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, > } > > gso: > - do { > - struct sk_buff *nskb = skb->next; > - > - skb->next = nskb->next; > - nskb->next = NULL; > - > - rc = xmit_one(nskb, dev, txq); > - if (unlikely(rc != NETDEV_TX_OK)) { > - if (rc & ~NETDEV_TX_MASK) > - goto out_kfree_gso_skb; > - nskb->next = skb->next; > - skb->next = nskb; > - return rc; > - } > - if (unlikely(netif_xmit_stopped(txq) && skb->next)) > - return NETDEV_TX_BUSY; > - } while (skb->next); > - > -out_kfree_gso_skb: > + skb->next = xmit_list(skb->next, dev, txq, &rc); > if (likely(skb->next == NULL)) { > skb->destructor = DEV_GSO_CB(skb)->destructor; > consume_skb(skb); > } > out_kfree_skb: > kfree_skb(skb); Formely, if 'netif_xmit_stopped(txq) && skb->next', NETDEV_TX_BUSY was returned, but the head 'skb' wasn't freed; Currently, if 'xmit_list' returns non-NULL due to same reason, head skb gets freed in out_kfree_skb. I'm not certain, but it looks like an oversight that got fixed later in the patchset: In net-next ce93718fb "net: Don't keep around original SKB when we software segment GSO frames." the 'kfree_skb(skb)' in 'dev_hard_start_xmit' got relocated into validate_xmit_skb's gso segmentation part, making the issue disappear. Am I right? Didn't we miss anything else here? -- 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
diff --git a/net/core/dev.c b/net/core/dev.c index 0fde7d2..ab7bb80 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2616,6 +2616,34 @@ static int xmit_one(struct sk_buff *skb, struct net_device *dev, return rc; } +static struct sk_buff *xmit_list(struct sk_buff *first, struct net_device *dev, + struct netdev_queue *txq, int *ret) +{ + struct sk_buff *skb = first; + int rc = NETDEV_TX_OK; + + while (skb) { + struct sk_buff *next = skb->next; + + skb->next = NULL; + rc = xmit_one(skb, dev, txq); + if (unlikely(!dev_xmit_complete(rc))) { + skb->next = next; + goto out; + } + + skb = next; + if (netif_xmit_stopped(txq) && skb) { + rc = NETDEV_TX_BUSY; + break; + } + } + +out: + *ret = rc; + return skb; +} + int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, struct netdev_queue *txq) { @@ -2681,25 +2709,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, } gso: - do { - struct sk_buff *nskb = skb->next; - - skb->next = nskb->next; - nskb->next = NULL; - - rc = xmit_one(nskb, dev, txq); - if (unlikely(rc != NETDEV_TX_OK)) { - if (rc & ~NETDEV_TX_MASK) - goto out_kfree_gso_skb; - nskb->next = skb->next; - skb->next = nskb; - return rc; - } - if (unlikely(netif_xmit_stopped(txq) && skb->next)) - return NETDEV_TX_BUSY; - } while (skb->next); - -out_kfree_gso_skb: + skb->next = xmit_list(skb->next, dev, txq, &rc); if (likely(skb->next == NULL)) { skb->destructor = DEV_GSO_CB(skb)->destructor; consume_skb(skb);
There is a slight policy change happening here as well. The previous code would drop the entire rest of the GSO skb if any of them got, for example, a congestion notification. That makes no sense, anything NET_XMIT_MASK and below is something like congestion or policing. And in the congestion case it doesn't even mean the packet was actually dropped. Just continue until dev_xmit_complete() evaluates to false. Signed-off-by: David S. Miller <davem@davemloft.net> --- net/core/dev.c | 48 +++++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 19 deletions(-)