All of lore.kernel.org
 help / color / mirror / Atom feed
* [net-next-2.6 PATCH] enic: Add timestamp to network interface stats
@ 2011-08-05  0:11 Danny Guo
  2011-08-05  4:42 ` Stephen Hemminger
  2011-08-06  6:25 ` David Miller
  0 siblings, 2 replies; 3+ messages in thread
From: Danny Guo @ 2011-08-05  0:11 UTC (permalink / raw)
  To: davem; +Cc: netdev

From: Danny Guo <dannguo@cisco.com>

This patch adds timestamps in ethtool stats. It makes it easier to provide scripts to users to calculate throughput, etc. It also allows software to synchronize timestamps with host time for correlating host events with stats collection.

Signed-off-by: Danny Guo <dannguo@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
Signed-off-by: Roopa Prabhu <roprabhu@cisco.com>
Signed-off-by: David Wang <dwang2@cisco.com>
---
 drivers/net/enic/enic.h       |    2 +-
 drivers/net/enic/enic_main.c  |    7 ++++++-
 drivers/net/enic/vnic_dev.c   |    1 +
 drivers/net/enic/vnic_stats.h |    1 +
 4 files changed, 9 insertions(+), 2 deletions(-)


diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index ce76d9a..2e58bdc 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -32,7 +32,7 @@
 
 #define DRV_NAME		"enic"
 #define DRV_DESCRIPTION		"Cisco VIC Ethernet NIC Driver"
-#define DRV_VERSION		"2.1.1.24"
+#define DRV_VERSION		"2.1.1.25"
 #define DRV_COPYRIGHT		"Copyright 2008-2011 Cisco Systems, Inc"
 
 #define ENIC_BARS_MAX		6
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 67a27cd..a3f61c1 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -121,6 +121,8 @@ static const struct enic_stat enic_rx_stats[] = {
 static const unsigned int enic_n_tx_stats = ARRAY_SIZE(enic_tx_stats);
 static const unsigned int enic_n_rx_stats = ARRAY_SIZE(enic_rx_stats);
 
+#define ENIC_TIMESTAMP_STAT_NAME	"timestamp(ns)"
+
 static int enic_is_dynamic(struct enic *enic)
 {
 	return enic->pdev->device == PCI_DEVICE_ID_CISCO_VIC_ENET_DYN;
@@ -224,6 +226,8 @@ static void enic_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 			memcpy(data, enic_rx_stats[i].name, ETH_GSTRING_LEN);
 			data += ETH_GSTRING_LEN;
 		}
+		memcpy(data, ENIC_TIMESTAMP_STAT_NAME, ETH_GSTRING_LEN);
+		data += ETH_GSTRING_LEN;
 		break;
 	}
 }
