[PATCH] bnx2: avoid flushing statistics when doing a MTU change

[PATCH] bnx2: avoid flushing statistics when doing a MTU change

[leitao]

Actually when bnx2 changes the interface's MTU size, it resets
the chip and consequently flushes the interface statistics.
This patch saves the statistics in a temporary space in order to
maintain the statistics correct after the chip reset.

Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
---
 drivers/net/bnx2.c |   52 +++++++++++++++++++++++++++++++++++++---------------
 drivers/net/bnx2.h |    1 +
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 7fa4048..f99e688 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6469,46 +6469,58 @@ bnx2_get_stats(struct net_device *dev)
 	net_stats->rx_packets =
 		GET_NET_STATS(stats_blk->stat_IfHCInUcastPkts) +
 		GET_NET_STATS(stats_blk->stat_IfHCInMulticastPkts) +
-		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts) + 
+		bp->stats_extra->rx_packets;
 
 	net_stats->tx_packets =
 		GET_NET_STATS(stats_blk->stat_IfHCOutUcastPkts) +
 		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
-		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts) +
+		bp->stats_extra->tx_packets;
 
 	net_stats->rx_bytes =
-		GET_NET_STATS(stats_blk->stat_IfHCInOctets);
+		GET_NET_STATS(stats_blk->stat_IfHCInOctets) +
+		bp->stats_extra->rx_bytes;
 
 	net_stats->tx_bytes =
-		GET_NET_STATS(stats_blk->stat_IfHCOutOctets);
+		GET_NET_STATS(stats_blk->stat_IfHCOutOctets) +
+		bp->stats_extra->tx_bytes;
 
 	net_stats->multicast =
-		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
+		bp->stats_extra->multicast;
 
 	net_stats->collisions =
-		(unsigned long) stats_blk->stat_EtherStatsCollisions;
+		(unsigned long) stats_blk->stat_EtherStatsCollisions +
+		bp->stats_extra->collisions;
 
 	net_stats->rx_length_errors =
 		(unsigned long) (stats_blk->stat_EtherStatsUndersizePkts +
-		stats_blk->stat_EtherStatsOverrsizePkts);
+		stats_blk->stat_EtherStatsOverrsizePkts) +
+		bp->stats_extra->rx_length_errors;
 
 	net_stats->rx_over_errors =
 		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
-		stats_blk->stat_IfInMBUFDiscards);
+		stats_blk->stat_IfInMBUFDiscards) +
+		bp->stats_extra->rx_over_errors;
 
 	net_stats->rx_frame_errors =
-		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors;
+		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors +
+		bp->stats_extra->rx_frame_errors;
 
 	net_stats->rx_crc_errors =
-		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors;
+		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors +
+		bp->stats_extra->rx_crc_errors;
 
 	net_stats->rx_errors = net_stats->rx_length_errors +
 		net_stats->rx_over_errors + net_stats->rx_frame_errors +
-		net_stats->rx_crc_errors;
+		net_stats->rx_crc_errors +
+		bp->stats_extra->rx_errors;
 
 	net_stats->tx_aborted_errors =
     		(unsigned long) (stats_blk->stat_Dot3StatsExcessiveCollisions +
-		stats_blk->stat_Dot3StatsLateCollisions);
+		stats_blk->stat_Dot3StatsLateCollisions) +
+		bp->stats_extra->tx_aborted_errors;
 
 	if ((CHIP_NUM(bp) == CHIP_NUM_5706) ||
 	    (CHIP_ID(bp) == CHIP_ID_5708_A0))
@@ -6516,7 +6528,8 @@ bnx2_get_stats(struct net_device *dev)
 	else {
 		net_stats->tx_carrier_errors =
 			(unsigned long)
-			stats_blk->stat_Dot3StatsCarrierSenseErrors;
+			stats_blk->stat_Dot3StatsCarrierSenseErrors +
+			bp->stats_extra->tx_carrier_errors;
 	}
 
 	net_stats->tx_errors =
@@ -6524,11 +6537,13 @@ bnx2_get_stats(struct net_device *dev)
 		stats_blk->stat_emac_tx_stat_dot3statsinternalmactransmiterrors
 		+
 		net_stats->tx_aborted_errors +
-		net_stats->tx_carrier_errors;
+		net_stats->tx_carrier_errors +
+		bp->stats_extra->tx_errors;
 
 	net_stats->rx_missed_errors =
 		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
-		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop);
+		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop) +
+		bp->stats_extra->rx_missed_errors;
 
 	return net_stats;
 }
