Re: [PATCH] loopback: better handling of packet drops

[Stephen Hemminger <shemminger@vyatta.com>]

On Fri, 17 Apr 2009 12:33:33 +0200
Eric Dumazet <dada1@cosmosbay.com> wrote:

> Eric Dumazet a écrit :
> > Eric Dumazet a écrit :
> >> David Miller a écrit :
> >>> From: Eric Dumazet <dada1@cosmosbay.com>
> >>> Date: Fri, 17 Apr 2009 10:56:57 +0200
> >>>
> >>>> We can in some situations drop packets in netif_rx()
> >>>>
> >>>> loopback driver does not report these (unlikely) drops to its stats,
> >>>> and incorrectly change packets/bytes counts.
> >>>>
> >>>> After this patch applied, "ifconfig lo" can reports these drops as in :
> >>>>
> >>>> # ifconfig lo
> >>>> lo        Link encap:Local Loopback
> >>>>           inet addr:127.0.0.1  Mask:255.0.0.0
> >>>>           UP LOOPBACK RUNNING  MTU:16436  Metric:1
> >>>>           RX packets:692562900 errors:0 dropped:0 overruns:0 frame:0
> >>>>           TX packets:692562900 errors:3228 dropped:3228 overruns:0 carrier:0
> >>>>           collisions:0 txqueuelen:0
> >>>>           RX bytes:2865674174 (2.6 GiB)  TX bytes:2865674174 (2.6 GiB)
> >>>>
> >>>> I chose to reflect those errors only in tx_dropped/tx_errors, and not mirror
> >>>> these errors in rx_dropped/rx_errors.
> >>>>
> >>>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> >>> Well, logically the receive is what failed, not the transmit.
> >>>
> >>> I think it's therefore misleading to count it as a TX drop.
> >>>
> >>> Do you feel strongly about this?
> >> Not at all, but my plan was to go a litle bit further, ie being able to 
> >> return from loopback_xmit() with a non null value.
> >>
> > 
> > Something like this :
> 
> I just noticed NETDEV_TX_BUSY & NETDEV_TX_OK, so here is an updated version
> using these macros instead of 0 & 1
> 
> [PATCH] loopback: better handling of packet drops
> 
> We can in some situations drop packets in netif_rx()
> 
> loopback driver does not report these (unlikely) drops to its stats,
> and incorrectly change packets/bytes counts. Also upper layers are
> not warned of these transmit failures.
> 
> After this patch applied, "ifconfig lo" can reports these drops as in :
> 
> # ifconfig lo
> lo        Link encap:Local Loopback
>           inet addr:127.0.0.1  Mask:255.0.0.0
>           UP LOOPBACK RUNNING  MTU:16436  Metric:1
>           RX packets:692562900 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:692562900 errors:3228 dropped:3228 overruns:0 carrier:0
>           collisions:0 txqueuelen:0
>           RX bytes:2865674174 (2.6 GiB)  TX bytes:2865674174 (2.6 GiB)
> 
> More over, loopback_xmit() can now return to its caller the indication that
> packet was not transmitted for better queue management and error handling.
> 
> I chose to reflect those errors only in tx_dropped/tx_errors, and not mirror
> them in rx_dropped/rx_errors.
> 
> Splitting netif_rx() with a helper function boosts tbench performance by 1%,
> because we can avoid two tests (about netpoll and timestamping)
> 
> Tested with /proc/sys/net/core/netdev_max_backlog set to 0, tbench
> can run at full speed even with some 'losses' on loopback. No more
> tcp stalls...
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> ---
>  drivers/net/loopback.c    |   24 +++++++++---
>  include/linux/netdevice.h |    1
>  net/core/dev.c            |   68 +++++++++++++++++++++++-------------
>  3 files changed, 62 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index b7d438a..101a3bc 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -62,6 +62,7 @@
>  struct pcpu_lstats {
>  	unsigned long packets;
>  	unsigned long bytes;
> +	unsigned long drops;
>  };
>  
>  /*
> @@ -71,20 +72,25 @@ struct pcpu_lstats {
>  static int loopback_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct pcpu_lstats *pcpu_lstats, *lb_stats;
> +	int len;
>  
>  	skb_orphan(skb);
>  
> -	skb->protocol = eth_type_trans(skb,dev);
> +	skb->protocol = eth_type_trans(skb, dev);
>  
>  	/* it's OK to use per_cpu_ptr() because BHs are off */
>  	pcpu_lstats = dev->ml_priv;
>  	lb_stats = per_cpu_ptr(pcpu_lstats, smp_processor_id());
> -	lb_stats->bytes += skb->len;
> -	lb_stats->packets++;
>  
> -	netif_rx(skb);
> +	len = skb->len;
> +	if (likely(__netif_rx(skb) == NET_RX_SUCCESS)) {
> +		lb_stats->bytes += len;
> +		lb_stats->packets++;
> +		return NETDEV_TX_OK;
> +	}
> +	lb_stats->drops++;
>  
> -	return 0;
> +	return NETDEV_TX_BUSY;
>  }

If you return NETDEV_TX_BUSY, then the xmit logic will retry
so it is not really a drop but a stall. I think it is confusing
to call this a packet loss.