@@ -232,7 +236,7 @@ static int enic_get_sset_count(struct net_device *netdev, int sset)
 {
 	switch (sset) {
 	case ETH_SS_STATS:
-		return enic_n_tx_stats + enic_n_rx_stats;
+		return enic_n_tx_stats + enic_n_rx_stats + 1;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -251,6 +255,7 @@ static void enic_get_ethtool_stats(struct net_device *netdev,
 		*(data++) = ((u64 *)&vstats->tx)[enic_tx_stats[i].offset];
 	for (i = 0; i < enic_n_rx_stats; i++)
 		*(data++) = ((u64 *)&vstats->rx)[enic_rx_stats[i].offset];
+	*(data++) = vstats->timestamp_ns;
 }
 
 static u32 enic_get_msglevel(struct net_device *netdev)
diff --git a/drivers/net/enic/vnic_dev.c b/drivers/net/enic/vnic_dev.c
index 8c4c8cf..656871a 100644
--- a/drivers/net/enic/vnic_dev.c
+++ b/drivers/net/enic/vnic_dev.c
@@ -467,6 +467,7 @@ int vnic_dev_stats_dump(struct vnic_dev *vdev, struct vnic_stats **stats)
 		if (!vdev->stats)
 			return -ENOMEM;
 	}
+	memset(vdev->stats, 0, sizeof(*vdev->stats));
 
 	*stats = vdev->stats;
 	a0 = vdev->stats_pa;
diff --git a/drivers/net/enic/vnic_stats.h b/drivers/net/enic/vnic_stats.h
index 77750ec..9d5158d 100644
--- a/drivers/net/enic/vnic_stats.h
+++ b/drivers/net/enic/vnic_stats.h
@@ -65,6 +65,7 @@ struct vnic_rx_stats {
 struct vnic_stats {
 	struct vnic_tx_stats tx;
 	struct vnic_rx_stats rx;
+	u64 timestamp_ns;
 };
 
 #endif /* _VNIC_STATS_H_ */


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [net-next-2.6 PATCH] enic: Add timestamp to network interface stats
  2011-08-05  0:11 [net-next-2.6 PATCH] enic: Add timestamp to network interface stats Danny Guo
@ 2011-08-05  4:42 ` Stephen Hemminger
  2011-08-06  6:25 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: Stephen Hemminger @ 2011-08-05  4:42 UTC (permalink / raw)
  To: Danny Guo; +Cc: davem, netdev

On Thu, 04 Aug 2011 17:11:24 -0700
Danny Guo <dannguo@cisco.com> wrote:

> From: Danny Guo <dannguo@cisco.com>
> 
> This patch adds timestamps in ethtool stats. It makes it easier to provide scripts to users to calculate throughput, etc. It also allows software to synchronize timestamps with host time for correlating host events with stats collection.
> 
> Signed-off-by: Danny Guo <dannguo@cisco.com>
> Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
> Signed-off-by: Roopa Prabhu <roprabhu@cisco.com>
> Signed-off-by: David Wang <dwang2@cisco.com>


The concept is okay, but don't like drivers doing device specific
things all the time.

> ---
>  drivers/net/enic/enic.h       |    2 +-
>  drivers/net/enic/enic_main.c  |    7 ++++++-
>  drivers/net/enic/vnic_dev.c   |    1 +
>  drivers/net/enic/vnic_stats.h |    1 +
>  4 files changed, 9 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
> index ce76d9a..2e58bdc 100644
> --- a/drivers/net/enic/enic.h
> +++ b/drivers/net/enic/enic.h
> @@ -32,7 +32,7 @@
>  
>  #define DRV_NAME		"enic"
>  #define DRV_DESCRIPTION		"Cisco VIC Ethernet NIC Driver"
> -#define DRV_VERSION		"2.1.1.24"
> +#define DRV_VERSION		"2.1.1.25"
>  #define DRV_COPYRIGHT		"Copyright 2008-2011 Cisco Systems, Inc"
>  
>  #define ENIC_BARS_MAX		6
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 67a27cd..a3f61c1 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -121,6 +121,8 @@ static const struct enic_stat enic_rx_stats[] = {
>  static const unsigned int enic_n_tx_stats = ARRAY_SIZE(enic_tx_stats);
>  static const unsigned int enic_n_rx_stats = ARRAY_SIZE(enic_rx_stats);
>  
> +#define ENIC_TIMESTAMP_STAT_NAME	"timestamp(ns)"
> +
>  static int enic_is_dynamic(struct enic *enic)
>  {
>  	return enic->pdev->device == PCI_DEVICE_ID_CISCO_VIC_ENET_DYN;
> @@ -224,6 +226,8 @@ static void enic_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
>  			memcpy(data, enic_rx_stats[i].name, ETH_GSTRING_LEN);
>  			data += ETH_GSTRING_LEN;
>  		}
> +		memcpy(data, ENIC_TIMESTAMP_STAT_NAME, ETH_GSTRING_LEN);
> +		data += ETH_GSTRING_LEN;
>  		break;
>  	}
>  }
> @@ -232,7 +236,7 @@ static int enic_get_sset_count(struct net_device *netdev, int sset)
>  {
>  	switch (sset) {
>  	case ETH_SS_STATS:
> -		return enic_n_tx_stats + enic_n_rx_stats;
> +		return enic_n_tx_stats + enic_n_rx_stats + 1;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -251,6 +255,7 @@ static void enic_get_ethtool_stats(struct net_device *netdev,
>  		*(data++) = ((u64 *)&vstats->tx)[enic_tx_stats[i].offset];
>  	for (i = 0; i < enic_n_rx_stats; i++)
>  		*(data++) = ((u64 *)&vstats->rx)[enic_rx_stats[i].offset];
> +	*(data++) = vstats->timestamp_ns;
>  }
>  
>  static u32 enic_get_msglevel(struct net_device *netdev)
> diff --git a/drivers/net/enic/vnic_dev.c b/drivers/net/enic/vnic_dev.c
> index 8c4c8cf..656871a 100644
> --- a/drivers/net/enic/vnic_dev.c
> +++ b/drivers/net/enic/vnic_dev.c
> @@ -467,6 +467,7 @@ int vnic_dev_stats_dump(struct vnic_dev *vdev, struct vnic_stats **stats)
>  		if (!vdev->stats)
>  			return -ENOMEM;
>  	}
> +	memset(vdev->stats, 0, sizeof(*vdev->stats));

No, reading stats should not clear them!


>  	*stats = vdev->stats;
>  	a0 = vdev->stats_pa;
> diff --git a/drivers/net/enic/vnic_stats.h b/drivers/net/enic/vnic_stats.h
> index 77750ec..9d5158d 100644
> --- a/drivers/net/enic/vnic_stats.h
> +++ b/drivers/net/enic/vnic_stats.h
> @@ -65,6 +65,7 @@ struct vnic_rx_stats {
>  struct vnic_stats {
>  	struct vnic_tx_stats tx;
>  	struct vnic_rx_stats rx;
> +	u64 timestamp_ns;
>  };

Two problems: one is that you are exposing a timestamp in a random unit, and
the other problem is that is never set (at least in this code).


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [net-next-2.6 PATCH] enic: Add timestamp to network interface stats
  2011-08-05  0:11 [net-next-2.6 PATCH] enic: Add timestamp to network interface stats Danny Guo
  2011-08-05  4:42 ` Stephen Hemminger
@ 2011-08-06  6:25 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2011-08-06  6:25 UTC (permalink / raw)
  To: dannguo; +Cc: netdev

From: Danny Guo <dannguo@cisco.com>
Date: Thu, 04 Aug 2011 17:11:24 -0700

> From: Danny Guo <dannguo@cisco.com>
> 
> This patch adds timestamps in ethtool stats. It makes it easier to provide scripts to users to calculate throughput, etc. It also allows software to synchronize timestamps with host time for correlating host events with stats collection.
> 
> Signed-off-by: Danny Guo <dannguo@cisco.com>
> Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
> Signed-off-by: Roopa Prabhu <roprabhu@cisco.com>
> Signed-off-by: David Wang <dwang2@cisco.com>

It's "easier" but only for your specific driver if we let this patch
go in.

Everyone tends to come up with the "easy" but localized and selfish
solution.

We have not one but several rate estimators in the kernel, if that's
unusable then fix them instead of just going for private facilities
that only will work with your driver.

I'm not applying this, sorry.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-08-06  6:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-05  0:11 [net-next-2.6 PATCH] enic: Add timestamp to network interface stats Danny Guo
2011-08-05  4:42 ` Stephen Hemminger
2011-08-06  6:25 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.