Patchwork [5/5] net/cpsw: redo rx skb allocation in rx path

login
register
mail settings
Submitter Sebastian Siewior
Date April 17, 2013, 9:52 p.m.
Message ID <1366235536-15744-6-git-send-email-bigeasy@linutronix.de>
Download mbox | patch
Permalink /patch/237372/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Sebastian Siewior - April 17, 2013, 9:52 p.m.
In case that we run into OOM during the allocation of the new rx-skb we
don't get one and we have one skb less than we used to have. If this
continues to happen then we end up with no rx-skbs at all.
This patch changes the following:
- if we fail to allocate the new skb, then we treat the currently
  completed skb as the new one and so drop the currently received data.
- instead of testing multiple times if the device is gone we rely one
  the status field which is set to -ENOSYS in case the channel is going
  down and incomplete requests are purged.
  cpdma_chan_stop() removes most of the packages with -ENOSYS. The
  currently active packet which is removed has the "tear down" bit set.
  So if that bit is set, we send ENOSYS as well otherwise we pass the
  status bits which are required to figure out which of the two possible
  just finished.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c          |   33 ++++++++++++-------------------
 drivers/net/ethernet/ti/davinci_cpdma.c |    7 ++++++-
 2 files changed, 19 insertions(+), 21 deletions(-)