@@ -6989,6 +7004,11 @@ bnx2_change_ring_size(struct bnx2 *bp, u32 rx, u32 tx)
 {
 	if (netif_running(bp->dev)) {
 		bnx2_netif_stop(bp);
+
+		/* Save statistics that is going to be reseted */
+		memcpy(bp->stats_extra, &bp->dev->stats,
+			sizeof(struct net_device_stats));
+
 		bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
 		bnx2_free_skbs(bp);
 		bnx2_free_mem(bp);
@@ -7649,6 +7669,7 @@ bnx2_init_board(struct pci_dev *pdev, struct net_device *dev)
 
 	bp->flags = 0;
 	bp->phy_flags = 0;
+	bp->stats_extra = kzalloc(sizeof(struct net_device_stats), GFP_KERNEL);
 
 	/* enable device (incl. PCI PM wakeup), and bus-mastering */
 	rc = pci_enable_device(pdev);
@@ -8162,6 +8183,7 @@ bnx2_remove_one(struct pci_dev *pdev)
 	if (bp->regview)
 		iounmap(bp->regview);
 
+	kfree(bp->stats_extra);
 	free_netdev(dev);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
diff --git a/drivers/net/bnx2.h b/drivers/net/bnx2.h
index a4d8340..b29b0d5 100644
--- a/drivers/net/bnx2.h
+++ b/drivers/net/bnx2.h
@@ -6912,6 +6912,7 @@ struct bnx2 {
 
 	const struct firmware	*mips_firmware;
 	const struct firmware	*rv2p_firmware;
+	struct net_device_stats *stats_extra;
 };
 
 #define REG_RD(bp, offset)					\
-- 
1.6.0.2

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Michael Chan]

leitao@linux.vnet.ibm.com wrote:

> Actually when bnx2 changes the interface's MTU size, it resets
> the chip and consequently flushes the interface statistics.
> This patch saves the statistics in a temporary space in order to
> maintain the statistics correct after the chip reset.

Thanks Breno.  This looks ok, but I think we need to zero the
stats_extra during bnx2_open().

>
> Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
> ---
>  drivers/net/bnx2.c |   52
> +++++++++++++++++++++++++++++++++++++---------------
>  drivers/net/bnx2.h |    1 +
>  2 files changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 7fa4048..f99e688 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6469,46 +6469,58 @@ bnx2_get_stats(struct net_device *dev)
>       net_stats->rx_packets =
>               GET_NET_STATS(stats_blk->stat_IfHCInUcastPkts) +
>               GET_NET_STATS(stats_blk->stat_IfHCInMulticastPkts) +
> -             GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts);
> +             GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts) +
> +             bp->stats_extra->rx_packets;
>
>       net_stats->tx_packets =
>               GET_NET_STATS(stats_blk->stat_IfHCOutUcastPkts) +
>               GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
> -             GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts);
> +             GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts) +
> +             bp->stats_extra->tx_packets;
>
>       net_stats->rx_bytes =
> -             GET_NET_STATS(stats_blk->stat_IfHCInOctets);
> +             GET_NET_STATS(stats_blk->stat_IfHCInOctets) +
> +             bp->stats_extra->rx_bytes;
>
>       net_stats->tx_bytes =
> -             GET_NET_STATS(stats_blk->stat_IfHCOutOctets);
> +             GET_NET_STATS(stats_blk->stat_IfHCOutOctets) +
> +             bp->stats_extra->tx_bytes;
>
>       net_stats->multicast =
> -             GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts);
> +             GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
> +             bp->stats_extra->multicast;
>
>       net_stats->collisions =
> -             (unsigned long) stats_blk->stat_EtherStatsCollisions;
> +             (unsigned long) stats_blk->stat_EtherStatsCollisions +
> +             bp->stats_extra->collisions;
>
>       net_stats->rx_length_errors =
>               (unsigned long)
> (stats_blk->stat_EtherStatsUndersizePkts +
> -             stats_blk->stat_EtherStatsOverrsizePkts);
> +             stats_blk->stat_EtherStatsOverrsizePkts) +
> +             bp->stats_extra->rx_length_errors;
>
>       net_stats->rx_over_errors =
>               (unsigned long) (stats_blk->stat_IfInFTQDiscards +
> -             stats_blk->stat_IfInMBUFDiscards);
> +             stats_blk->stat_IfInMBUFDiscards) +
> +             bp->stats_extra->rx_over_errors;
>
>       net_stats->rx_frame_errors =
> -             (unsigned long)
> stats_blk->stat_Dot3StatsAlignmentErrors;
> +             (unsigned long)
> stats_blk->stat_Dot3StatsAlignmentErrors +
> +             bp->stats_extra->rx_frame_errors;
>
>       net_stats->rx_crc_errors =
> -             (unsigned long) stats_blk->stat_Dot3StatsFCSErrors;
> +             (unsigned long) stats_blk->stat_Dot3StatsFCSErrors +
> +             bp->stats_extra->rx_crc_errors;
>
>       net_stats->rx_errors = net_stats->rx_length_errors +
>               net_stats->rx_over_errors + net_stats->rx_frame_errors +
> -             net_stats->rx_crc_errors;
> +             net_stats->rx_crc_errors +
> +             bp->stats_extra->rx_errors;
>
>       net_stats->tx_aborted_errors =
>               (unsigned long)
> (stats_blk->stat_Dot3StatsExcessiveCollisions +
> -             stats_blk->stat_Dot3StatsLateCollisions);
> +             stats_blk->stat_Dot3StatsLateCollisions) +
> +             bp->stats_extra->tx_aborted_errors;
>
>       if ((CHIP_NUM(bp) == CHIP_NUM_5706) ||
>           (CHIP_ID(bp) == CHIP_ID_5708_A0))
> @@ -6516,7 +6528,8 @@ bnx2_get_stats(struct net_device *dev)
>       else {
>               net_stats->tx_carrier_errors =
>                       (unsigned long)
> -                     stats_blk->stat_Dot3StatsCarrierSenseErrors;
> +                     stats_blk->stat_Dot3StatsCarrierSenseErrors +
> +                     bp->stats_extra->tx_carrier_errors;
>       }
>
>       net_stats->tx_errors =
> @@ -6524,11 +6537,13 @@ bnx2_get_stats(struct net_device *dev)
>
> stats_blk->stat_emac_tx_stat_dot3statsinternalmactransmiterrors
>               +
>               net_stats->tx_aborted_errors +
> -             net_stats->tx_carrier_errors;
> +             net_stats->tx_carrier_errors +
> +             bp->stats_extra->tx_errors;
>
>       net_stats->rx_missed_errors =
>               (unsigned long) (stats_blk->stat_IfInFTQDiscards +
> -             stats_blk->stat_IfInMBUFDiscards +
> stats_blk->stat_FwRxDrop);
> +             stats_blk->stat_IfInMBUFDiscards +
> stats_blk->stat_FwRxDrop) +
> +             bp->stats_extra->rx_missed_errors;
>
>       return net_stats;
>  }
> @@ -6989,6 +7004,11 @@ bnx2_change_ring_size(struct bnx2 *bp,
> u32 rx, u32 tx)
>  {
>       if (netif_running(bp->dev)) {
>               bnx2_netif_stop(bp);
> +
> +             /* Save statistics that is going to be reseted */
> +             memcpy(bp->stats_extra, &bp->dev->stats,
> +                     sizeof(struct net_device_stats));
> +
>               bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
>               bnx2_free_skbs(bp);
>               bnx2_free_mem(bp);
> @@ -7649,6 +7669,7 @@ bnx2_init_board(struct pci_dev *pdev,
> struct net_device *dev)
>
>       bp->flags = 0;
>       bp->phy_flags = 0;
> +     bp->stats_extra = kzalloc(sizeof(struct
> net_device_stats), GFP_KERNEL);
>
>       /* enable device (incl. PCI PM wakeup), and bus-mastering */
>       rc = pci_enable_device(pdev);
> @@ -8162,6 +8183,7 @@ bnx2_remove_one(struct pci_dev *pdev)
>       if (bp->regview)
>               iounmap(bp->regview);
>
> +     kfree(bp->stats_extra);
>       free_netdev(dev);
>       pci_release_regions(pdev);
>       pci_disable_device(pdev);
> diff --git a/drivers/net/bnx2.h b/drivers/net/bnx2.h
> index a4d8340..b29b0d5 100644
> --- a/drivers/net/bnx2.h
> +++ b/drivers/net/bnx2.h
> @@ -6912,6 +6912,7 @@ struct bnx2 {
>
>       const struct firmware   *mips_firmware;
>       const struct firmware   *rv2p_firmware;
> +     struct net_device_stats *stats_extra;
>  };
>
>  #define REG_RD(bp, offset)                                   \
> --
> 1.6.0.2
>
>
>

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Eric Dumazet]

leitao@linux.vnet.ibm.com a écrit :
> Actually when bnx2 changes the interface's MTU size, it resets
> the chip and consequently flushes the interface statistics.
> This patch saves the statistics in a temporary space in order to
> maintain the statistics correct after the chip reset.
> 
> Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>


  
>  	return net_stats;
>  }
> @@ -6989,6 +7004,11 @@ bnx2_change_ring_size(struct bnx2 *bp, u32 rx, u32 tx)
>  {
>  	if (netif_running(bp->dev)) {
>  		bnx2_netif_stop(bp);

I wonder if you need to renew this stats before copying them ?
 (eg calling bnx2_get_stats())


> +
> +		/* Save statistics that is going to be reseted */
> +		memcpy(bp->stats_extra, &bp->dev->stats,
> +			sizeof(struct net_device_stats));
> +
>  		bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
>  		bnx2_free_skbs(bp);
>  		bnx2_free_mem(bp);
> @@ -7649,6 +7669,7 @@ bnx2_init_board(struct pci_dev *pdev, struct net_device *dev)
>  
>  	bp->flags = 0;
>  	bp->phy_flags = 0;
> +	bp->stats_extra = kzalloc(sizeof(struct net_device_stats), GFP_KERNEL);

There is no test of failed allocation.

>  
>  	/* enable device (incl. PCI PM wakeup), and bus-mastering */
>  	rc = pci_enable_device(pdev);
> @@ -8162,6 +8183,7 @@ bnx2_remove_one(struct pci_dev *pdev)
>  	if (bp->regview)
>  		iounmap(bp->regview);
>  
> +	kfree(bp->stats_extra);
>  	free_netdev(dev);
>  	pci_release_regions(pdev);
>  	pci_disable_device(pdev);

Structure is small anyway, why not including it in struct bnx2 ?

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Michael Chan]

Eric Dumazet wrote:

> leitao@linux.vnet.ibm.com a écrit :
> > Actually when bnx2 changes the interface's MTU size, it resets
> > the chip and consequently flushes the interface statistics.
> > This patch saves the statistics in a temporary space in order to
> > maintain the statistics correct after the chip reset.
> >
> > Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
>
>
>
> >     return net_stats;
> >  }
> > @@ -6989,6 +7004,11 @@ bnx2_change_ring_size(struct bnx2
> *bp, u32 rx, u32 tx)
> >  {
> >     if (netif_running(bp->dev)) {
> >             bnx2_netif_stop(bp);
>
> I wonder if you need to renew this stats before copying them ?
>  (eg calling bnx2_get_stats())

Calling bnx2_get_stats() won't refresh the stats counters because
they are DMA'ed once a second.  There's an I/O to immediately DMA
the counters:

REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
                                BNX2_HC_COMMAND_STATS_NOW);
REG_RD(bp, BNX2_HC_COMMAND);

We then need to add some delay to wait for the DMA to complete.

>
>
> > +
> > +           /* Save statistics that is going to be reseted */
> > +           memcpy(bp->stats_extra, &bp->dev->stats,
> > +                   sizeof(struct net_device_stats));
> > +
> >             bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
> >             bnx2_free_skbs(bp);
> >             bnx2_free_mem(bp);

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Eric Dumazet]

