[PATCH v2 net-next] net: systemport: Support 64bit statistics

[PATCH v2 net-next] net: systemport: Support 64bit statistics

[Jianming.qiao]

Signed-off-by: Jianming.qiao <kiki-good@hotmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 52 +++++++++++++++++++++++++++---
 drivers/net/ethernet/broadcom/bcmsysport.h |  9 ++++--
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 5274501..56f8951 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -662,6 +662,7 @@ static int bcm_sysport_alloc_rx_bufs(struct bcm_sysport_priv *priv)
 static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
 					unsigned int budget)
 {
+	struct bcm_sysport_stats *stats64 = &priv->stats64;
 	struct net_device *ndev = priv->netdev;
 	unsigned int processed = 0, to_process;
 	struct bcm_sysport_cb *cb;
@@ -765,6 +766,10 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
 		skb->protocol = eth_type_trans(skb, ndev);
 		ndev->stats.rx_packets++;
 		ndev->stats.rx_bytes += len;
+		u64_stats_update_begin(&stats64->syncp);
+		stats64->rx_packets++;
+		stats64->rx_bytes += len;
+		u64_stats_update_end(&stats64->syncp);
 
 		napi_gro_receive(&priv->napi, skb);
 next:
@@ -784,24 +789,31 @@ static void bcm_sysport_tx_reclaim_one(struct bcm_sysport_tx_ring *ring,
 				       unsigned int *pkts_compl)
 {
 	struct bcm_sysport_priv *priv = ring->priv;
+	struct bcm_sysport_stats *stats64 = &priv->stats64;
 	struct device *kdev = &priv->pdev->dev;
+	unsigned int len = 0;
 
 	if (cb->skb) {
-		ring->bytes += cb->skb->len;
-		*bytes_compl += cb->skb->len;
+		len = cb->skb->len;
+		*bytes_compl += len;
 		dma_unmap_single(kdev, dma_unmap_addr(cb, dma_addr),
 				 dma_unmap_len(cb, dma_len),
 				 DMA_TO_DEVICE);
-		ring->packets++;
 		(*pkts_compl)++;
 		bcm_sysport_free_cb(cb);
 	/* SKB fragment */
 	} else if (dma_unmap_addr(cb, dma_addr)) {
-		ring->bytes += dma_unmap_len(cb, dma_len);
+		len = dma_unmap_len(cb, dma_len);
 		dma_unmap_page(kdev, dma_unmap_addr(cb, dma_addr),
 			       dma_unmap_len(cb, dma_len), DMA_TO_DEVICE);
 		dma_unmap_addr_set(cb, dma_addr, 0);
 	}
+
+	u64_stats_update_begin(&stats64->syncp);
+	ring->bytes += len;
+	if (cb->skb)
+		ring->packets++;
+	u64_stats_update_end(&stats64->syncp);
 }
 
 /* Reclaim queued SKBs for transmission completion, lockless version */
@@ -1923,6 +1935,37 @@ static int bcm_sysport_stop(struct net_device *dev)
 	return 0;
 }
 
+static void bcm_sysport_get_stats64(struct net_device *dev,
+				    struct rtnl_link_stats64 *stats)
+{
+	struct bcm_sysport_priv *priv = netdev_priv(dev);
+	struct bcm_sysport_stats *stats64 = &priv->stats64;
+	struct bcm_sysport_tx_ring *ring;
+	u64 tx_packets = 0, tx_bytes = 0;
+	unsigned int start;
+	unsigned int q;
+
+	netdev_stats_to_stats64(stats, &dev->stats);
+
+	for (q = 0; q < dev->num_tx_queues; q++) {
+		ring = &priv->tx_rings[q];
+		do {
+			start = u64_stats_fetch_begin_irq(&stats64->syncp);
+			tx_bytes += ring->bytes;
+			tx_packets += ring->packets;
+		} while (u64_stats_fetch_retry_irq(&stats64->syncp, start));
+	}
+
+	stats->tx_packets = tx_packets;
+	stats->tx_bytes = tx_bytes;
+
+	do {
+		start = u64_stats_fetch_begin_irq(&stats64->syncp);
+		stats->rx_packets = stats64->rx_packets;
+		stats->rx_bytes = stats64->rx_bytes;
+	} while (u64_stats_fetch_retry_irq(&stats64->syncp, start));
+}
+
 static const struct ethtool_ops bcm_sysport_ethtool_ops = {
 	.get_drvinfo		= bcm_sysport_get_drvinfo,
 	.get_msglevel		= bcm_sysport_get_msglvl,
@@ -1951,6 +1994,7 @@ static int bcm_sysport_stop(struct net_device *dev)
 	.ndo_poll_controller	= bcm_sysport_poll_controller,
 #endif
 	.ndo_get_stats		= bcm_sysport_get_nstats,
+	.ndo_get_stats64	= bcm_sysport_get_stats64,
 };
 
 #define REV_FMT	"v%2x.%02x"
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index 77a51c1..c03a176 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -657,6 +657,9 @@ struct bcm_sysport_stats {
 	enum bcm_sysport_stat_type type;
 	/* reg offset from UMAC base for misc counters */
 	u16 reg_offset;
+	u64     rx_packets;
+	u64     rx_bytes;
+	struct u64_stats_sync   syncp;
 };
 
 /* Software house keeping helper structure */
