From: Jason Wang <jasowang@redhat.com>
To: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Jesper Dangaard Brouer <hawk@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
Stanislav Fomichev <sdf@google.com>,
Amritha Nambiar <amritha.nambiar@intel.com>,
Larysa Zaremba <larysa.zaremba@intel.com>,
Sridhar Samudrala <sridhar.samudrala@intel.com>,
Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
virtualization@lists.linux.dev, bpf@vger.kernel.org
Subject: Re: [PATCH net-next v5 4/9] virtio_net: support device stats
Date: Mon, 15 Apr 2024 16:34:37 +0800 [thread overview]
Message-ID: <CACGkMEucuS0yGP1d9w3KEUigGusP_ks71KwZAwuBJNdZQ=v13Q@mail.gmail.com> (raw)
In-Reply-To: <1713168679.2714498-1-xuanzhuo@linux.alibaba.com>
On Mon, Apr 15, 2024 at 4:15 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Mon, 15 Apr 2024 14:45:36 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Mon, Apr 15, 2024 at 10:51 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Thu, 11 Apr 2024 14:09:24 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Wed, Apr 10, 2024 at 6:55 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > > On Wed, 10 Apr 2024 14:09:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > On Mon, Mar 18, 2024 at 7:06 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > >
> > > > > > > As the spec https://github.com/oasis-tcs/virtio-spec/commit/42f389989823039724f95bbbd243291ab0064f82
> > > > > > >
> > > > > > > make virtio-net support getting the stats from the device by ethtool -S
> > > > > > > <eth0>.
> > > > > > >
> > > > > > > Due to the numerous descriptors stats, an organization method is
> > > > > > > required. For this purpose, I have introduced the "virtnet_stats_map".
> > > > > > > Utilizing this array simplifies coding tasks such as generating field
> > > > > > > names, calculating buffer sizes for requests and responses, and parsing
> > > > > > > replies from the device. By iterating over the "virtnet_stats_map,"
> > > > > > > these operations become more streamlined and efficient.
> > > > > > >
> > > > > > > NIC statistics:
> > > > > > > rx0_packets: 582951
> > > > > > > rx0_bytes: 155307077
> > > > > > > rx0_drops: 0
> > > > > > > rx0_xdp_packets: 0
> > > > > > > rx0_xdp_tx: 0
> > > > > > > rx0_xdp_redirects: 0
> > > > > > > rx0_xdp_drops: 0
> > > > > > > rx0_kicks: 17007
> > > > > > > rx0_hw_packets: 2179409
> > > > > > > rx0_hw_bytes: 510015040
> > > > > > > rx0_hw_notifications: 0
> > > > > > > rx0_hw_interrupts: 0
> > > > > > > rx0_hw_drops: 12964
> > > > > > > rx0_hw_drop_overruns: 0
> > > > > > > rx0_hw_csum_valid: 2179409
> > > > > > > rx0_hw_csum_none: 0
> > > > > > > rx0_hw_csum_bad: 0
> > > > > > > rx0_hw_needs_csum: 2179409
> > > > > > > rx0_hw_ratelimit_packets: 0
> > > > > > > rx0_hw_ratelimit_bytes: 0
> > > > > > > tx0_packets: 15361
> > > > > > > tx0_bytes: 1918970
> > > > > > > tx0_xdp_tx: 0
> > > > > > > tx0_xdp_tx_drops: 0
> > > > > > > tx0_kicks: 15361
> > > > > > > tx0_timeouts: 0
> > > > > > > tx0_hw_packets: 32272
> > > > > > > tx0_hw_bytes: 4311698
> > > > > > > tx0_hw_notifications: 0
> > > > > > > tx0_hw_interrupts: 0
> > > > > > > tx0_hw_drops: 0
> > > > > > > tx0_hw_drop_malformed: 0
> > > > > > > tx0_hw_csum_none: 0
> > > > > > > tx0_hw_needs_csum: 32272
> > > > > > > tx0_hw_ratelimit_packets: 0
> > > > > > > tx0_hw_ratelimit_bytes: 0
> > > > > > >
> > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > > ---
> > > > > > > drivers/net/virtio_net.c | 401 ++++++++++++++++++++++++++++++++++++++-
> > > > > > > 1 file changed, 397 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > index 8cb5bdd7ad91..70c1d4e850e0 100644
> > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > @@ -128,6 +128,129 @@ static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
> > > > > > > #define VIRTNET_SQ_STATS_LEN ARRAY_SIZE(virtnet_sq_stats_desc)
> > > > > > > #define VIRTNET_RQ_STATS_LEN ARRAY_SIZE(virtnet_rq_stats_desc)
> > > > > > >
> > > > > > > +#define VIRTNET_STATS_DESC_CQ(name) \
> > > > > > > + {#name, offsetof(struct virtio_net_stats_cvq, name)}
> > > > > > > +
> > > > > > > +#define VIRTNET_STATS_DESC_RX(class, name) \
> > > > > > > + {#name, offsetof(struct virtio_net_stats_rx_ ## class, rx_ ## name)}
> > > > > > > +
> > > > > > > +#define VIRTNET_STATS_DESC_TX(class, name) \
> > > > > > > + {#name, offsetof(struct virtio_net_stats_tx_ ## class, tx_ ## name)}
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_cvq_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_CQ(command_num),
> > > > > > > + VIRTNET_STATS_DESC_CQ(ok_num),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_rx_basic_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_RX(basic, packets),
> > > > > > > + VIRTNET_STATS_DESC_RX(basic, bytes),
> > > > > > > +
> > > > > > > + VIRTNET_STATS_DESC_RX(basic, notifications),
> > > > > > > + VIRTNET_STATS_DESC_RX(basic, interrupts),
> > > > > > > +
> > > > > > > + VIRTNET_STATS_DESC_RX(basic, drops),
> > > > > > > + VIRTNET_STATS_DESC_RX(basic, drop_overruns),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_tx_basic_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_TX(basic, packets),
> > > > > > > + VIRTNET_STATS_DESC_TX(basic, bytes),
> > > > > > > +
> > > > > > > + VIRTNET_STATS_DESC_TX(basic, notifications),
> > > > > > > + VIRTNET_STATS_DESC_TX(basic, interrupts),
> > > > > > > +
> > > > > > > + VIRTNET_STATS_DESC_TX(basic, drops),
> > > > > > > + VIRTNET_STATS_DESC_TX(basic, drop_malformed),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_rx_csum_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_RX(csum, csum_valid),
> > > > > > > + VIRTNET_STATS_DESC_RX(csum, needs_csum),
> > > > > > > +
> > > > > > > + VIRTNET_STATS_DESC_RX(csum, csum_none),
> > > > > > > + VIRTNET_STATS_DESC_RX(csum, csum_bad),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_tx_csum_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_TX(csum, needs_csum),
> > > > > > > + VIRTNET_STATS_DESC_TX(csum, csum_none),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_rx_gso_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_RX(gso, gso_packets),
> > > > > > > + VIRTNET_STATS_DESC_RX(gso, gso_bytes),
> > > > > > > + VIRTNET_STATS_DESC_RX(gso, gso_packets_coalesced),
> > > > > > > + VIRTNET_STATS_DESC_RX(gso, gso_bytes_coalesced),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_tx_gso_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_TX(gso, gso_packets),
> > > > > > > + VIRTNET_STATS_DESC_TX(gso, gso_bytes),
> > > > > > > + VIRTNET_STATS_DESC_TX(gso, gso_segments),
> > > > > > > + VIRTNET_STATS_DESC_TX(gso, gso_segments_bytes),
> > > > > > > + VIRTNET_STATS_DESC_TX(gso, gso_packets_noseg),
> > > > > > > + VIRTNET_STATS_DESC_TX(gso, gso_bytes_noseg),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_rx_speed_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_RX(speed, ratelimit_packets),
> > > > > > > + VIRTNET_STATS_DESC_RX(speed, ratelimit_bytes),
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct virtnet_stat_desc virtnet_stats_tx_speed_desc[] = {
> > > > > > > + VIRTNET_STATS_DESC_TX(speed, ratelimit_packets),
> > > > > > > + VIRTNET_STATS_DESC_TX(speed, ratelimit_bytes),
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define VIRTNET_Q_TYPE_RX 0
> > > > > > > +#define VIRTNET_Q_TYPE_TX 1
> > > > > > > +#define VIRTNET_Q_TYPE_CQ 2
> > > > > > > +
> > > > > > > +struct virtnet_stats_map {
> > > > > > > + /* The stat type in bitmap. */
> > > > > > > + u64 stat_type;
> > > > > > > +
> > > > > > > + /* The bytes of the response for the stat. */
> > > > > > > + u32 len;
> > > > > > > +
> > > > > > > + /* The num of the response fields for the stat. */
> > > > > > > + u32 num;
> > > > > > > +
> > > > > > > + /* The type of queue corresponding to the statistics. (cq, rq, sq) */
> > > > > > > + u32 queue_type;
> > > > > > > +
> > > > > > > + /* The reply type of the stat. */
> > > > > > > + u8 reply_type;
> > > > > > > +
> > > > > > > + /* Describe the name and the offset in the response. */
> > > > > > > + const struct virtnet_stat_desc *desc;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define VIRTNET_DEVICE_STATS_MAP_ITEM(TYPE, type, queue_type) \
> > > > > > > + { \
> > > > > > > + VIRTIO_NET_STATS_TYPE_##TYPE, \
> > > > > > > + sizeof(struct virtio_net_stats_ ## type), \
> > > > > > > + ARRAY_SIZE(virtnet_stats_ ## type ##_desc), \
> > > > > > > + VIRTNET_Q_TYPE_##queue_type, \
> > > > > > > + VIRTIO_NET_STATS_TYPE_REPLY_##TYPE, \
> > > > > > > + &virtnet_stats_##type##_desc[0] \
> > > > > > > + }
> > > > > > > +
> > > > > > > +static struct virtnet_stats_map virtio_net_stats_map[] = {
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(CVQ, cvq, CQ),
> > > > > > > +
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(RX_BASIC, rx_basic, RX),
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(RX_CSUM, rx_csum, RX),
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(RX_GSO, rx_gso, RX),
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(RX_SPEED, rx_speed, RX),
> > > > > > > +
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(TX_BASIC, tx_basic, TX),
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(TX_CSUM, tx_csum, TX),
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(TX_GSO, tx_gso, TX),
> > > > > > > + VIRTNET_DEVICE_STATS_MAP_ITEM(TX_SPEED, tx_speed, TX),
> > > > > > > +};
> > > > > >
> > > > > > I think the reason you did this is to ease the future extensions but
> > > > > > multiple levels of nested macros makes the code hard to review. Any
> > > > > > way to eliminate this?
> > > > >
> > > > >
> > > > > NOT only for the future extensions.
> > > > >
> > > > > When we parse the reply from the device, we need to check the reply stats
> > > > > one by one, we need the stats info to help parse the stats.
> > > >
> > > > Yes, but I meant for example any reason why it can't be done by
> > > > extending virtnet_stat_desc ?
> > >
> > >
> > >
> > > You know, virtio_net_stats_map is way to organize the descs.
> > >
> > > This is used to avoid the big if-else when parsing the replys from the device.
> > >
> > > If no this map, we will have a big if-else like:
> > >
> > > if (reply.type == rx_basic) {
> > > /* do the same something */
> > > }
> > > if (reply.type == tx_basic) {
> > > /* do the same something */
> > > }
> > > if (reply.type == rx_csum) {
> > > /* do the same something */
> > > }
> > > if (reply.type == tx_csum) {
> > > /* do the same something */
> > > }
> > > if (reply.type == rx_gso) {
> > > /* do the same something */
> > > }
> > > if (reply.type == tx_gso) {
> > > /* do the same something */
> > > }
> > > if (reply.type == rx_speed) {
> > > /* do the same something */
> > > }
> > > if (reply.type == tx_speed) {
> > > /* do the same something */
> > > }
> > >
> > > I want to avoid this, so introducing this map.
> >
> > Could we have a function pointers array indexed by the type?
>
> Then these functions will be similar and mass.
>
> Maybe we can start with the if-else or the function.
> That will be easy to review. We can optimize on that.
Fine with me.
Thanks
>
> Thanks.
>
>
>
> >
> > Thanks
> >
> > >
> > > YES. I noticed other comments, but I think we should
> > > fix this problem firstly.
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > > >
> > > > > static void virtnet_fill_stats(struct virtnet_info *vi, u32 qid,
> > > > > struct virtnet_stats_ctx *ctx,
> > > > > const u8 *base, u8 type)
> > > > > {
> > > > > u32 queue_type, num_rx, num_tx, num_cq;
> > > > > struct virtnet_stats_map *m;
> > > > > u64 offset, bitmap;
> > > > > const __le64 *v;
> > > > > int i, j;
> > > > >
> > > > > num_rx = VIRTNET_RQ_STATS_LEN + ctx->desc_num[VIRTNET_Q_TYPE_RX];
> > > > > num_tx = VIRTNET_SQ_STATS_LEN + ctx->desc_num[VIRTNET_Q_TYPE_TX];
> > > > > num_cq = ctx->desc_num[VIRTNET_Q_TYPE_CQ];
> > > > >
> > > > > queue_type = vq_type(vi, qid);
> > > > > bitmap = ctx->bitmap[queue_type];
> > > > > offset = 0;
> > > > >
> > > > > if (queue_type == VIRTNET_Q_TYPE_TX) {
> > > > > offset = num_cq + num_rx * vi->curr_queue_pairs + num_tx * (qid / 2);
> > > > > offset += VIRTNET_SQ_STATS_LEN;
> > > > > } else if (queue_type == VIRTNET_Q_TYPE_RX) {
> > > > > offset = num_cq + num_rx * (qid / 2) + VIRTNET_RQ_STATS_LEN;
> > > > > }
> > > > >
> > > > > for (i = 0; i < ARRAY_SIZE(virtio_net_stats_map); ++i) {
> > > > > m = &virtio_net_stats_map[i];
> > > > >
> > > > > -> if (m->stat_type & bitmap)
> > > > > offset += m->num;
> > > > >
> > > > > -> if (type != m->reply_type)
> > > > > continue;
> > > > >
> > > > > for (j = 0; j < m->num; ++j) {
> > > > > v = (const __le64 *)(base + m->desc[j].offset);
> > > > > ctx->data[offset + j] = le64_to_cpu(*v);
> > > > > }
> > > > >
> > > > > break;
> > > > > }
> > > > > }
> > > > >
> > > > > Thanks.
> > > >
> > > > Btw, just a reminder, there are other comments for this patch.
> > > >
> > > > Thanks
> > > >
> > >
> >
>
next prev parent reply other threads:[~2024-04-15 8:34 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 11:05 [PATCH net-next v5 0/9] virtio-net: support device stats Xuan Zhuo
2024-03-18 11:05 ` [PATCH net-next v5 1/9] virtio_net: introduce device stats feature and structures Xuan Zhuo
2024-04-10 6:09 ` Jason Wang
2024-03-18 11:05 ` [PATCH net-next v5 2/9] virtio_net: virtnet_send_command supports command-specific-result Xuan Zhuo
2024-04-10 6:09 ` Jason Wang
2024-04-10 10:50 ` Xuan Zhuo
2024-03-18 11:05 ` [PATCH net-next v5 3/9] virtio_net: remove "_queue" from ethtool -S Xuan Zhuo
2024-04-10 6:09 ` Jason Wang
2024-03-18 11:05 ` [PATCH net-next v5 4/9] virtio_net: support device stats Xuan Zhuo
2024-04-10 6:09 ` Jason Wang
2024-04-10 10:52 ` Xuan Zhuo
2024-04-11 6:09 ` Jason Wang
2024-04-15 2:42 ` Xuan Zhuo
2024-04-15 6:45 ` Jason Wang
2024-04-15 8:11 ` Xuan Zhuo
2024-04-15 8:34 ` Jason Wang [this message]
2024-03-18 11:05 ` [PATCH net-next v5 5/9] virtio_net: stats map include driver stats Xuan Zhuo
2024-03-18 11:05 ` [PATCH net-next v5 6/9] virtio_net: add the total stats field Xuan Zhuo
2024-03-18 11:06 ` [PATCH net-next v5 7/9] virtio_net: rename stat tx_timeout to timeout Xuan Zhuo
2024-04-10 6:09 ` Jason Wang
2024-03-18 11:06 ` [PATCH net-next v5 8/9] netdev: add queue stats Xuan Zhuo
2024-03-18 11:06 ` [PATCH net-next v5 9/9] virtio-net: support queue stat Xuan Zhuo
2024-03-18 11:52 ` [PATCH net-next v5 0/9] virtio-net: support device stats Jiri Pirko
2024-03-18 11:53 ` Xuan Zhuo
2024-03-18 12:19 ` Jiri Pirko
2024-03-19 10:12 ` Paolo Abeni
2024-03-20 8:04 ` Xuan Zhuo
2024-03-20 12:23 ` Jiri Pirko
2024-03-21 3:38 ` Jakub Kicinski
2024-03-21 3:54 ` Xuan Zhuo
2024-03-21 12:42 ` Simon Horman
2024-03-20 9:45 ` Xuan Zhuo
2024-04-22 20:33 ` Michael S. Tsirkin
2024-04-23 5:54 ` Xuan Zhuo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACGkMEucuS0yGP1d9w3KEUigGusP_ks71KwZAwuBJNdZQ=v13Q@mail.gmail.com' \
--to=jasowang@redhat.com \
--cc=amritha.nambiar@intel.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hawk@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=larysa.zaremba@intel.com \
--cc=maciej.fijalkowski@intel.com \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sdf@google.com \
--cc=sridhar.samudrala@intel.com \
--cc=virtualization@lists.linux.dev \
--cc=xuanzhuo@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).