Michael Chan a écrit :
> Eric Dumazet wrote:
> 
>> leitao@linux.vnet.ibm.com a écrit :
>>> Actually when bnx2 changes the interface's MTU size, it resets
>>> the chip and consequently flushes the interface statistics.
>>> This patch saves the statistics in a temporary space in order to
>>> maintain the statistics correct after the chip reset.
>>>
>>> Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
>>
>>
>>>     return net_stats;
>>>  }
>>> @@ -6989,6 +7004,11 @@ bnx2_change_ring_size(struct bnx2
>> *bp, u32 rx, u32 tx)
>>>  {
>>>     if (netif_running(bp->dev)) {
>>>             bnx2_netif_stop(bp);
>> I wonder if you need to renew this stats before copying them ?
>>  (eg calling bnx2_get_stats())
> 
> Calling bnx2_get_stats() won't refresh the stats counters because
> they are DMA'ed once a second.  There's an I/O to immediately DMA
> the counters:
> 
> REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
>                                 BNX2_HC_COMMAND_STATS_NOW);
> REG_RD(bp, BNX2_HC_COMMAND);
> 
> We then need to add some delay to wait for the DMA to complete.

Yes, but if you dont call bnx2_get_stats() netdev->stats will contain
possibly very old values.

At least, calling it will give one no more than second error.

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Michael Chan]

