Messages in this thread | | | Date | Wed, 06 Jun 2012 13:02:19 +0800 | From | Jason Wang <> | Subject | Re: [net-next RFC PATCH] virtio_net: collect satistics and export through ethtool |
| |
On 06/05/2012 06:10 PM, Michael S. Tsirkin wrote: > On Tue, Jun 05, 2012 at 04:38:41PM +0800, Jason Wang wrote: >> Satistics counters is useful for debugging and performance optimization, so this >> patch lets virtio_net driver collect following and export them to userspace >> through "ethtool -S": >> >> - number of packets sent/received >> - number of bytes sent/received >> - number of callbacks for tx/rx >> - number of kick for tx/rx >> - number of bytes/packets queued for tx >> >> As virtnet_stats were per-cpu, so both per-cpu and gloabl satistics were exposed >> like: >> >> NIC statistics: >> tx_bytes[0]: 2551 >> tx_packets[0]: 12 >> tx_kick[0]: 12 >> tx_callbacks[0]: 1 >> tx_queued_packets[0]: 12 >> tx_queued_bytes[0]: 3055 >> rx_bytes[0]: 0 >> rx_packets[0]: 0 >> rx_kick[0]: 0 >> rx_callbacks[0]: 0 >> tx_bytes[1]: 5575 >> tx_packets[1]: 37 >> tx_kick[1]: 38 >> tx_callbacks[1]: 0 >> tx_queued_packets[1]: 38 >> tx_queued_bytes[1]: 5217 >> rx_bytes[1]: 4175 >> rx_packets[1]: 25 >> rx_kick[1]: 1 >> rx_callbacks[1]: 16 >> tx_bytes: 8126 >> tx_packets: 49 >> tx_kick: 50 >> tx_callbacks: 1 >> tx_queued_packets: 50 >> tx_queued_bytes: 8272 >> rx_bytes: 4175 >> rx_packets: 25 >> rx_kick: 1 >> rx_callbacks: 16 >> >> TODO: >> >> - more satistics >> - unitfy the ndo_get_stats64 and get_ethtool_stats >> - calculate the pending bytes/pkts >> >> Signed-off-by: Jason Wang<jasowang@redhat.com> >> --- >> drivers/net/virtio_net.c | 130 +++++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 127 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 5214b1e..7ab0cc1 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -41,6 +41,10 @@ module_param(gso, bool, 0444); >> #define VIRTNET_SEND_COMMAND_SG_MAX 2 >> #define VIRTNET_DRIVER_VERSION "1.0.0" >> >> +#define VIRTNET_STAT_OFF(m) offsetof(struct virtnet_stats, m) >> +#define VIRTNET_STAT(stat, i) (*((u64 *)((char *)stat + \ > What's going on? Why cast to char *?
It's used to let the pointer advance at the unit of bytes instead of the whole stat strcuture. >> + virtnet_stats_str_attr[i].stat_offset))) > These are confusing unless you see what virtnet_stats_str_attr > is so please move them near that definition.
ok. >> + >> struct virtnet_stats { >> struct u64_stats_sync syncp; >> u64 tx_bytes; >> @@ -48,8 +52,33 @@ struct virtnet_stats { >> >> u64 rx_bytes; >> u64 rx_packets; >> + >> + u64 tx_kick; >> + u64 rx_kick; >> + u64 tx_callbacks; >> + u64 rx_callbacks; >> + u64 tx_queued_packets; >> + u64 tx_queued_bytes; >> +}; > I have an idea (not a must): why don't we simply create an enum > enum virtnet_stats { > VIRTNET_TX_KICK, > VIRTNET_RX_KICK, > .... > VIRTNET_MAX_STAT, > } > > > now stats can just do > stats->data[VIRTNET_RX_KICK] instead of stats->rx_kick > which is not a big problem, but copying them in bulk > becomes straight-forward, no need for macros at all. > > If we decide to do this, needs to be a separate patch, > then this one on top.
Make sense, would do this. >> + >> +static struct { > static const. > >> + char string[ETH_GSTRING_LEN]; >> + int stat_offset; >> +} virtnet_stats_str_attr[] = { >> + { "tx_bytes", VIRTNET_STAT_OFF(tx_bytes)}, >> + { "tx_packets", VIRTNET_STAT_OFF(tx_packets)}, >> + { "tx_kick", VIRTNET_STAT_OFF(tx_kick)}, >> + { "tx_callbacks", VIRTNET_STAT_OFF(tx_callbacks)}, >> + { "tx_queued_packets", VIRTNET_STAT_OFF(tx_queued_packets)}, >> + { "tx_queued_bytes", VIRTNET_STAT_OFF(tx_queued_bytes)}, >> + { "rx_bytes" , VIRTNET_STAT_OFF(rx_bytes)}, >> + { "rx_packets", VIRTNET_STAT_OFF(rx_packets)}, >> + { "rx_kick", VIRTNET_STAT_OFF(rx_kick)}, >> + { "rx_callbacks", VIRTNET_STAT_OFF(rx_callbacks)}, > VIRTNET_STAT_OFF does not save much here, but if you are after > saving characters then make the macro instanciate the string > as well. > >> }; >> >> +#define VIRTNET_NUM_STATS ARRAY_SIZE(virtnet_stats_str_attr) >> + > if you pass virtnet_stats_str_attr to VIRTNET_STAT macro, > then it's explicit and VIRTNET_NUM_STATS won't be needed either.
It's used to report the number of satistics through .get_sset_count. > >> struct virtnet_info { >> struct virtio_device *vdev; >> struct virtqueue *rvq, *svq, *cvq; >> @@ -142,6 +171,11 @@ static struct page *get_a_page(struct virtnet_info *vi, gfp_t gfp_mask) >> static void skb_xmit_done(struct virtqueue *svq) >> { >> struct virtnet_info *vi = svq->vdev->priv; >> + struct virtnet_stats *stats = this_cpu_ptr(vi->stats); >> + >> + u64_stats_update_begin(&stats->syncp); >> + stats->tx_callbacks++; >> + u64_stats_update_end(&stats->syncp); >> >> /* Suppress further interrupts. */ >> virtqueue_disable_cb(svq); >> @@ -461,6 +495,7 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp) >> { >> int err; >> bool oom; >> + struct virtnet_stats *stats = this_cpu_ptr(vi->stats); >> >> do { >> if (vi->mergeable_rx_bufs) >> @@ -477,13 +512,24 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp) >> } while (err> 0); >> if (unlikely(vi->num> vi->max)) >> vi->max = vi->num; >> - virtqueue_kick(vi->rvq); >> + if (virtqueue_kick_prepare(vi->rvq)) { >> + virtqueue_notify(vi->rvq); >> + u64_stats_update_begin(&stats->syncp); >> + stats->rx_kick++; >> + u64_stats_update_end(&stats->syncp); >> + } >> return !oom; >> } >> >> static void skb_recv_done(struct virtqueue *rvq) >> { >> struct virtnet_info *vi = rvq->vdev->priv; >> + struct virtnet_stats *stats = this_cpu_ptr(vi->stats); >> + >> + u64_stats_update_begin(&stats->syncp); >> + stats->rx_callbacks++; >> + u64_stats_update_end(&stats->syncp); >> + >> /* Schedule NAPI, Suppress further interrupts if successful. */ >> if (napi_schedule_prep(&vi->napi)) { >> virtqueue_disable_cb(rvq); >> @@ -626,7 +672,9 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) >> static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) >> { >> struct virtnet_info *vi = netdev_priv(dev); >> + struct virtnet_stats *stats = this_cpu_ptr(vi->stats); >> int capacity; >> + bool kick; >> >> /* Free up any pending old buffers before queueing new ones. */ >> free_old_xmit_skbs(vi); >> @@ -651,7 +699,17 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) >> kfree_skb(skb); >> return NETDEV_TX_OK; >> } >> - virtqueue_kick(vi->svq); >> + >> + kick = virtqueue_kick_prepare(vi->svq); >> + if (kick) > probably > if (unlikely(kick)) > >> + virtqueue_notify(vi->svq); >> + >> + u64_stats_update_begin(&stats->syncp); >> + if (kick) > this too > >> + stats->tx_kick++; >> + stats->tx_queued_bytes += skb->len; >> + stats->tx_queued_packets++; >> + u64_stats_update_end(&stats->syncp); >> >> /* Don't wait up for transmitted skbs to be freed. */ >> skb_orphan(skb); >> @@ -926,7 +984,6 @@ static void virtnet_get_ringparam(struct net_device *dev, >> >> } >> >> - >> static void virtnet_get_drvinfo(struct net_device *dev, >> struct ethtool_drvinfo *info) >> { >> @@ -939,10 +996,77 @@ static void virtnet_get_drvinfo(struct net_device *dev, >> >> } >> >> +static void virtnet_get_strings(struct net_device *dev, u32 stringset, u8 *buf) >> +{ >> + int i, cpu; >> + switch (stringset) { >> + case ETH_SS_STATS: >> + for_each_possible_cpu(cpu) >> + for (i = 0; i< VIRTNET_NUM_STATS; i++) { >> + sprintf(buf, "%s[%u]", >> + virtnet_stats_str_attr[i].string, cpu); >> + buf += ETH_GSTRING_LEN; >> + } >> + for (i = 0; i< VIRTNET_NUM_STATS; i++) { >> + memcpy(buf, virtnet_stats_str_attr[i].string, >> + ETH_GSTRING_LEN); >> + buf += ETH_GSTRING_LEN; >> + } >> + break; >> + } >> +} >> + >> +static int virtnet_get_sset_count(struct net_device *dev, int sset) >> +{ >> + switch (sset) { >> + case ETH_SS_STATS: >> + return VIRTNET_NUM_STATS * (num_online_cpus() + 1); > This will allocate buffers for online cpus only, but the above > will fill them in for all possible cpus. > Will this overrun some buffer? >
Yes, a typo here, should be num_possible_cpus(). Thanks >> + default: >> + return -EOPNOTSUPP; >> + } >> +} >> + >> +static void virtnet_get_ethtool_stats(struct net_device *dev, >> + struct ethtool_stats *stats, u64 *buf) > The coding style says > Descendants are always substantially shorter than the parent and > are placed substantially to the right. > > you can't call it substantially to the right if it's to the left of > the opening '(' :), so please indent it aligning on the opening.
Looks like something wrong in my emacs c-style confiugration, would check it. >> +{ >> + struct virtnet_info *vi = netdev_priv(dev); >> + int cpu, i; >> + unsigned int start; >> + struct virtnet_stats sample, total; >> + >> + memset(&total, 0, sizeof(total)); >> + memset(&sample, 0, sizeof(sample)); >> + >> + for_each_possible_cpu(cpu) { >> + struct virtnet_stats *stats = per_cpu_ptr(vi->stats, cpu); >> + do { >> + start = u64_stats_fetch_begin(&stats->syncp); >> + for (i = 0; i< VIRTNET_NUM_STATS; i++) { >> + VIRTNET_STAT(&sample, i) = >> + VIRTNET_STAT(stats, i); > when you feel the need to break lines like this - don't :) > use an inline function instead. > >> + > kill empty line here >> + } > don't put {} around single statements pls.
Sure >> + } while (u64_stats_fetch_retry(&stats->syncp, start)); >> + for (i = 0; i< VIRTNET_NUM_STATS; i++) { >> + *buf = VIRTNET_STAT(&sample, i); >> + VIRTNET_STAT(&total, i) += VIRTNET_STAT(stats, i); >> + buf++; >> + } >> + } >> + >> + for (i = 0; i< VIRTNET_NUM_STATS; i++) { >> + *buf = VIRTNET_STAT(&total, i); >> + buf++; >> + } >> +} >> + >> static const struct ethtool_ops virtnet_ethtool_ops = { >> .get_drvinfo = virtnet_get_drvinfo, >> .get_link = ethtool_op_get_link, >> .get_ringparam = virtnet_get_ringparam, >> + .get_ethtool_stats = virtnet_get_ethtool_stats, >> + .get_strings = virtnet_get_strings, >> + .get_sset_count = virtnet_get_sset_count, >> }; >> >> #define MIN_MTU 68 > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
| |