| Message ID | 20180613193608.444-1-yidren@linuxonhyperv.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] hv_netvsc: Add per-cpu ethtool stats for netvsc | expand |
On 06/13/2018 12:36 PM, Yidong Ren wrote: > From: Yidong Ren <yidren@microsoft.com> > > This patch implements following ethtool stats fields for netvsc: > cpu<n>_tx/rx_packets/bytes > cpu<n>_vf_tx/rx_packets/bytes ... > > + pcpu_sum = alloc_percpu(struct netvsc_ethtool_pcpu_stats); > + netvsc_get_pcpu_stats(dev, pcpu_sum); > + for_each_present_cpu(cpu) { > + struct netvsc_ethtool_pcpu_stats *this_sum = > + per_cpu_ptr(pcpu_sum, cpu); > + for (j = 0; j < ARRAY_SIZE(pcpu_stats); j++) > + data[i++] = *(u64 *)((void *)this_sum > + + pcpu_stats[j].offset); > + } > + free_percpu(pcpu_sum); > Using alloc_percpu() / free_percpu() for a short section of code makes no sense. You actually want to allocate memory local to this cpu, possibly in one chunk, not spread all over the places. kvmalloc(nr_cpu_ids * sizeof(struct netvsc_ethtool_pcpu_stats)) should be really better, since it would most of the time be satisfied by a single kmalloc()
> From: Eric Dumazet <eric.dumazet@gmail.com> > You actually want to allocate memory local to this cpu, possibly in one chunk, > not spread all over the places. > > kvmalloc(nr_cpu_ids * sizeof(struct netvsc_ethtool_pcpu_stats)) should be > really better, since it would most of the time be satisfied by a single kmalloc() Got it. I'm just trying to allocate memory for each cpu. It doesn't have to be __percpu variable.
On Wed, 13 Jun 2018 12:36:08 -0700 Yidong Ren <yidren@linuxonhyperv.com> wrote: > From: Yidong Ren <yidren@microsoft.com> > > This patch implements following ethtool stats fields for netvsc: > cpu<n>_tx/rx_packets/bytes > cpu<n>_vf_tx/rx_packets/bytes > > Corresponding per-cpu counters exist in current code. Exposing these > counters will help troubleshooting performance issues. > > Signed-off-by: Yidong Ren <yidren@microsoft.com> > --- > Changes in v2: > - Remove cpp style comment > - Resubmit after freeze > > drivers/net/hyperv/hyperv_net.h | 11 +++++ > drivers/net/hyperv/netvsc_drv.c | 104 +++++++++++++++++++++++++++++++++++++++- > 2 files changed, 113 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index 23304ac..c825353 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -873,6 +873,17 @@ struct netvsc_ethtool_stats { > unsigned long wake_queue; > }; > > +struct netvsc_ethtool_pcpu_stats { > + u64 rx_packets; > + u64 rx_bytes; > + u64 tx_packets; > + u64 tx_bytes; > + u64 vf_rx_packets; > + u64 vf_rx_bytes; > + u64 vf_tx_packets; > + u64 vf_tx_bytes; > +}; > + > struct netvsc_vf_pcpu_stats { > u64 rx_packets; > u64 rx_bytes; > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 7b18a8c..6803aae 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -1105,6 +1105,66 @@ static void netvsc_get_vf_stats(struct net_device *net, > } > } > > +static void netvsc_get_pcpu_stats(struct net_device *net, > + struct netvsc_ethtool_pcpu_stats > + __percpu *pcpu_tot) > +{ > + struct net_device_context *ndev_ctx = netdev_priv(net); > + struct netvsc_device *nvdev = rcu_dereference_rtnl(ndev_ctx->nvdev); > + int i; > + > + /* fetch percpu stats of vf */ > + for_each_possible_cpu(i) { > + const struct netvsc_vf_pcpu_stats *stats = > + per_cpu_ptr(ndev_ctx->vf_stats, i); > + struct netvsc_ethtool_pcpu_stats *this_tot = > + per_cpu_ptr(pcpu_tot, i); > + unsigned int start; > + > + do { > + start = u64_stats_fetch_begin_irq(&stats->syncp); > + this_tot->vf_rx_packets = stats->rx_packets; > + this_tot->vf_tx_packets = stats->tx_packets; > + this_tot->vf_rx_bytes = stats->rx_bytes; > + this_tot->vf_tx_bytes = stats->tx_bytes; > + } while (u64_stats_fetch_retry_irq(&stats->syncp, start)); > + this_tot->rx_packets = this_tot->vf_rx_packets; > + this_tot->tx_packets = this_tot->vf_tx_packets; > + this_tot->rx_bytes = this_tot->vf_rx_bytes; > + this_tot->tx_bytes = this_tot->vf_tx_bytes; > + } > + > + /* fetch percpu stats of netvsc */ > + for (i = 0; i < nvdev->num_chn; i++) { > + const struct netvsc_channel *nvchan = &nvdev->chan_table[i]; > + const struct netvsc_stats *stats; > + struct netvsc_ethtool_pcpu_stats *this_tot = > + per_cpu_ptr(pcpu_tot, nvchan->channel->target_cpu); > + u64 packets, bytes; > + unsigned int start; > + > + stats = &nvchan->tx_stats; > + do { > + start = u64_stats_fetch_begin_irq(&stats->syncp); > + packets = stats->packets; > + bytes = stats->bytes; > + } while (u64_stats_fetch_retry_irq(&stats->syncp, start)); > + > + this_tot->tx_bytes += bytes; > + this_tot->tx_packets += packets; > + > + stats = &nvchan->rx_stats; > + do { > + start = u64_stats_fetch_begin_irq(&stats->syncp); > + packets = stats->packets; > + bytes = stats->bytes; > + } while (u64_stats_fetch_retry_irq(&stats->syncp, start)); > + > + this_tot->rx_bytes += bytes; > + this_tot->rx_packets += packets; > + } > +} > + > static void netvsc_get_stats64(struct net_device *net, > struct rtnl_link_stats64 *t) > { > @@ -1202,6 +1262,23 @@ static const struct { > { "rx_no_memory", offsetof(struct netvsc_ethtool_stats, rx_no_memory) }, > { "stop_queue", offsetof(struct netvsc_ethtool_stats, stop_queue) }, > { "wake_queue", offsetof(struct netvsc_ethtool_stats, wake_queue) }, > +}, pcpu_stats[] = { > + { "cpu%u_rx_packets", > + offsetof(struct netvsc_ethtool_pcpu_stats, rx_packets) }, > + { "cpu%u_rx_bytes", > + offsetof(struct netvsc_ethtool_pcpu_stats, rx_bytes) }, > + { "cpu%u_tx_packets", > + offsetof(struct netvsc_ethtool_pcpu_stats, tx_packets) }, > + { "cpu%u_tx_bytes", > + offsetof(struct netvsc_ethtool_pcpu_stats, tx_bytes) }, > + { "cpu%u_vf_rx_packets", > + offsetof(struct netvsc_ethtool_pcpu_stats, vf_rx_packets) }, > + { "cpu%u_vf_rx_bytes", > + offsetof(struct netvsc_ethtool_pcpu_stats, vf_rx_bytes) }, > + { "cpu%u_vf_tx_packets", > + offsetof(struct netvsc_ethtool_pcpu_stats, vf_tx_packets) }, > + { "cpu%u_vf_tx_bytes", > + offsetof(struct netvsc_ethtool_pcpu_stats, vf_tx_bytes) }, > }, vf_stats[] = { > { "vf_rx_packets", offsetof(struct netvsc_vf_pcpu_stats, rx_packets) }, > { "vf_rx_bytes", offsetof(struct netvsc_vf_pcpu_stats, rx_bytes) }, > @@ -1213,6 +1290,9 @@ static const struct { > #define NETVSC_GLOBAL_STATS_LEN ARRAY_SIZE(netvsc_stats) > #define NETVSC_VF_STATS_LEN ARRAY_SIZE(vf_stats) > > +/* statistics per queue (rx/tx packets/bytes) */ > +#define NETVSC_PCPU_STATS_LEN (num_present_cpus() * ARRAY_SIZE(pcpu_stats)) Even though Hyper-V/Azure does not support hot plug cpu's it might be better to num_cpu_possible to avoid any possible future surprises.
> From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf > Of Stephen Hemminger > > +/* statistics per queue (rx/tx packets/bytes) */ #define > > +NETVSC_PCPU_STATS_LEN (num_present_cpus() * > ARRAY_SIZE(pcpu_stats)) > > Even though Hyper-V/Azure does not support hot plug cpu's it might be > better to num_cpu_possible to avoid any possible future surprises. That will create a very long output (num_cpu_possible = 128 on my machine) for ethtool, While doesn't provide additional info. num_present_cpus() would cause problem only if someone removed cpu between netvsc_get_sset_count() and netvsc_get_strings() and netvsc_get_ethtool_stats(). An alternative way could be: Check all stats, and only output if not zero. This need to be done in two pass. First pass to get the correct count, second pass to print the number. Is there an elegant way to do this?
On Wed, 13 Jun 2018 22:03:34 +0000 Yidong Ren <Yidong.Ren@microsoft.com> wrote: > > From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf > > Of Stephen Hemminger > > > +/* statistics per queue (rx/tx packets/bytes) */ #define > > > +NETVSC_PCPU_STATS_LEN (num_present_cpus() * > > ARRAY_SIZE(pcpu_stats)) > > > > Even though Hyper-V/Azure does not support hot plug cpu's it might be > > better to num_cpu_possible to avoid any possible future surprises. > > That will create a very long output (num_cpu_possible = 128 on my machine) for ethtool, > While doesn't provide additional info. > num_present_cpus() would cause problem only if someone removed cpu > between netvsc_get_sset_count() and netvsc_get_strings() and netvsc_get_ethtool_stats(). > > An alternative way could be: Check all stats, and only output if not zero. > This need to be done in two pass. First pass to get the correct count, second pass to print the number. > Is there an elegant way to do this? Ok, but there is a race between getting names and getting statistics. If a cpu was added/removed then statistics would not match.
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index 23304ac..c825353 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -873,6 +873,17 @@ struct netvsc_ethtool_stats { unsigned long wake_queue; }; +struct netvsc_ethtool_pcpu_stats { + u64 rx_packets; + u64 rx_bytes; + u64 tx_packets; + u64 tx_bytes; + u64 vf_rx_packets; + u64 vf_rx_bytes; + u64 vf_tx_packets; + u64 vf_tx_bytes; +}; + struct netvsc_vf_pcpu_stats { u64 rx_packets; u64 rx_bytes; diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 7b18a8c..6803aae 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -1105,6 +1105,66 @@ static void netvsc_get_vf_stats(struct net_device *net, } } +static void netvsc_get_pcpu_stats(struct net_device *net, + struct netvsc_ethtool_pcpu_stats + __percpu *pcpu_tot) +{ + struct net_device_context *ndev_ctx = netdev_priv(net); + struct netvsc_device *nvdev = rcu_dereference_rtnl(ndev_ctx->nvdev); + int i; + + /* fetch percpu stats of vf */ + for_each_possible_cpu(i) { + const struct netvsc_vf_pcpu_stats *stats = + per_cpu_ptr(ndev_ctx->vf_stats, i); + struct netvsc_ethtool_pcpu_stats *this_tot = + per_cpu_ptr(pcpu_tot, i); + unsigned int start; + + do { + start = u64_stats_fetch_begin_irq(&stats->syncp); + this_tot->vf_rx_packets = stats->rx_packets; + this_tot->vf_tx_packets = stats->tx_packets; + this_tot->vf_rx_bytes = stats->rx_bytes; + this_tot->vf_tx_bytes = stats->tx_bytes; + } while (u64_stats_fetch_retry_irq(&stats->syncp, start)); + this_tot->rx_packets = this_tot->vf_rx_packets; + this_tot->tx_packets = this_tot->vf_tx_packets; + this_tot->rx_bytes = this_tot->vf_rx_bytes; + this_tot->tx_bytes = this_tot->vf_tx_bytes; + } + + /* fetch percpu stats of netvsc */ + for (i = 0; i < nvdev->num_chn; i++) { + const struct netvsc_channel *nvchan = &nvdev->chan_table[i]; + const struct netvsc_stats *stats; + struct netvsc_ethtool_pcpu_stats *this_tot = + per_cpu_ptr(pcpu_tot, nvchan->channel->target_cpu); + u64 packets, bytes; + unsigned int start; + + stats = &nvchan->tx_stats; + do { + start = u64_stats_fetch_begin_irq(&stats->syncp); + packets = stats->packets; + bytes = stats->bytes; + } while (u64_stats_fetch_retry_irq(&stats->syncp, start)); + + this_tot->tx_bytes += bytes; + this_tot->tx_packets += packets; + + stats = &nvchan->rx_stats; + do { + start = u64_stats_fetch_begin_irq(&stats->syncp); + packets = stats->packets; + bytes = stats->bytes; + } while (u64_stats_fetch_retry_irq(&stats->syncp, start)); + + this_tot->rx_bytes += bytes; + this_tot->rx_packets += packets; + } +} + static void netvsc_get_stats64(struct net_device *net, struct rtnl_link_stats64 *t) { @@ -1202,6 +1262,23 @@ static const struct { { "rx_no_memory", offsetof(struct netvsc_ethtool_stats, rx_no_memory) }, { "stop_queue", offsetof(struct netvsc_ethtool_stats, stop_queue) }, { "wake_queue", offsetof(struct netvsc_ethtool_stats, wake_queue) }, +}, pcpu_stats[] = { + { "cpu%u_rx_packets", + offsetof(struct netvsc_ethtool_pcpu_stats, rx_packets) }, + { "cpu%u_rx_bytes", + offsetof(struct netvsc_ethtool_pcpu_stats, rx_bytes) }, + { "cpu%u_tx_packets", + offsetof(struct netvsc_ethtool_pcpu_stats, tx_packets) }, + { "cpu%u_tx_bytes", + offsetof(struct netvsc_ethtool_pcpu_stats, tx_bytes) }, + { "cpu%u_vf_rx_packets", + offsetof(struct netvsc_ethtool_pcpu_stats, vf_rx_packets) }, + { "cpu%u_vf_rx_bytes", + offsetof(struct netvsc_ethtool_pcpu_stats, vf_rx_bytes) }, + { "cpu%u_vf_tx_packets", + offsetof(struct netvsc_ethtool_pcpu_stats, vf_tx_packets) }, + { "cpu%u_vf_tx_bytes", + offsetof(struct netvsc_ethtool_pcpu_stats, vf_tx_bytes) }, }, vf_stats[] = { { "vf_rx_packets", offsetof(struct netvsc_vf_pcpu_stats, rx_packets) }, { "vf_rx_bytes", offsetof(struct netvsc_vf_pcpu_stats, rx_bytes) }, @@ -1213,6 +1290,9 @@ static const struct { #define NETVSC_GLOBAL_STATS_LEN ARRAY_SIZE(netvsc_stats) #define NETVSC_VF_STATS_LEN ARRAY_SIZE(vf_stats) +/* statistics per queue (rx/tx packets/bytes) */ +#define NETVSC_PCPU_STATS_LEN (num_present_cpus() * ARRAY_SIZE(pcpu_stats)) + /* 4 statistics per queue (rx/tx packets/bytes) */ #define NETVSC_QUEUE_STATS_LEN(dev) ((dev)->num_chn * 4) @@ -1228,6 +1308,7 @@ static int netvsc_get_sset_count(struct net_device *dev, int string_set) case ETH_SS_STATS: return NETVSC_GLOBAL_STATS_LEN + NETVSC_VF_STATS_LEN + + NETVSC_PCPU_STATS_LEN + NETVSC_QUEUE_STATS_LEN(nvdev); default: return -EINVAL; @@ -1242,9 +1323,10 @@ static void netvsc_get_ethtool_stats(struct net_device *dev, const void *nds = &ndc->eth_stats; const struct netvsc_stats *qstats; struct netvsc_vf_pcpu_stats sum; + struct netvsc_ethtool_pcpu_stats __percpu *pcpu_sum; unsigned int start; u64 packets, bytes; - int i, j; + int i, j, cpu; if (!nvdev) return; @@ -1256,6 +1338,17 @@ static void netvsc_get_ethtool_stats(struct net_device *dev, for (j = 0; j < NETVSC_VF_STATS_LEN; j++) data[i++] = *(u64 *)((void *)&sum + vf_stats[j].offset); + pcpu_sum = alloc_percpu(struct netvsc_ethtool_pcpu_stats); + netvsc_get_pcpu_stats(dev, pcpu_sum); + for_each_present_cpu(cpu) { + struct netvsc_ethtool_pcpu_stats *this_sum = + per_cpu_ptr(pcpu_sum, cpu); + for (j = 0; j < ARRAY_SIZE(pcpu_stats); j++) + data[i++] = *(u64 *)((void *)this_sum + + pcpu_stats[j].offset); + } + free_percpu(pcpu_sum); + for (j = 0; j < nvdev->num_chn; j++) { qstats = &nvdev->chan_table[j].tx_stats; @@ -1283,7 +1376,7 @@ static void netvsc_get_strings(struct net_device *dev, u32 stringset, u8 *data) struct net_device_context *ndc = netdev_priv(dev); struct netvsc_device *nvdev = rtnl_dereference(ndc->nvdev); u8 *p = data; - int i; + int i, cpu; if (!nvdev) return; @@ -1300,6 +1393,13 @@ static void netvsc_get_strings(struct net_device *dev, u32 stringset, u8 *data) p += ETH_GSTRING_LEN; } + for_each_present_cpu(cpu) { + for (i = 0; i < ARRAY_SIZE(pcpu_stats); i++) { + sprintf(p, pcpu_stats[i].name, cpu); + p += ETH_GSTRING_LEN; + } + } + for (i = 0; i < nvdev->num_chn; i++) { sprintf(p, "tx_queue_%u_packets", i); p += ETH_GSTRING_LEN;