Eric Dumazet wrote:

> Michael Chan a écrit :
> > Calling bnx2_get_stats() won't refresh the stats counters because
> > they are DMA'ed once a second.  There's an I/O to immediately DMA
> > the counters:
> >
> > REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
> >                                 BNX2_HC_COMMAND_STATS_NOW);
> > REG_RD(bp, BNX2_HC_COMMAND);
> >
> > We then need to add some delay to wait for the DMA to complete.
>
> Yes, but if you dont call bnx2_get_stats() netdev->stats will contain
> possibly very old values.
>
> At least, calling it will give one no more than second error.
>

OK I see.  We are also not saving the ethtool stats.  So perhaps the
more complete solution is to save the entire statistics block before
reset.

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Breno Leitao]

Hi Michael, Eric, 

Thanks for the reviews. Now stats_extra is inline in the bnx2 structure,
bnx2_open is cleaning stats_extra and I am doing a DMA to the statistics
structure before saving it.

Michael Chan wrote:
> Eric Dumazet wrote:
>> I wonder if you need to renew this stats before copying them ?
>>  (eg calling bnx2_get_stats())
> 
> Calling bnx2_get_stats() won't refresh the stats counters because
> they are DMA'ed once a second.  There's an I/O to immediately DMA
> the counters:
> 
> REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
>                                 BNX2_HC_COMMAND_STATS_NOW);
> REG_RD(bp, BNX2_HC_COMMAND);
> 
> We then need to add some delay to wait for the DMA to complete.
In the following patch, I just called this I/O as specified above, 
but looking into the code, I saw that the following I/O is just called
when BNX2_FLAG_BROKEN_STATS is set, and I am not sure if my call is correct.

Here is my new patch:

---

[PATCH] bnx2: avoid flushing statistics when doing a MTU change

Actually when bnx2 changes the interface's MTU size, it resets
the chip and consequently flushes the interface statistics.
This patch saves the statistics in a temporary space in order to
maintain the statistics correct after the chip reset.

Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
---
 drivers/net/bnx2.c |   56 ++++++++++++++++++++++++++++++++++++++-------------
 drivers/net/bnx2.h |    1 +
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 7fa4048..3cefee3 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6221,6 +6221,8 @@ bnx2_open(struct net_device *dev)
 	else if (bp->flags & BNX2_FLAG_USING_MSIX)
 		printk(KERN_INFO PFX "%s: using MSIX\n", dev->name);
 
+	memset(&bp->stats_extra, 0, sizeof(struct net_device_stats));
+
 	netif_tx_start_all_queues(dev);
 
 	return 0;
@@ -6469,46 +6471,58 @@ bnx2_get_stats(struct net_device *dev)
 	net_stats->rx_packets =
 		GET_NET_STATS(stats_blk->stat_IfHCInUcastPkts) +
 		GET_NET_STATS(stats_blk->stat_IfHCInMulticastPkts) +
-		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts) + 
+		bp->stats_extra.rx_packets;
 
 	net_stats->tx_packets =
 		GET_NET_STATS(stats_blk->stat_IfHCOutUcastPkts) +
 		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
-		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts) +
+		bp->stats_extra.tx_packets;
 
 	net_stats->rx_bytes =
-		GET_NET_STATS(stats_blk->stat_IfHCInOctets);
+		GET_NET_STATS(stats_blk->stat_IfHCInOctets) +
+		bp->stats_extra.rx_bytes;
 
 	net_stats->tx_bytes =
-		GET_NET_STATS(stats_blk->stat_IfHCOutOctets);
+		GET_NET_STATS(stats_blk->stat_IfHCOutOctets) +
+		bp->stats_extra.tx_bytes;
 
 	net_stats->multicast =
-		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
+		bp->stats_extra.multicast;
 
 	net_stats->collisions =
-		(unsigned long) stats_blk->stat_EtherStatsCollisions;
+		(unsigned long) stats_blk->stat_EtherStatsCollisions +
+		bp->stats_extra.collisions;
 
 	net_stats->rx_length_errors =
 		(unsigned long) (stats_blk->stat_EtherStatsUndersizePkts +
-		stats_blk->stat_EtherStatsOverrsizePkts);
+		stats_blk->stat_EtherStatsOverrsizePkts) +
+		bp->stats_extra.rx_length_errors;
 
 	net_stats->rx_over_errors =
 		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
-		stats_blk->stat_IfInMBUFDiscards);
+		stats_blk->stat_IfInMBUFDiscards) +
+		bp->stats_extra.rx_over_errors;
 
 	net_stats->rx_frame_errors =
-		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors;
+		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors +
+		bp->stats_extra.rx_frame_errors;
 
 	net_stats->rx_crc_errors =
-		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors;
+		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors +
+		bp->stats_extra.rx_crc_errors;
 
 	net_stats->rx_errors = net_stats->rx_length_errors +
 		net_stats->rx_over_errors + net_stats->rx_frame_errors +
-		net_stats->rx_crc_errors;
+		net_stats->rx_crc_errors +
+		bp->stats_extra.rx_errors;
 
 	net_stats->tx_aborted_errors =
     		(unsigned long) (stats_blk->stat_Dot3StatsExcessiveCollisions +
-		stats_blk->stat_Dot3StatsLateCollisions);
+		stats_blk->stat_Dot3StatsLateCollisions) +
+		bp->stats_extra.tx_aborted_errors;
 
 	if ((CHIP_NUM(bp) == CHIP_NUM_5706) ||
 	    (CHIP_ID(bp) == CHIP_ID_5708_A0))
@@ -6516,7 +6530,8 @@ bnx2_get_stats(struct net_device *dev)
 	else {
 		net_stats->tx_carrier_errors =
 			(unsigned long)
-			stats_blk->stat_Dot3StatsCarrierSenseErrors;
+			stats_blk->stat_Dot3StatsCarrierSenseErrors +
+			bp->stats_extra.tx_carrier_errors;
 	}
 
 	net_stats->tx_errors =
@@ -6524,11 +6539,13 @@ bnx2_get_stats(struct net_device *dev)
 		stats_blk->stat_emac_tx_stat_dot3statsinternalmactransmiterrors
 		+
 		net_stats->tx_aborted_errors +
-		net_stats->tx_carrier_errors;
+		net_stats->tx_carrier_errors +
+		bp->stats_extra.tx_errors;
 
 	net_stats->rx_missed_errors =
 		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
-		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop);
+		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop) +
+		bp->stats_extra.rx_missed_errors;
 
 	return net_stats;
 }
@@ -6989,6 +7006,15 @@ bnx2_change_ring_size(struct bnx2 *bp, u32 rx, u32 tx)
 {
 	if (netif_running(bp->dev)) {
 		bnx2_netif_stop(bp);
+
+		/* Save statistics that is going to be reseted */
+		REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
+				BNX2_HC_COMMAND_STATS_NOW);
+		REG_RD(bp, BNX2_HC_COMMAND);
+		udelay(5);
+		memcpy(&bp->stats_extra, &bp->dev->stats,
+			sizeof(struct net_device_stats));
+
 		bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
 		bnx2_free_skbs(bp);
 		bnx2_free_mem(bp);
diff --git a/drivers/net/bnx2.h b/drivers/net/bnx2.h
index a4d8340..1063d1a 100644
--- a/drivers/net/bnx2.h
+++ b/drivers/net/bnx2.h
@@ -6912,6 +6912,7 @@ struct bnx2 {
 
 	const struct firmware	*mips_firmware;
 	const struct firmware	*rv2p_firmware;
+	struct net_device_stats stats_extra;
 };
 
 #define REG_RD(bp, offset)					\
-- 
1.6.0.2

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Eric Dumazet]