Mugunthan V N - April 18, 2013, 11:50 a.m.
On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> In case that we run into OOM during the allocation of the new rx-skb we
> don't get one and we have one skb less than we used to have. If this
> continues to happen then we end up with no rx-skbs at all.
> This patch changes the following:
> - if we fail to allocate the new skb, then we treat the currently
>    completed skb as the new one and so drop the currently received data.
> - instead of testing multiple times if the device is gone we rely one
>    the status field which is set to -ENOSYS in case the channel is going
>    down and incomplete requests are purged.
>    cpdma_chan_stop() removes most of the packages with -ENOSYS. The
>    currently active packet which is removed has the "tear down" bit set.
>    So if that bit is set, we send ENOSYS as well otherwise we pass the
>    status bits which are required to figure out which of the two possible
>    just finished.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   drivers/net/ethernet/ti/cpsw.c          |   33 ++++++++++++-------------------
>   drivers/net/ethernet/ti/davinci_cpdma.c |    7 ++++++-
>   2 files changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 559b020..f684e9b 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -469,43 +469,36 @@ void cpsw_tx_handler(void *token, int len, int status)
>   void cpsw_rx_handler(void *token, int len, int status)
>   {
>   	struct sk_buff		*skb = token;
> +	struct sk_buff		*new_skb;
>   	struct net_device	*ndev = skb->dev;
>   	struct cpsw_priv	*priv = netdev_priv(ndev);
>   	int			ret = 0;
>   
>   	cpsw_dual_emac_src_port_detect(status, priv, ndev, skb);
>   
> -	/* free and bail if we are shutting down */
> -	if (unlikely(!netif_running(ndev)) ||
> -			unlikely(!netif_carrier_ok(ndev))) {
> +	if (unlikely(status < 0)) {
> +		/* the interface is going down, skbs are purged */
>   		dev_kfree_skb_any(skb);
>   		return;
>   	}
> -	if (likely(status >= 0)) {
> +
> +	new_skb = netdev_alloc_skb_ip_align(ndev, priv->rx_packet_max);
> +	if (new_skb) {
>   		skb_put(skb, len);
>   		cpts_rx_timestamp(priv->cpts, skb);
>   		skb->protocol = eth_type_trans(skb, ndev);
>   		netif_receive_skb(skb);
>   		priv->stats.rx_bytes += len;
>   		priv->stats.rx_packets++;
> -		skb = NULL;
> -	}
> -
> -	if (unlikely(!netif_running(ndev))) {
> -		if (skb)
> -			dev_kfree_skb_any(skb);
> -		return;
> +	} else {
> +		priv->stats.rx_dropped++;
> +		new_skb = skb;
Why you want to drop a successfully received packet as you memory alloc 
failed?
Let the stack get it processed and there after you can continue with one 
less
rx skb
>   	}
>   
> -	if (likely(!skb)) {
> -		skb = netdev_alloc_skb_ip_align(ndev, priv->rx_packet_max);
> -		if (WARN_ON(!skb))
> -			return;
> -
> -		ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
> -					skb_tailroom(skb), 0);
> -	}
> -	WARN_ON(ret < 0);
> +	ret = cpdma_chan_submit(priv->rxch, new_skb, new_skb->data,
> +			skb_tailroom(new_skb), 0);
> +	if (WARN_ON(ret < 0))
> +		dev_kfree_skb_any(new_skb);
>   }
>   
>   static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 3cc20e7..6b0a89f 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -776,6 +776,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
>   	struct cpdma_ctlr		*ctlr = chan->ctlr;
>   	struct cpdma_desc __iomem	*desc;
>   	int				status, outlen;
> +	int				cb_status = 0;
>   	struct cpdma_desc_pool		*pool = ctlr->pool;
>   	dma_addr_t			desc_dma;
>   	unsigned long			flags;
> @@ -811,8 +812,12 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
>   	}
>   
>   	spin_unlock_irqrestore(&chan->lock, flags);
> +	if (unlikely(status & CPDMA_DESC_TD_COMPLETE))
> +		cb_status = -ENOSYS;
> +	else
> +		cb_status = status;
>   
> -	__cpdma_chan_free(chan, desc, outlen, status);
> +	__cpdma_chan_free(chan, desc, outlen, cb_status);
>   	return status;
>   
>   unlock_ret:

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
Sebastian Siewior - April 18, 2013, 12:13 p.m.
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 559b020..f684e9b 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -469,43 +469,36 @@ void cpsw_tx_handler(void *token, int len, int
<snip>
>> -    if (unlikely(!netif_running(ndev))) {
>> -        if (skb)
>> -            dev_kfree_skb_any(skb);
>> -        return;
>> +    } else {
>> +        priv->stats.rx_dropped++;
>> +        new_skb = skb;
> Why you want to drop a successfully received packet as you memory alloc
> failed?
> Let the stack get it processed and there after you can continue with one
> less
> rx skb

Because, as I wrote, if this continues you end up without rx skbs.
However, if you if you are under memory pressure it is possible that
other callers in the stack have the same problem.

> 
> 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
Eric Dumazet - April 18, 2013, 2:59 p.m.
On Thu, 2013-04-18 at 17:20 +0530, Mugunthan V N wrote:
> On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> > In case that we run into OOM during the allocation of the new rx-skb we
> > don't get one and we have one skb less than we used to have. If this
> > continues to happen then we end up with no rx-skbs at all.
> > This patch changes the following:
> > - if we fail to allocate the new skb, then we treat the currently
> >    completed skb as the new one and so drop the currently received data.
> > - instead of testing multiple times if the device is gone we rely one
> >    the status field which is set to -ENOSYS in case the channel is going
> >    down and incomplete requests are purged.
> >    cpdma_chan_stop() removes most of the packages with -ENOSYS. The
> >    currently active packet which is removed has the "tear down" bit set.
> >    So if that bit is set, we send ENOSYS as well otherwise we pass the
> >    status bits which are required to figure out which of the two possible
> >    just finished.
> >
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >   drivers/net/ethernet/ti/cpsw.c          |   33 ++++++++++++-------------------
> >   drivers/net/ethernet/ti/davinci_cpdma.c |    7 ++++++-
> >   2 files changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 559b020..f684e9b 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -469,43 +469,36 @@ void cpsw_tx_handler(void *token, int len, int status)
> >   void cpsw_rx_handler(void *token, int len, int status)
> >   {
> >   	struct sk_buff		*skb = token;
> > +	struct sk_buff		*new_skb;
> >   	struct net_device	*ndev = skb->dev;
> >   	struct cpsw_priv	*priv = netdev_priv(ndev);
> >   	int			ret = 0;
> >   
> >   	cpsw_dual_emac_src_port_detect(status, priv, ndev, skb);
> >   
> > -	/* free and bail if we are shutting down */
> > -	if (unlikely(!netif_running(ndev)) ||
> > -			unlikely(!netif_carrier_ok(ndev))) {
> > +	if (unlikely(status < 0)) {
> > +		/* the interface is going down, skbs are purged */
> >   		dev_kfree_skb_any(skb);
> >   		return;
> >   	}
> > -	if (likely(status >= 0)) {
> > +
> > +	new_skb = netdev_alloc_skb_ip_align(ndev, priv->rx_packet_max);
> > +	if (new_skb) {
> >   		skb_put(skb, len);
> >   		cpts_rx_timestamp(priv->cpts, skb);
> >   		skb->protocol = eth_type_trans(skb, ndev);
> >   		netif_receive_skb(skb);
> >   		priv->stats.rx_bytes += len;
> >   		priv->stats.rx_packets++;
> > -		skb = NULL;
> > -	}
> > -
> > -	if (unlikely(!netif_running(ndev))) {
> > -		if (skb)
> > -			dev_kfree_skb_any(skb);
> > -		return;
> > +	} else {
> > +		priv->stats.rx_dropped++;
> > +		new_skb = skb;
> Why you want to drop a successfully received packet as you memory alloc 
> failed?
> Let the stack get it processed and there after you can continue with one 
> less
> rx skb

Have you read the changelog at all ?

This patch sounds quite correct to me.

If you cannot allocate a new skb, then drop the incoming frame, instead
of dealing with strange allocation retries later.



--
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
Mugunthan V N - April 19, 2013, 10:28 a.m.
On 4/18/2013 5:43 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 559b020..f684e9b 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -469,43 +469,36 @@ void cpsw_tx_handler(void *token, int len, int
> <snip>
>>> -    if (unlikely(!netif_running(ndev))) {
>>> -        if (skb)
>>> -            dev_kfree_skb_any(skb);
>>> -        return;
>>> +    } else {
>>> +        priv->stats.rx_dropped++;
>>> +        new_skb = skb;
>> Why you want to drop a successfully received packet as you memory alloc
>> failed?
>> Let the stack get it processed and there after you can continue with one
>> less
>> rx skb
> Because, as I wrote, if this continues you end up without rx skbs.
> However, if you if you are under memory pressure it is possible that
> other callers in the stack have the same problem.
>
I had gone through some other drivers and found that this patch is valid 
so add

Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

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

Patch

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 559b020..f684e9b 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -469,43 +469,36 @@  void cpsw_tx_handler(void *token, int len, int status)
 void cpsw_rx_handler(void *token, int len, int status)
 {
 	struct sk_buff		*skb = token;
+	struct sk_buff		*new_skb;
 	struct net_device	*ndev = skb->dev;
 	struct cpsw_priv	*priv = netdev_priv(ndev);
 	int			ret = 0;
 
 	cpsw_dual_emac_src_port_detect(status, priv, ndev, skb);
 
-	/* free and bail if we are shutting down */
-	if (unlikely(!netif_running(ndev)) ||
-			unlikely(!netif_carrier_ok(ndev))) {
+	if (unlikely(status < 0)) {
+		/* the interface is going down, skbs are purged */
 		dev_kfree_skb_any(skb);
 		return;
 	}
-	if (likely(status >= 0)) {
+
+	new_skb = netdev_alloc_skb_ip_align(ndev, priv->rx_packet_max);
+	if (new_skb) {
 		skb_put(skb, len);
 		cpts_rx_timestamp(priv->cpts, skb);
 		skb->protocol = eth_type_trans(skb, ndev);
 		netif_receive_skb(skb);
 		priv->stats.rx_bytes += len;
 		priv->stats.rx_packets++;
-		skb = NULL;
-	}
-
-	if (unlikely(!netif_running(ndev))) {
-		if (skb)
-			dev_kfree_skb_any(skb);
-		return;
+	} else {
+		priv->stats.rx_dropped++;
+		new_skb = skb;
 	}
 
-	if (likely(!skb)) {
-		skb = netdev_alloc_skb_ip_align(ndev, priv->rx_packet_max);
-		if (WARN_ON(!skb))
-			return;
-
-		ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
-					skb_tailroom(skb), 0);
-	}
-	WARN_ON(ret < 0);
+	ret = cpdma_chan_submit(priv->rxch, new_skb, new_skb->data,
+			skb_tailroom(new_skb), 0);
+	if (WARN_ON(ret < 0))
+		dev_kfree_skb_any(new_skb);
 }
 
 static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 3cc20e7..6b0a89f 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -776,6 +776,7 @@  static int __cpdma_chan_process(struct cpdma_chan *chan)
 	struct cpdma_ctlr		*ctlr = chan->ctlr;
 	struct cpdma_desc __iomem	*desc;
 	int				status, outlen;
+	int				cb_status = 0;
 	struct cpdma_desc_pool		*pool = ctlr->pool;
 	dma_addr_t			desc_dma;
 	unsigned long			flags;
@@ -811,8 +812,12 @@  static int __cpdma_chan_process(struct cpdma_chan *chan)
 	}
 
 	spin_unlock_irqrestore(&chan->lock, flags);
+	if (unlikely(status & CPDMA_DESC_TD_COMPLETE))
+		cb_status = -ENOSYS;
+	else
+		cb_status = status;
 
-	__cpdma_chan_free(chan, desc, outlen, status);
+	__cpdma_chan_free(chan, desc, outlen, cb_status);
 	return status;
 
 unlock_ret: