[PATCH net-next] r8169: Add 64bit statistics

[PATCH net-next] r8169: Add 64bit statistics

[Junchang Wang]

I submitted this two months ago. This is a resubmission. Thanks.

Switch to use ndo_get_stats64 to get 64bit statistics.
Two sync entries are used (one for Rx and one for Tx).

Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c |   61 +++++++++++++++++++++++++++++-----
 1 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index fbd855b..c1e2421 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -675,6 +675,12 @@ enum rtl_flag {
 	RTL_FLAG_MAX
 };
 
+struct rtl8169_stats {
+	struct u64_stats_sync	syncp;
+	u64			packets;
+	u64			bytes;
+};
+
 struct rtl8169_private {
 	void __iomem *mmio_addr;	/* memory map physical address */
 	struct pci_dev *pci_dev;
@@ -689,6 +695,8 @@ struct rtl8169_private {
 	u32 dirty_tx;
 	struct TxDesc *TxDescArray;	/* 256-aligned Tx descriptor ring */
 	struct RxDesc *RxDescArray;	/* 256-aligned Rx descriptor ring */
+	struct rtl8169_stats rx_stats;
+	struct rtl8169_stats tx_stats;
 	dma_addr_t TxPhyAddr;
 	dma_addr_t RxPhyAddr;
 	void *Rx_databuff[NUM_RX_DESC];	/* Rx data buffers */
@@ -775,7 +783,9 @@ static void rtl_hw_start(struct net_device *dev);
 static int rtl8169_close(struct net_device *dev);
 static void rtl_set_rx_mode(struct net_device *dev);
 static void rtl8169_tx_timeout(struct net_device *dev);
-static struct net_device_stats *rtl8169_get_stats(struct net_device *dev);
+static struct rtnl_link_stats64 *rtl8169_get_stats64(struct net_device *dev,
+						    struct rtnl_link_stats64
+						    *stats);
 static int rtl8169_change_mtu(struct net_device *dev, int new_mtu);
 static void rtl8169_rx_clear(struct rtl8169_private *tp);
 static int rtl8169_poll(struct napi_struct *napi, int budget);
@@ -3521,7 +3531,7 @@ static void rtl_disable_msi(struct pci_dev *pdev, struct rtl8169_private *tp)
 static const struct net_device_ops rtl8169_netdev_ops = {
 	.ndo_open		= rtl8169_open,
 	.ndo_stop		= rtl8169_close,
-	.ndo_get_stats		= rtl8169_get_stats,
+	.ndo_get_stats64	= rtl8169_get_stats64,
 	.ndo_start_xmit		= rtl8169_start_xmit,
 	.ndo_tx_timeout		= rtl8169_tx_timeout,
 	.ndo_validate_addr	= eth_validate_addr,
@@ -5658,8 +5668,10 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			u64_stats_update_begin(&tp->tx_stats.syncp);
+			tp->tx_stats.packets++;
+			tp->tx_stats.bytes += tx_skb->skb->len;
+			u64_stats_update_end(&tp->tx_stats.syncp);
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
@@ -5807,8 +5819,10 @@ process_pkt:
 
 			napi_gro_receive(&tp->napi, skb);
 
-			dev->stats.rx_bytes += pkt_size;
-			dev->stats.rx_packets++;
+			u64_stats_update_begin(&tp->rx_stats.syncp);
+			tp->rx_stats.packets++;
+			tp->rx_stats.bytes += pkt_size;
+			u64_stats_update_end(&tp->rx_stats.syncp);
 		}
 
 		/* Work around for AMD plateform. */
@@ -6070,20 +6084,49 @@ static void rtl_set_rx_mode(struct net_device *dev)
 }
 
 /**
- *  rtl8169_get_stats - Get rtl8169 read/write statistics
+ *  rtl8169_get_stats64 - Get rtl8169 read/write statistics
  *  @dev: The Ethernet Device to get statistics for
  *
  *  Get TX/RX statistics for rtl8169
  */
-static struct net_device_stats *rtl8169_get_stats(struct net_device *dev)
+static struct rtnl_link_stats64 *
+rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
+	u64 _packets, _bytes;
+	unsigned int start;
 
 	if (netif_running(dev))
 		rtl8169_rx_missed(dev, ioaddr);
 
-	return &dev->stats;
+	do {
+		start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
+		_packets = tp->rx_stats.packets;
+		_bytes = tp->rx_stats.bytes;
+	} while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));
+
+	stats->rx_packets = _packets;
+	stats->rx_bytes	= _bytes;
+
+	do {
+		start = u64_stats_fetch_begin_bh(&tp->tx_stats.syncp);
+		_packets = tp->tx_stats.packets;
+		_bytes = tp->tx_stats.bytes;
+	} while (u64_stats_fetch_retry_bh(&tp->tx_stats.syncp, start));
+
+	stats->tx_packets = _packets;
+	stats->tx_bytes	= _bytes;
+
+	stats->rx_dropped	= dev->stats.rx_dropped;
+	stats->tx_dropped	= dev->stats.tx_dropped;
+	stats->rx_length_errors = dev->stats.rx_length_errors;
+	stats->rx_errors	= dev->stats.rx_errors;
+	stats->rx_crc_errors	= dev->stats.rx_crc_errors;
+	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
+	stats->rx_missed_errors = dev->stats.rx_missed_errors;
+
+	return stats;
 }
 
 static void rtl8169_net_suspend(struct net_device *dev)
--

Re: [PATCH net-next] r8169: Add 64bit statistics

[Eric Dumazet]

Le dimanche 04 mars 2012 à 17:37 +0800, Junchang Wang a écrit :
> I submitted this two months ago. This is a resubmission. Thanks.
> 
> Switch to use ndo_get_stats64 to get 64bit statistics.
> Two sync entries are used (one for Rx and one for Tx).
> 
> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c |   61 +++++++++++++++++++++++++++++-----
>  1 files changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index fbd855b..c1e2421 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -675,6 +675,12 @@ enum rtl_flag {
>  	RTL_FLAG_MAX
>  };
>  
> +struct rtl8169_stats {
> +	struct u64_stats_sync	syncp;

small point : Using this means you have a 32bit hole here (on 32bit
build). Its minor, you dont need to change.

> +	u64			packets;
> +	u64			bytes;
> +};
> +
>  struct rtl8169_private {
>  	void __iomem *mmio_addr;	/* memory map physical address */
>  	struct pci_dev *pci_dev;
> @@ -689,6 +695,8 @@ struct rtl8169_private {
>  	u32 dirty_tx;
>  	struct TxDesc *TxDescArray;	/* 256-aligned Tx descriptor ring */
>  	struct RxDesc *RxDescArray;	/* 256-aligned Rx descriptor ring */
> +	struct rtl8169_stats rx_stats;
> +	struct rtl8169_stats tx_stats;

	You could try to put these somewhere else, to try to keep this portion
as read only memory, to be more SMP friendly.
(Some loaded server could have one CPU serving RX stuff, and other cpus
doing TX stuff) 

>  	dma_addr_t TxPhyAddr;
>  	dma_addr_t RxPhyAddr;
>  	void *Rx_databuff[NUM_RX_DESC];	/* Rx data buffers */
> @@ -775,7 +783,9 @@ static void rtl_hw_start(struct net_device *dev);
>  static int rtl8169_close(struct net_device *dev);
>  static void rtl_set_rx_mode(struct net_device *dev);
>  static void rtl8169_tx_timeout(struct net_device *dev);
> -static struct net_device_stats *rtl8169_get_stats(struct net_device *dev);
> +static struct rtnl_link_stats64 *rtl8169_get_stats64(struct net_device *dev,
> +						    struct rtnl_link_stats64
> +						    *stats);
>  static int rtl8169_change_mtu(struct net_device *dev, int new_mtu);
>  static void rtl8169_rx_clear(struct rtl8169_private *tp);
>  static int rtl8169_poll(struct napi_struct *napi, int budget);
> @@ -3521,7 +3531,7 @@ static void rtl_disable_msi(struct pci_dev *pdev, struct rtl8169_private *tp)
>  static const struct net_device_ops rtl8169_netdev_ops = {
>  	.ndo_open		= rtl8169_open,
>  	.ndo_stop		= rtl8169_close,
> -	.ndo_get_stats		= rtl8169_get_stats,
> +	.ndo_get_stats64	= rtl8169_get_stats64,
>  	.ndo_start_xmit		= rtl8169_start_xmit,
>  	.ndo_tx_timeout		= rtl8169_tx_timeout,
>  	.ndo_validate_addr	= eth_validate_addr,
> @@ -5658,8 +5668,10 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
>  		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
>  				     tp->TxDescArray + entry);
>  		if (status & LastFrag) {
> -			dev->stats.tx_packets++;
> -			dev->stats.tx_bytes += tx_skb->skb->len;
> +			u64_stats_update_begin(&tp->tx_stats.syncp);
> +			tp->tx_stats.packets++;
> +			tp->tx_stats.bytes += tx_skb->skb->len;
> +			u64_stats_update_end(&tp->tx_stats.syncp);
>  			dev_kfree_skb(tx_skb->skb);
>  			tx_skb->skb = NULL;
>  		}
> @@ -5807,8 +5819,10 @@ process_pkt:
>  
>  			napi_gro_receive(&tp->napi, skb);
>  
> -			dev->stats.rx_bytes += pkt_size;
> -			dev->stats.rx_packets++;
> +			u64_stats_update_begin(&tp->rx_stats.syncp);
> +			tp->rx_stats.packets++;
> +			tp->rx_stats.bytes += pkt_size;
> +			u64_stats_update_end(&tp->rx_stats.syncp);
>  		}
>  
>  		/* Work around for AMD plateform. */
> @@ -6070,20 +6084,49 @@ static void rtl_set_rx_mode(struct net_device *dev)
>  }
>  
>  /**
> - *  rtl8169_get_stats - Get rtl8169 read/write statistics
> + *  rtl8169_get_stats64 - Get rtl8169 read/write statistics
>   *  @dev: The Ethernet Device to get statistics for

missing @stats

>   *
>   *  Get TX/RX statistics for rtl8169
>   */
> -static struct net_device_stats *rtl8169_get_stats(struct net_device *dev)
> +static struct rtnl_link_stats64 *
> +rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  	void __iomem *ioaddr = tp->mmio_addr;
> +	u64 _packets, _bytes;
> +	unsigned int start;
>  
>  	if (netif_running(dev))
>  		rtl8169_rx_missed(dev, ioaddr);
>  
> -	return &dev->stats;
> +	do {
> +		start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
> +		_packets = tp->rx_stats.packets;
> +		_bytes = tp->rx_stats.bytes;
> +	} while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));
> +
> +	stats->rx_packets = _packets;
> +	stats->rx_bytes	= _bytes;

You dont need _bytes and _packets temp variables, as @stats points to a
private memory, we can use it as working storage, just do :

       do {
               start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
               stats->rx_packets = tp->rx_stats.packets;
               stats->rx_bytes = tp->rx_stats.bytes;
       } while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));


> +
> +	do {
> +		start = u64_stats_fetch_begin_bh(&tp->tx_stats.syncp);
> +		_packets = tp->tx_stats.packets;
> +		_bytes = tp->tx_stats.bytes;
> +	} while (u64_stats_fetch_retry_bh(&tp->tx_stats.syncp, start));

same here.

> +
> +	stats->tx_packets = _packets;
> +	stats->tx_bytes	= _bytes;
> +

Re: [PATCH net-next] r8169: Add 64bit statistics

[Francois Romieu]

Eric Dumazet <eric.dumazet@gmail.com> :
[...]
> small point : Using this means you have a 32bit hole here (on 32bit
> build). Its minor, you dont need to change.

Ok.

[...]
> 	You could try to put these somewhere else, to try to keep this portion
> as read only memory, to be more SMP friendly.
> (Some loaded server could have one CPU serving RX stuff, and other cpus
> doing TX stuff) 

Point taken. It could make sense to rework the rtl8169_private struct a bit
more.

[...]
> > @@ -6070,20 +6084,49 @@ static void rtl_set_rx_mode(struct net_device *dev)
> >  }
> >  
> >  /**
> > - *  rtl8169_get_stats - Get rtl8169 read/write statistics
> > + *  rtl8169_get_stats64 - Get rtl8169 read/write statistics
> >   *  @dev: The Ethernet Device to get statistics for
> 
> missing @stats

This documentation is almost useless anyway. I removed it.

[...]
> You dont need _bytes and _packets temp variables, as @stats points to a
> private memory, we can use it as working storage, just do :
> 
>        do {
>                start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
>                stats->rx_packets = tp->rx_stats.packets;
>                stats->rx_bytes = tp->rx_stats.bytes;
>        } while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));

It should give something like the patch below.

If I understand correctly we do not care much about the error counters,
right ?

From: Junchang Wang <junchangwang@gmail.com>
Date: Sun, 4 Mar 2012 23:30:32 +0100
Subject: [PATCH 1/2] r8169: add 64bit statistics.

Switch to use ndo_get_stats64 to get 64bit statistics.
Two sync entries are used (one for Rx and one for Tx).

Signed-off-by: Junchang Wang <junchangwang@gmail.com>
Reviewed-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/realtek/r8169.c |   59 ++++++++++++++++++++++++++--------
 1 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index fbd855b..a4d7674 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -675,6 +675,12 @@ enum rtl_flag {
 	RTL_FLAG_MAX
 };
 
+struct rtl8169_stats {
+	u64			packets;
+	u64			bytes;
+	struct u64_stats_sync	syncp;
+};
+
 struct rtl8169_private {
 	void __iomem *mmio_addr;	/* memory map physical address */
 	struct pci_dev *pci_dev;
@@ -687,6 +693,8 @@ struct rtl8169_private {
 	u32 cur_tx; /* Index into the Tx descriptor buffer of next Rx pkt. */
 	u32 dirty_rx;
 	u32 dirty_tx;
+	struct rtl8169_stats rx_stats;
+	struct rtl8169_stats tx_stats;
 	struct TxDesc *TxDescArray;	/* 256-aligned Tx descriptor ring */
 	struct RxDesc *RxDescArray;	/* 256-aligned Rx descriptor ring */
 	dma_addr_t TxPhyAddr;
@@ -775,7 +783,9 @@ static void rtl_hw_start(struct net_device *dev);
 static int rtl8169_close(struct net_device *dev);
 static void rtl_set_rx_mode(struct net_device *dev);
 static void rtl8169_tx_timeout(struct net_device *dev);
-static struct net_device_stats *rtl8169_get_stats(struct net_device *dev);
+static struct rtnl_link_stats64 *rtl8169_get_stats64(struct net_device *dev,
+						    struct rtnl_link_stats64
+						    *stats);
 static int rtl8169_change_mtu(struct net_device *dev, int new_mtu);
 static void rtl8169_rx_clear(struct rtl8169_private *tp);
 static int rtl8169_poll(struct napi_struct *napi, int budget);
@@ -3521,7 +3531,7 @@ static void rtl_disable_msi(struct pci_dev *pdev, struct rtl8169_private *tp)
 static const struct net_device_ops rtl8169_netdev_ops = {
 	.ndo_open		= rtl8169_open,
 	.ndo_stop		= rtl8169_close,
-	.ndo_get_stats		= rtl8169_get_stats,
+	.ndo_get_stats64	= rtl8169_get_stats64,
 	.ndo_start_xmit		= rtl8169_start_xmit,
 	.ndo_tx_timeout		= rtl8169_tx_timeout,
 	.ndo_validate_addr	= eth_validate_addr,
@@ -5658,8 +5668,10 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			u64_stats_update_begin(&tp->tx_stats.syncp);
+			tp->tx_stats.packets++;
+			tp->tx_stats.bytes += tx_skb->skb->len;
+			u64_stats_update_end(&tp->tx_stats.syncp);
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
@@ -5807,8 +5819,10 @@ process_pkt:
 
 			napi_gro_receive(&tp->napi, skb);
 
-			dev->stats.rx_bytes += pkt_size;
-			dev->stats.rx_packets++;
+			u64_stats_update_begin(&tp->rx_stats.syncp);
+			tp->rx_stats.packets++;
+			tp->rx_stats.bytes += pkt_size;
+			u64_stats_update_end(&tp->rx_stats.syncp);
 		}
 
 		/* Work around for AMD plateform. */
@@ -6069,21 +6083,38 @@ static void rtl_set_rx_mode(struct net_device *dev)
 	RTL_W32(RxConfig, tmp);
 }
 
-/**
- *  rtl8169_get_stats - Get rtl8169 read/write statistics
- *  @dev: The Ethernet Device to get statistics for
- *
- *  Get TX/RX statistics for rtl8169
- */
-static struct net_device_stats *rtl8169_get_stats(struct net_device *dev)
+static struct rtnl_link_stats64 *
+rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
+	unsigned int start;
 
 	if (netif_running(dev))
 		rtl8169_rx_missed(dev, ioaddr);
 
-	return &dev->stats;
+	do {
+		start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
+		stats->rx_packets = tp->rx_stats.packets;
+		stats->rx_bytes	= tp->rx_stats.bytes;
+	} while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));
+
+
+	do {
+		start = u64_stats_fetch_begin_bh(&tp->tx_stats.syncp);
+		stats->tx_packets = tp->tx_stats.packets;
+		stats->tx_bytes	= tp->tx_stats.bytes;
+	} while (u64_stats_fetch_retry_bh(&tp->tx_stats.syncp, start));
+
+	stats->rx_dropped	= dev->stats.rx_dropped;
+	stats->tx_dropped	= dev->stats.tx_dropped;
+	stats->rx_length_errors = dev->stats.rx_length_errors;
+	stats->rx_errors	= dev->stats.rx_errors;
+	stats->rx_crc_errors	= dev->stats.rx_crc_errors;
+	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
+	stats->rx_missed_errors = dev->stats.rx_missed_errors;
+
+	return stats;
 }
 
 static void rtl8169_net_suspend(struct net_device *dev)
-- 
1.7.7.6

[PATCH net-next] r8169: add byte queue limit support.

[Francois Romieu]

Btw the stuff below and the 64bits stats overlap. Both could
go together.

From: Igor Maravic <igorm@etf.rs>
Date: Mon, 5 Mar 2012 00:01:25 +0100
Subject: [PATCH 2/2] r8169: add byte queue limit support.

Nothing fancy:
- sent bytes count is notified in the start_xmit path right before
  updating the owner bit in the hardware Tx descriptor (E. Dumazet)
- avoid useless tp->dev dereferencing in start_xmit (E. Dumazet)

Use of netdev_reset_queue is favored over proper accounting in
rtl8169_tx_clear_range since the latter would need more work for the
same result (nb: said accounting degenerates to nothing in xmit_frags).

Signed-off-by: Igor Maravic <igorm@etf.rs>
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/realtek/r8169.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index a4d7674..515a7ed 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5399,6 +5399,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 {
 	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl_reset_work(struct rtl8169_private *tp)
@@ -5553,6 +5554,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	txd->opts2 = cpu_to_le32(opts[1]);
 
+	netdev_sent_queue(dev, skb->len);
+
 	wmb();
 
 	/* Anti gcc 2.95.3 bugware (sic) */
@@ -5647,9 +5650,16 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
 	rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
 }
 
+struct rtl_txc {
+	int packets;
+	int bytes;
+};
+
 static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 {
+	struct rtl8169_stats *tx_stats = &tp->tx_stats;
 	unsigned int dirty_tx, tx_left;
+	struct rtl_txc txc = { 0, 0 };
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5668,17 +5678,24 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			u64_stats_update_begin(&tp->tx_stats.syncp);
-			tp->tx_stats.packets++;
-			tp->tx_stats.bytes += tx_skb->skb->len;
-			u64_stats_update_end(&tp->tx_stats.syncp);
-			dev_kfree_skb(tx_skb->skb);
+			struct sk_buff *skb = tx_skb->skb;
+
+			txc.packets++;
+			txc.bytes += skb->len;
+			dev_kfree_skb(skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;
 		tx_left--;
 	}
 
+	u64_stats_update_begin(&tx_stats->syncp);
+	tx_stats->packets += txc.packets;
+	tx_stats->bytes += txc.bytes;
+	u64_stats_update_end(&tx_stats->syncp);
+
+	netdev_completed_queue(dev, txc.packets, txc.bytes);
+
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
 		/* Sync with rtl8169_start_xmit:
-- 
1.7.7.6

Re: [PATCH net-next] r8169: Add 64bit statistics

[Eric Dumazet]

Le lundi 05 mars 2012 à 00:24 +0100, Francois Romieu a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> :
> [...]
> > small point : Using this means you have a 32bit hole here (on 32bit
> > build). Its minor, you dont need to change.
> 
> Ok.
> 
> [...]
> > 	You could try to put these somewhere else, to try to keep this portion
> > as read only memory, to be more SMP friendly.
> > (Some loaded server could have one CPU serving RX stuff, and other cpus
> > doing TX stuff) 
> 
> Point taken. It could make sense to rework the rtl8169_private struct a bit
> more.
> 
> [...]
> > > @@ -6070,20 +6084,49 @@ static void rtl_set_rx_mode(struct net_device *dev)
> > >  }
> > >  
> > >  /**
> > > - *  rtl8169_get_stats - Get rtl8169 read/write statistics
> > > + *  rtl8169_get_stats64 - Get rtl8169 read/write statistics
> > >   *  @dev: The Ethernet Device to get statistics for
> > 
> > missing @stats
> 
> This documentation is almost useless anyway. I removed it.
> 
> [...]
> > You dont need _bytes and _packets temp variables, as @stats points to a
> > private memory, we can use it as working storage, just do :
> > 
> >        do {
> >                start = u64_stats_fetch_begin_bh(&tp->rx_stats.syncp);
> >                stats->rx_packets = tp->rx_stats.packets;
> >                stats->rx_bytes = tp->rx_stats.bytes;
> >        } while (u64_stats_fetch_retry_bh(&tp->rx_stats.syncp, start));
> 
> It should give something like the patch below.
> 
> If I understand correctly we do not care much about the error counters,
> right ?
> 
> From: Junchang Wang <junchangwang@gmail.com>
> Date: Sun, 4 Mar 2012 23:30:32 +0100
> Subject: [PATCH 1/2] r8169: add 64bit statistics.
> 
> Switch to use ndo_get_stats64 to get 64bit statistics.
> Two sync entries are used (one for Rx and one for Tx).
> 
> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
> Reviewed-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c |   59 ++++++++++++++++++++++++++--------
>  1 files changed, 45 insertions(+), 14 deletions(-)
> 

Seems Good To Me, thanks !

Re: [PATCH net-next] r8169: add byte queue limit support.

[Eric Dumazet]

Le lundi 05 mars 2012 à 00:28 +0100, Francois Romieu a écrit :
> Btw the stuff below and the 64bits stats overlap. Both could
> go together.
> 
> From: Igor Maravic <igorm@etf.rs>
> Date: Mon, 5 Mar 2012 00:01:25 +0100
> Subject: [PATCH 2/2] r8169: add byte queue limit support.
> 
> Nothing fancy:
> - sent bytes count is notified in the start_xmit path right before
>   updating the owner bit in the hardware Tx descriptor (E. Dumazet)
> - avoid useless tp->dev dereferencing in start_xmit (E. Dumazet)
> 
> Use of netdev_reset_queue is favored over proper accounting in
> rtl8169_tx_clear_range since the latter would need more work for the
> same result (nb: said accounting degenerates to nothing in xmit_frags).
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c |   27 ++++++++++++++++++++++-----
>  1 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index a4d7674..515a7ed 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -5399,6 +5399,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
>  {
>  	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
>  	tp->cur_tx = tp->dirty_tx = 0;
> +	netdev_reset_queue(tp->dev);
>  }
>  
>  static void rtl_reset_work(struct rtl8169_private *tp)
> @@ -5553,6 +5554,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>  
>  	txd->opts2 = cpu_to_le32(opts[1]);
>  
> +	netdev_sent_queue(dev, skb->len);
> +
>  	wmb();
>  
>  	/* Anti gcc 2.95.3 bugware (sic) */
> @@ -5647,9 +5650,16 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
>  	rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
>  }
>  
> +struct rtl_txc {
> +	int packets;
> +	int bytes;
> +};
> +
>  static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
>  {
> +	struct rtl8169_stats *tx_stats = &tp->tx_stats;
>  	unsigned int dirty_tx, tx_left;
> +	struct rtl_txc txc = { 0, 0 };
>  
>  	dirty_tx = tp->dirty_tx;
>  	smp_rmb();
> @@ -5668,17 +5678,24 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
>  		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
>  				     tp->TxDescArray + entry);
>  		if (status & LastFrag) {
> -			u64_stats_update_begin(&tp->tx_stats.syncp);
> -			tp->tx_stats.packets++;
> -			tp->tx_stats.bytes += tx_skb->skb->len;
> -			u64_stats_update_end(&tp->tx_stats.syncp);
> -			dev_kfree_skb(tx_skb->skb);
> +			struct sk_buff *skb = tx_skb->skb;
> +
> +			txc.packets++;
> +			txc.bytes += skb->len;
> +			dev_kfree_skb(skb);
>  			tx_skb->skb = NULL;
>  		}
>  		dirty_tx++;
>  		tx_left--;
>  	}
>  
> +	u64_stats_update_begin(&tx_stats->syncp);
> +	tx_stats->packets += txc.packets;
> +	tx_stats->bytes += txc.bytes;
> +	u64_stats_update_end(&tx_stats->syncp);
> +
> +	netdev_completed_queue(dev, txc.packets, txc.bytes);
> +
>  	if (tp->dirty_tx != dirty_tx) {
>  		tp->dirty_tx = dirty_tx;
>  		/* Sync with rtl8169_start_xmit:

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

By the way, the "From: Igor Maravic <igorm@etf.rs>" should be the very
first line of your mail, as mentioned in Documentation/SubmittingPatches
(around line 543). 

Otherwise, risk is that "Author" attribution might be you instead of
Igor (but the Signed-off-by order will be ok)

Re: [PATCH net-next] r8169: add byte queue limit support.

[Francois Romieu]

Eric Dumazet <eric.dumazet@gmail.com> :
[...]
> By the way, the "From: Igor Maravic <igorm@etf.rs>" should be the very
> first line of your mail, as mentioned in Documentation/SubmittingPatches
> (around line 543). 
> 
> Otherwise, risk is that "Author" attribution might be you instead of
> Igor (but the Signed-off-by order will be ok)

I'll do a formal submission on monday evening. It only was a heads up
as things conflicted and I had to modify Igor's patch.

Thanks for the (re-)reminder anyway.

-- 
Ueimor

Re: [PATCH net-next] r8169: Add 64bit statistics

[Junchang Wang]

>> 
>> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
>> Reviewed-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169.c |   59 ++++++++++++++++++++++++++--------
>>  1 files changed, 45 insertions(+), 14 deletions(-)
>> 
>
>Seems Good To Me, thanks !
>
Great. Thank you so much. :)

--Junchang