Breno Leitao a écrit :
  	if (netif_running(bp->dev)) {
>  		bnx2_netif_stop(bp);
> +
> +		/* Save statistics that is going to be reseted */
> +		REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
> +				BNX2_HC_COMMAND_STATS_NOW);
> +		REG_RD(bp, BNX2_HC_COMMAND);
> +		udelay(5);

I see nothing in the driver/patch that will consolidate bp->dev->stats 
before your memcpy(). DMA transfert doesnt touch bp->dev->stats.

Please take a look at bnx2_get_stats()

> +		memcpy(&bp->stats_extra, &bp->dev->stats,
> +			sizeof(struct net_device_stats));
> +
>  		bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Breno Leitao]

Eric Dumazet wrote:
> I see nothing in the driver/patch that will consolidate bp->dev->stats 
> before your memcpy(). DMA transfert doesnt touch bp->dev->stats.
You're right, calling bnx2_get_stats() after the DMA should be enough, 
as the following patch
---

[PATCH] bnx2: avoid flushing statistics when doing a MTU change

Actually when bnx2 changes the interface's MTU size, it resets
the chip and consequently flushes the interface statistics.
This patch saves the statistics in a temporary space in order to
maintain the statistics correct after the chip reset.

Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
---
 drivers/net/bnx2.c |   57 ++++++++++++++++++++++++++++++++++++++-------------
 drivers/net/bnx2.h |    1 +
 2 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 7fa4048..07c0376 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6221,6 +6221,8 @@ bnx2_open(struct net_device *dev)
 	else if (bp->flags & BNX2_FLAG_USING_MSIX)
 		printk(KERN_INFO PFX "%s: using MSIX\n", dev->name);
 
+	memset(&bp->stats_extra, 0, sizeof(struct net_device_stats));
+
 	netif_tx_start_all_queues(dev);
 
 	return 0;
@@ -6469,46 +6471,58 @@ bnx2_get_stats(struct net_device *dev)
 	net_stats->rx_packets =
 		GET_NET_STATS(stats_blk->stat_IfHCInUcastPkts) +
 		GET_NET_STATS(stats_blk->stat_IfHCInMulticastPkts) +
-		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts) + 
+		bp->stats_extra.rx_packets;
 
 	net_stats->tx_packets =
 		GET_NET_STATS(stats_blk->stat_IfHCOutUcastPkts) +
 		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
-		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts) +
+		bp->stats_extra.tx_packets;
 
 	net_stats->rx_bytes =
-		GET_NET_STATS(stats_blk->stat_IfHCInOctets);
+		GET_NET_STATS(stats_blk->stat_IfHCInOctets) +
+		bp->stats_extra.rx_bytes;
 
 	net_stats->tx_bytes =
-		GET_NET_STATS(stats_blk->stat_IfHCOutOctets);
+		GET_NET_STATS(stats_blk->stat_IfHCOutOctets) +
+		bp->stats_extra.tx_bytes;
 
 	net_stats->multicast =
-		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts);
+		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
+		bp->stats_extra.multicast;
 
 	net_stats->collisions =
-		(unsigned long) stats_blk->stat_EtherStatsCollisions;
+		(unsigned long) stats_blk->stat_EtherStatsCollisions +
+		bp->stats_extra.collisions;
 
 	net_stats->rx_length_errors =
 		(unsigned long) (stats_blk->stat_EtherStatsUndersizePkts +
-		stats_blk->stat_EtherStatsOverrsizePkts);
+		stats_blk->stat_EtherStatsOverrsizePkts) +
+		bp->stats_extra.rx_length_errors;
 
 	net_stats->rx_over_errors =
 		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
-		stats_blk->stat_IfInMBUFDiscards);
+		stats_blk->stat_IfInMBUFDiscards) +
+		bp->stats_extra.rx_over_errors;
 
 	net_stats->rx_frame_errors =
-		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors;
+		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors +
+		bp->stats_extra.rx_frame_errors;
 
 	net_stats->rx_crc_errors =
-		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors;
+		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors +
+		bp->stats_extra.rx_crc_errors;
 
 	net_stats->rx_errors = net_stats->rx_length_errors +
 		net_stats->rx_over_errors + net_stats->rx_frame_errors +
-		net_stats->rx_crc_errors;
+		net_stats->rx_crc_errors +
+		bp->stats_extra.rx_errors;
 
 	net_stats->tx_aborted_errors =
     		(unsigned long) (stats_blk->stat_Dot3StatsExcessiveCollisions +
-		stats_blk->stat_Dot3StatsLateCollisions);
+		stats_blk->stat_Dot3StatsLateCollisions) +
+		bp->stats_extra.tx_aborted_errors;
 
 	if ((CHIP_NUM(bp) == CHIP_NUM_5706) ||
 	    (CHIP_ID(bp) == CHIP_ID_5708_A0))