@@ -693,8 +696,8 @@ struct bcm_sysport_tx_ring {
 	struct bcm_sysport_cb *cbs;	/* Transmit control blocks */
 	struct dma_desc	*desc_cpu;	/* CPU view of the descriptor */
 	struct bcm_sysport_priv *priv;	/* private context backpointer */
-	unsigned long	packets;	/* packets statistics */
-	unsigned long	bytes;		/* bytes statistics */
+	u64	packets;		/* packets statistics */
+	u64	bytes;			/* bytes statistics */
 };
 
 /* Driver private structure */
@@ -743,5 +746,7 @@ struct bcm_sysport_priv {
 
 	/* Ethtool */
 	u32			msg_enable;
+	/* 64bit stats on 32bit/64bit Machine */
+	struct bcm_sysport_stats stats64;
 };
 #endif /* __BCM_SYSPORT_H */
-- 
1.9.1

Re: [PATCH v2 net-next] net: systemport: Support 64bit statistics

[David Miller]

From: "Jianming.qiao" <jqiaoulk@gmail.com>
Date: Wed, 19 Jul 2017 01:18:40 +0100

> Signed-off-by: Jianming.qiao <kiki-good@hotmail.com>

Supporting both deprecated .ndo_get_stats and 64-bit .ndo_get_stats64
at the same time makes no sense.

.ndo_get_stats will never be called if .ndo_get_stats64 is non-NULL

The lack of a commit log message, explaining in detail, why you are
doing this and why you are doing it this way, concerns me as well.

This submission so far has been a bit of a mess.  You don't
communicate enough, your commit message is empty, and therefore we
have no idea why you are doing things, and in particular the reasons
for your decisions.

I'm not applying this, sorry.

Re: [PATCH v2 net-next] net: systemport: Support 64bit statistics

[kiki good]

Hi David:

I am sorry for missing the commit log message;Since I did conversation
with Florian Fainelli about this patch in another email thread
"[PATCH] net: systemport: Support 64bit statistics", i incorrectly
thought it was unnecessary to add the commit log again when submitting
the revised patch.

The reason for keeping both  .ndo_get_stats and 64-bit
.ndo_get_stats64 comes from Florian, suggesting that because there is
no harm in keeping bcm_sysport_get_stats and we can always deprecate
it later, it is just to minimize the amount of changes to review.

Why do we need this change ?
When using Broadcom Systemport device in 32bit Platform, ifconfig can
only report up to 4G tx,rx status, which will be wrapped to 0 when the
number of incoming or outgoing packets exceeds 4G, only taking
around 2 hours in busy network environment (such as streaming).
Therefore, it makes hard for network diagnostic tool to get reliable
statistical result, so the patch is used to add 64bit support for
Broadcom Systemport device in 32bit Platform.

Thanks
Jmqiao

On Thu, Jul 20, 2017 at 6:25 AM, David Miller <davem@davemloft.net> wrote:
> From: "Jianming.qiao" <jqiaoulk@gmail.com>
> Date: Wed, 19 Jul 2017 01:18:40 +0100
>
>> Signed-off-by: Jianming.qiao <kiki-good@hotmail.com>
>
> Supporting both deprecated .ndo_get_stats and 64-bit .ndo_get_stats64
> at the same time makes no sense.
>
> .ndo_get_stats will never be called if .ndo_get_stats64 is non-NULL
>
> The lack of a commit log message, explaining in detail, why you are
> doing this and why you are doing it this way, concerns me as well.
>
> This submission so far has been a bit of a mess.  You don't
> communicate enough, your commit message is empty, and therefore we
> have no idea why you are doing things, and in particular the reasons
> for your decisions.
>
> I'm not applying this, sorry.

Re: [PATCH v2 net-next] net: systemport: Support 64bit statistics

[Florian Fainelli]

On 07/20/2017 03:13 AM, kiki good wrote:
> Hi David:
> 
> I am sorry for missing the commit log message;Since I did conversation
> with Florian Fainelli about this patch in another email thread
> "[PATCH] net: systemport: Support 64bit statistics", i incorrectly
> thought it was unnecessary to add the commit log again when submitting
> the revised patch.
> 
> The reason for keeping both  .ndo_get_stats and 64-bit
> .ndo_get_stats64 comes from Florian, suggesting that because there is
> no harm in keeping bcm_sysport_get_stats and we can always deprecate
> it later, it is just to minimize the amount of changes to review.

Yes, my bad for suggesting that, Jmqiao, please resubmit with a commit
message that is essentially your paragraph after the "Why do we need
this change?" question (don't include the question in the commit message).

Thanks!

> 
> Why do we need this change ?
> When using Broadcom Systemport device in 32bit Platform, ifconfig can
> only report up to 4G tx,rx status, which will be wrapped to 0 when the
> number of incoming or outgoing packets exceeds 4G, only taking
> around 2 hours in busy network environment (such as streaming).
> Therefore, it makes hard for network diagnostic tool to get reliable
> statistical result, so the patch is used to add 64bit support for
> Broadcom Systemport device in 32bit Platform.
> 
> Thanks
> Jmqiao
> 
> On Thu, Jul 20, 2017 at 6:25 AM, David Miller <davem@davemloft.net> wrote:
>> From: "Jianming.qiao" <jqiaoulk@gmail.com>
>> Date: Wed, 19 Jul 2017 01:18:40 +0100
>>
>>> Signed-off-by: Jianming.qiao <kiki-good@hotmail.com>
>>
>> Supporting both deprecated .ndo_get_stats and 64-bit .ndo_get_stats64
>> at the same time makes no sense.
>>
>> .ndo_get_stats will never be called if .ndo_get_stats64 is non-NULL
>>
>> The lack of a commit log message, explaining in detail, why you are
>> doing this and why you are doing it this way, concerns me as well.
>>
>> This submission so far has been a bit of a mess.  You don't
>> communicate enough, your commit message is empty, and therefore we
>> have no idea why you are doing things, and in particular the reasons
>> for your decisions.
>>
>> I'm not applying this, sorry.

-- 
Florian

Re: [PATCH v2 net-next] net: systemport: Support 64bit statistics

[Stephen Hemminger]

On Wed, 19 Jul 2017 01:18:40 +0100
"Jianming.qiao" <jqiaoulk@gmail.com> wrote:

> Signed-off-by: Jianming.qiao <kiki-good@hotmail.com>

You may want to consider using per-cpu statistics.

Re: [PATCH v2 net-next] net: systemport: Support 64bit statistics

[kiki good]

Hi Stephen:

Thanks for the suggestion of using per-cpu statistics. In this part of
code, there are two major reasons
not to use per-cpu variable for calculating Tx packets:

1. The update of ring->bytes and ring ->packets are protected with irq
version's spin lock in the current code logic. Although BH
     is disabled and it is safe to use per-cpu variable, i don't see
much more benefit of using it if it is still under irq version's spin
lock protection.

2. This driver requires update each ring's bytes and packets; The
number of rings are decided dynamically. so in order to use per-cpu
     variable, we need to track allocation and free every time when
the sizes of rings change that might have some performance concerns.

For Rx packets, using per-cpu variable is possible.

Thanks
Jmqiao

On Thu, Jul 20, 2017 at 4:44 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 19 Jul 2017 01:18:40 +0100
> "Jianming.qiao" <jqiaoulk@gmail.com> wrote:
>
>> Signed-off-by: Jianming.qiao <kiki-good@hotmail.com>
>
> You may want to consider using per-cpu statistics.
>