[V2,RFC,net-next,1/2] virtio_net: convert the statistics into array
diff mbox series

Message ID 20120606075208.29081.75284.stgit@amd-6168-8-1.englab.nay.redhat.com
State New, archived
Headers show
Series
  • [V2,RFC,net-next,1/2] virtio_net: convert the statistics into array
Related show

Commit Message

Jason Wang June 6, 2012, 7:52 a.m. UTC
Currently, we store the statistics in the independent fields of virtnet_stats,
this is not scalable when we want to add more counters. As suggested by Michael,
this patch convert it to an array and use the enum as the index to access them.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c |   30 +++++++++++++++++-------------
 1 files changed, 17 insertions(+), 13 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Eric Dumazet June 6, 2012, 8:22 a.m. UTC | #1
On Wed, 2012-06-06 at 15:52 +0800, Jason Wang wrote:
> Currently, we store the statistics in the independent fields of virtnet_stats,
> this is not scalable when we want to add more counters. As suggested by Michael,
> this patch convert it to an array and use the enum as the index to access them.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c |   30 +++++++++++++++++-------------
>  1 files changed, 17 insertions(+), 13 deletions(-)
> 

>  struct virtnet_stats {
>  	struct u64_stats_sync syncp;
> -	u64 tx_bytes;
> -	u64 tx_packets;
> -
> -	u64 rx_bytes;
> -	u64 rx_packets;
> +	u64 data[VIRTNET_NUM_STATS];
>  };
>  

Interesting, but I fear you'll have a lot of problems.

Current code is buggy, and you are adding more possible races.

We could have one cpu doing the :

       u64_stats_update_begin(&stats->syncp);
       stats->rx_bytes += skb->len;
       stats->rx_packets++;
       u64_stats_update_end(&stats->syncp);

And another one doing :

       u64_stats_update_begin(&stats->syncp);
       stats->tx_bytes += skb->len;
       stats->tx_packets++;
       u64_stats_update_end(&stats->syncp);
 
And one syncp sequence increment can be lost, since both cpus are
basically doing this at the same time :

    write_seqcount_begin(&syncp->seq);

I'll send a fix in a separate thread.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5214b1e..6e4aa6f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -41,13 +41,17 @@  module_param(gso, bool, 0444);
 #define VIRTNET_SEND_COMMAND_SG_MAX    2
 #define VIRTNET_DRIVER_VERSION "1.0.0"
 
+enum virtnet_stats_type {
+	VIRTNET_TX_BYTES,
+	VIRTNET_TX_PACKETS,
+	VIRTNET_RX_BYTES,
+	VIRTNET_RX_PACKETS,
+	VIRTNET_NUM_STATS,
+};
+
 struct virtnet_stats {
 	struct u64_stats_sync syncp;
-	u64 tx_bytes;
-	u64 tx_packets;
-
-	u64 rx_bytes;
-	u64 rx_packets;
+	u64 data[VIRTNET_NUM_STATS];
 };
 
 struct virtnet_info {
@@ -301,8 +305,8 @@  static void receive_buf(struct net_device *dev, void *buf, unsigned int len)
 	hdr = skb_vnet_hdr(skb);
 
 	u64_stats_update_begin(&stats->syncp);
-	stats->rx_bytes += skb->len;
-	stats->rx_packets++;
+	stats->data[VIRTNET_RX_BYTES] += skb->len;
+	stats->data[VIRTNET_RX_PACKETS]++;
 	u64_stats_update_end(&stats->syncp);
 
 	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
@@ -566,8 +570,8 @@  static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
 		pr_debug("Sent skb %p\n", skb);
 
 		u64_stats_update_begin(&stats->syncp);
-		stats->tx_bytes += skb->len;
-		stats->tx_packets++;
+		stats->data[VIRTNET_TX_BYTES] += skb->len;
+		stats->data[VIRTNET_TX_PACKETS]++;
 		u64_stats_update_end(&stats->syncp);
 
 		tot_sgs += skb_vnet_hdr(skb)->num_sg;
@@ -704,10 +708,10 @@  static struct rtnl_link_stats64 *virtnet_stats(struct net_device *dev,
 
 		do {
 			start = u64_stats_fetch_begin(&stats->syncp);
-			tpackets = stats->tx_packets;
-			tbytes   = stats->tx_bytes;
-			rpackets = stats->rx_packets;
-			rbytes   = stats->rx_bytes;
+			tpackets = stats->data[VIRTNET_TX_PACKETS];
+			tbytes   = stats->data[VIRTNET_TX_BYTES];
+			rpackets = stats->data[VIRTNET_RX_PACKETS];
+			rbytes   = stats->data[VIRTNET_RX_BYTES];
 		} while (u64_stats_fetch_retry(&stats->syncp, start));
 
 		tot->rx_packets += rpackets;