@@ -6516,7 +6530,8 @@ bnx2_get_stats(struct net_device *dev)
 	else {
 		net_stats->tx_carrier_errors =
 			(unsigned long)
-			stats_blk->stat_Dot3StatsCarrierSenseErrors;
+			stats_blk->stat_Dot3StatsCarrierSenseErrors +
+			bp->stats_extra.tx_carrier_errors;
 	}
 
 	net_stats->tx_errors =
@@ -6524,11 +6539,13 @@ bnx2_get_stats(struct net_device *dev)
 		stats_blk->stat_emac_tx_stat_dot3statsinternalmactransmiterrors
 		+
 		net_stats->tx_aborted_errors +
-		net_stats->tx_carrier_errors;
+		net_stats->tx_carrier_errors +
+		bp->stats_extra.tx_errors;
 
 	net_stats->rx_missed_errors =
 		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
-		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop);
+		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop) +
+		bp->stats_extra.rx_missed_errors;
 
 	return net_stats;
 }
@@ -6989,6 +7006,16 @@ bnx2_change_ring_size(struct bnx2 *bp, u32 rx, u32 tx)
 {
 	if (netif_running(bp->dev)) {
 		bnx2_netif_stop(bp);
+
+		/* Save statistics that is going to be reseted */
+		REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
+				BNX2_HC_COMMAND_STATS_NOW);
+		REG_RD(bp, BNX2_HC_COMMAND);
+		udelay(5);
+		bnx2_get_stats(bp->dev);
+		memcpy(&bp->stats_extra, &bp->dev->stats,
+			sizeof(struct net_device_stats));
+
 		bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
 		bnx2_free_skbs(bp);
 		bnx2_free_mem(bp);
diff --git a/drivers/net/bnx2.h b/drivers/net/bnx2.h
index a4d8340..1063d1a 100644
--- a/drivers/net/bnx2.h
+++ b/drivers/net/bnx2.h
@@ -6912,6 +6912,7 @@ struct bnx2 {
 
 	const struct firmware	*mips_firmware;
 	const struct firmware	*rv2p_firmware;
+	struct net_device_stats stats_extra;
 };
 
 #define REG_RD(bp, offset)					\
-- 
1.6.0.2

Re: [PATCH] bnx2: avoid flushing statistics when doing a MTU change

[Michael Chan]

On Mon, 2009-11-30 at 06:05 -0800, Breno Leitao wrote:
> Eric Dumazet wrote:
> > I see nothing in the driver/patch that will consolidate bp->dev->stats 
> > before your memcpy(). DMA transfert doesnt touch bp->dev->stats.
> You're right, calling bnx2_get_stats() after the DMA should be enough, 
> as the following patch
> ---
> 
> [PATCH] bnx2: avoid flushing statistics when doing a MTU change
> 
> Actually when bnx2 changes the interface's MTU size, it resets
> the chip and consequently flushes the interface statistics.
> This patch saves the statistics in a temporary space in order to
> maintain the statistics correct after the chip reset.

Thanks Breno.  As I mentioned earlier, this does not save the ethtool -S
stats before reset.  The user may be confused when netstat does not
match ethtool -S.  I think we should save the statistics block so
everything will match up.  If you don't beat me to it, I'll work on the
patch later today.

> 
> Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
> ---
>  drivers/net/bnx2.c |   57 ++++++++++++++++++++++++++++++++++++++-------------
>  drivers/net/bnx2.h |    1 +
>  2 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 7fa4048..07c0376 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6221,6 +6221,8 @@ bnx2_open(struct net_device *dev)
>  	else if (bp->flags & BNX2_FLAG_USING_MSIX)
>  		printk(KERN_INFO PFX "%s: using MSIX\n", dev->name);
>  
> +	memset(&bp->stats_extra, 0, sizeof(struct net_device_stats));
> +
>  	netif_tx_start_all_queues(dev);
>  
>  	return 0;
> @@ -6469,46 +6471,58 @@ bnx2_get_stats(struct net_device *dev)
>  	net_stats->rx_packets =
>  		GET_NET_STATS(stats_blk->stat_IfHCInUcastPkts) +
>  		GET_NET_STATS(stats_blk->stat_IfHCInMulticastPkts) +
> -		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts);
> +		GET_NET_STATS(stats_blk->stat_IfHCInBroadcastPkts) + 
> +		bp->stats_extra.rx_packets;
>  
>  	net_stats->tx_packets =
>  		GET_NET_STATS(stats_blk->stat_IfHCOutUcastPkts) +
>  		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
> -		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts);
> +		GET_NET_STATS(stats_blk->stat_IfHCOutBroadcastPkts) +
> +		bp->stats_extra.tx_packets;
>  
>  	net_stats->rx_bytes =
> -		GET_NET_STATS(stats_blk->stat_IfHCInOctets);
> +		GET_NET_STATS(stats_blk->stat_IfHCInOctets) +
> +		bp->stats_extra.rx_bytes;
>  
>  	net_stats->tx_bytes =
> -		GET_NET_STATS(stats_blk->stat_IfHCOutOctets);
> +		GET_NET_STATS(stats_blk->stat_IfHCOutOctets) +
> +		bp->stats_extra.tx_bytes;
>  
>  	net_stats->multicast =
> -		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts);
> +		GET_NET_STATS(stats_blk->stat_IfHCOutMulticastPkts) +
> +		bp->stats_extra.multicast;
>  
>  	net_stats->collisions =
> -		(unsigned long) stats_blk->stat_EtherStatsCollisions;
> +		(unsigned long) stats_blk->stat_EtherStatsCollisions +
> +		bp->stats_extra.collisions;
>  
>  	net_stats->rx_length_errors =
>  		(unsigned long) (stats_blk->stat_EtherStatsUndersizePkts +
> -		stats_blk->stat_EtherStatsOverrsizePkts);
> +		stats_blk->stat_EtherStatsOverrsizePkts) +
> +		bp->stats_extra.rx_length_errors;
>  
>  	net_stats->rx_over_errors =
>  		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
> -		stats_blk->stat_IfInMBUFDiscards);
> +		stats_blk->stat_IfInMBUFDiscards) +
> +		bp->stats_extra.rx_over_errors;
>  
>  	net_stats->rx_frame_errors =
> -		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors;
> +		(unsigned long) stats_blk->stat_Dot3StatsAlignmentErrors +
> +		bp->stats_extra.rx_frame_errors;
>  
>  	net_stats->rx_crc_errors =
> -		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors;
> +		(unsigned long) stats_blk->stat_Dot3StatsFCSErrors +
> +		bp->stats_extra.rx_crc_errors;
>  
>  	net_stats->rx_errors = net_stats->rx_length_errors +
>  		net_stats->rx_over_errors + net_stats->rx_frame_errors +
> -		net_stats->rx_crc_errors;
> +		net_stats->rx_crc_errors +
> +		bp->stats_extra.rx_errors;
>  
>  	net_stats->tx_aborted_errors =
>      		(unsigned long) (stats_blk->stat_Dot3StatsExcessiveCollisions +
> -		stats_blk->stat_Dot3StatsLateCollisions);
> +		stats_blk->stat_Dot3StatsLateCollisions) +
> +		bp->stats_extra.tx_aborted_errors;
>  
>  	if ((CHIP_NUM(bp) == CHIP_NUM_5706) ||
>  	    (CHIP_ID(bp) == CHIP_ID_5708_A0))
> @@ -6516,7 +6530,8 @@ bnx2_get_stats(struct net_device *dev)
>  	else {
>  		net_stats->tx_carrier_errors =
>  			(unsigned long)
> -			stats_blk->stat_Dot3StatsCarrierSenseErrors;
> +			stats_blk->stat_Dot3StatsCarrierSenseErrors +
> +			bp->stats_extra.tx_carrier_errors;
>  	}
>  
>  	net_stats->tx_errors =
> @@ -6524,11 +6539,13 @@ bnx2_get_stats(struct net_device *dev)
>  		stats_blk->stat_emac_tx_stat_dot3statsinternalmactransmiterrors
>  		+
>  		net_stats->tx_aborted_errors +
> -		net_stats->tx_carrier_errors;
> +		net_stats->tx_carrier_errors +
> +		bp->stats_extra.tx_errors;
>  
>  	net_stats->rx_missed_errors =
>  		(unsigned long) (stats_blk->stat_IfInFTQDiscards +
> -		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop);
> +		stats_blk->stat_IfInMBUFDiscards + stats_blk->stat_FwRxDrop) +
> +		bp->stats_extra.rx_missed_errors;
>  
>  	return net_stats;
>  }
> @@ -6989,6 +7006,16 @@ bnx2_change_ring_size(struct bnx2 *bp, u32 rx, u32 tx)
>  {
>  	if (netif_running(bp->dev)) {
>  		bnx2_netif_stop(bp);
> +
> +		/* Save statistics that is going to be reseted */
> +		REG_WR(bp, BNX2_HC_COMMAND, bp->hc_cmd |
> +				BNX2_HC_COMMAND_STATS_NOW);
> +		REG_RD(bp, BNX2_HC_COMMAND);
> +		udelay(5);
> +		bnx2_get_stats(bp->dev);
> +		memcpy(&bp->stats_extra, &bp->dev->stats,
> +			sizeof(struct net_device_stats));
> +
>  		bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
>  		bnx2_free_skbs(bp);
>  		bnx2_free_mem(bp);
> diff --git a/drivers/net/bnx2.h b/drivers/net/bnx2.h
> index a4d8340..1063d1a 100644
> --- a/drivers/net/bnx2.h
> +++ b/drivers/net/bnx2.h
> @@ -6912,6 +6912,7 @@ struct bnx2 {
>  
>  	const struct firmware	*mips_firmware;
>  	const struct firmware	*rv2p_firmware;
> +	struct net_device_stats stats_extra;
>  };
>  
>  #define REG_RD(bp, offset)					\