| Message ID | 1366235536-15744-3-git-send-email-bigeasy@linutronix.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote: > if during "ifconfig up" we run out of mem we continue regardless how > many skbs we got. In worst case we have zero RX skbs and can't ever > receive further packets since the RX skbs are never reallocated. If > cpdma_chan_submit() fails we even leak the skb. > This patch changes the behavior here: > If we fail to allocate an skb during bring up we don't continue and > report that error. Same goes for errors from cpdma_chan_submit(). > While here I changed to __netdev_alloc_skb_ip_align() so GFP_KERNEL can > be used. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/net/ethernet/ti/cpsw.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index e2ba702..3b22a36 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev) > struct sk_buff *skb; > > ret = -ENOMEM; > - skb = netdev_alloc_skb_ip_align(priv->ndev, > - priv->rx_packet_max); > + skb = __netdev_alloc_skb_ip_align(priv->ndev, > + priv->rx_packet_max, GFP_KERNEL); > if (!skb) > - break; > + goto err_cleanup; > ret = cpdma_chan_submit(priv->rxch, skb, skb->data, > skb_tailroom(skb), 0, GFP_KERNEL); > - if (WARN_ON(ret < 0)) > - break; > + if (ret < 0) { > + kfree_skb(skb); > + goto err_cleanup; Why you need to close the device even you have some skb allocated and submitted successfully. Can allow the device to continue with lower performance > + } > } > /* continue even if we didn't manage to submit all > * receive descs > @@ -944,6 +946,10 @@ static int cpsw_ndo_open(struct net_device *ndev) > if (priv->data.dual_emac) > priv->slaves[priv->emac_port].open_stat = true; > return 0; > + > +err_cleanup: > + cpdma_ctlr_stop(priv->dma); > + return ret; > } only cpdma is halted and allocated skb are released, need to have other calls like pm_runtime_put_sync, close host and slave ports Regards Mugunthan V N -- 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
On 04/18/2013 01:50 PM, Mugunthan V N wrote: >> diff --git a/drivers/net/ethernet/ti/cpsw.c >> b/drivers/net/ethernet/ti/cpsw.c >> index e2ba702..3b22a36 100644 >> --- a/drivers/net/ethernet/ti/cpsw.c >> +++ b/drivers/net/ethernet/ti/cpsw.c >> @@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev) >> struct sk_buff *skb; >> ret = -ENOMEM; >> - skb = netdev_alloc_skb_ip_align(priv->ndev, >> - priv->rx_packet_max); >> + skb = __netdev_alloc_skb_ip_align(priv->ndev, >> + priv->rx_packet_max, GFP_KERNEL); >> if (!skb) >> - break; >> + goto err_cleanup; >> ret = cpdma_chan_submit(priv->rxch, skb, skb->data, >> skb_tailroom(skb), 0, GFP_KERNEL); >> - if (WARN_ON(ret < 0)) >> - break; >> + if (ret < 0) { >> + kfree_skb(skb); >> + goto err_cleanup; > Why you need to close the device even you have some skb allocated and > submitted successfully. Can allow the device to continue with lower > performance Because this should not happen. If you run out-of-memory because an application is going crazy than you won't have much anyway. If you configured too much skbs then this should be fixed as well. >> + } >> } >> /* continue even if we didn't manage to submit all >> * receive descs >> @@ -944,6 +946,10 @@ static int cpsw_ndo_open(struct net_device *ndev) >> if (priv->data.dual_emac) >> priv->slaves[priv->emac_port].open_stat = true; >> return 0; >> + >> +err_cleanup: >> + cpdma_ctlr_stop(priv->dma); >> + return ret; >> } > only cpdma is halted and allocated skb are released, need to have other > calls like pm_runtime_put_sync, close host and slave ports Okay, will fix. > > Regards > Mugunthan V N Sebastian -- 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
On 4/18/2013 5:39 PM, Sebastian Andrzej Siewior wrote: > On 04/18/2013 01:50 PM, Mugunthan V N wrote: > >>> diff --git a/drivers/net/ethernet/ti/cpsw.c >>> b/drivers/net/ethernet/ti/cpsw.c >>> index e2ba702..3b22a36 100644 >>> --- a/drivers/net/ethernet/ti/cpsw.c >>> +++ b/drivers/net/ethernet/ti/cpsw.c >>> @@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev) >>> struct sk_buff *skb; >>> ret = -ENOMEM; >>> - skb = netdev_alloc_skb_ip_align(priv->ndev, >>> - priv->rx_packet_max); >>> + skb = __netdev_alloc_skb_ip_align(priv->ndev, >>> + priv->rx_packet_max, GFP_KERNEL); >>> if (!skb) >>> - break; >>> + goto err_cleanup; >>> ret = cpdma_chan_submit(priv->rxch, skb, skb->data, >>> skb_tailroom(skb), 0, GFP_KERNEL); >>> - if (WARN_ON(ret < 0)) >>> - break; >>> + if (ret < 0) { >>> + kfree_skb(skb); >>> + goto err_cleanup; >> Why you need to close the device even you have some skb allocated and >> submitted successfully. Can allow the device to continue with lower >> performance > Because this should not happen. If you run out-of-memory because an > application is going crazy than you won't have much anyway. If you > configured too much skbs then this should be fixed as well. > But i am seeing most of the drivers allowing to open the device with lesser rx skb count? But i don't know where this has been changed recently, may be some other network experts can comment on this. Regards Mugunthan V N -- 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
On 04/19/2013 12:40 PM, Mugunthan V N wrote: > But i am seeing most of the drivers allowing to open the device with lesser > rx skb count? But i don't know where this has been changed recently, may > be some other network experts can comment on this. If you configure a special value in your device tree you should obey it or else you won't the same result. > > Regards > Mugunthan V N Sebastian -- 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/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index e2ba702..3b22a36 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev) struct sk_buff *skb; ret = -ENOMEM; - skb = netdev_alloc_skb_ip_align(priv->ndev, - priv->rx_packet_max); + skb = __netdev_alloc_skb_ip_align(priv->ndev, + priv->rx_packet_max, GFP_KERNEL); if (!skb) - break; + goto err_cleanup; ret = cpdma_chan_submit(priv->rxch, skb, skb->data, skb_tailroom(skb), 0, GFP_KERNEL); - if (WARN_ON(ret < 0)) - break; + if (ret < 0) { + kfree_skb(skb); + goto err_cleanup; + } } /* continue even if we didn't manage to submit all * receive descs @@ -944,6 +946,10 @@ static int cpsw_ndo_open(struct net_device *ndev) if (priv->data.dual_emac) priv->slaves[priv->emac_port].open_stat = true; return 0; + +err_cleanup: + cpdma_ctlr_stop(priv->dma); + return ret; } static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
if during "ifconfig up" we run out of mem we continue regardless how many skbs we got. In worst case we have zero RX skbs and can't ever receive further packets since the RX skbs are never reallocated. If cpdma_chan_submit() fails we even leak the skb. This patch changes the behavior here: If we fail to allocate an skb during bring up we don't continue and report that error. Same goes for errors from cpdma_chan_submit(). While here I changed to __netdev_alloc_skb_ip_align() so GFP_KERNEL can be used. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/net/ethernet/ti/cpsw.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)