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

[Eric Dumazet <eric.dumazet@gmail.com>]

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.