Messages in this thread | | | From | Eugenio Perez Martin <> | Date | Wed, 21 Dec 2022 07:33:26 +0100 | Subject | Re: [PATCH 4/4] vdpa_sim_net: vendor satistics |
| |
On Wed, Dec 21, 2022 at 7:17 AM Jason Wang <jasowang@redhat.com> wrote: > > This patch adds support for basic vendor stats that include counters > for tx, rx and cvq. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 + > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 215 ++++++++++++++++++++++++++- > 2 files changed, 211 insertions(+), 6 deletions(-) > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > index 02e892f819e7..595d9d5a372f 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > @@ -755,8 +755,10 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = { > .set_vq_cb = vdpasim_set_vq_cb, > .set_vq_ready = vdpasim_set_vq_ready, > .get_vq_ready = vdpasim_get_vq_ready, > + .get_vendor_vq_stats = vdpasim_get_vq_stats, > .set_vq_state = vdpasim_set_vq_state, > .get_vq_state = vdpasim_get_vq_state, > + .get_vendor_vq_stats = vdpasim_get_vq_stats,
The patch adds .get_vendor_vq_stats twice to vdpasim_batch_config_ops assignment.
> .get_vq_align = vdpasim_get_vq_align, > .get_vq_group = vdpasim_get_vq_group, > .get_device_features = vdpasim_get_device_features, > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > index 20cd5cdff919..3c05e932d90d 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > @@ -15,6 +15,7 @@ > #include <linux/etherdevice.h> > #include <linux/vringh.h> > #include <linux/vdpa.h> > +#include <net/netlink.h> > #include <uapi/linux/virtio_net.h> > #include <uapi/linux/vdpa.h> > > @@ -36,6 +37,34 @@ > #define VDPASIM_NET_AS_NUM 2 > #define VDPASIM_NET_GROUP_NUM 2 > > +struct vdpasim_dataq_stats { > + struct u64_stats_sync syncp; > + u64 pkts; > + u64 bytes; > + u64 drops; > + u64 errors; > + u64 overruns; > +}; > + > +struct vdpasim_cq_stats { > + struct u64_stats_sync syncp; > + u64 requests; > + u64 successes; > + u64 errors; > +}; > + > +struct vdpasim_net{ > + struct vdpasim vdpasim; > + struct vdpasim_dataq_stats tx_stats; > + struct vdpasim_dataq_stats rx_stats; > + struct vdpasim_cq_stats cq_stats; > +}; > + > +static struct vdpasim_net *sim_to_net(struct vdpasim *vdpasim) > +{ > + return container_of(vdpasim, struct vdpasim_net, vdpasim); > +} > + > static void vdpasim_net_complete(struct vdpasim_virtqueue *vq, size_t len) > { > /* Make sure data is wrote before advancing index */ > @@ -93,9 +122,11 @@ static virtio_net_ctrl_ack vdpasim_handle_ctrl_mac(struct vdpasim *vdpasim, > static void vdpasim_handle_cvq(struct vdpasim *vdpasim) > { > struct vdpasim_virtqueue *cvq = &vdpasim->vqs[2]; > + struct vdpasim_net *net = sim_to_net(vdpasim); > virtio_net_ctrl_ack status = VIRTIO_NET_ERR; > struct virtio_net_ctrl_hdr ctrl; > size_t read, write; > + u64 requests = 0, errors = 0; > int err; > > if (!(vdpasim->features & (1ULL << VIRTIO_NET_F_CTRL_VQ))) > @@ -113,8 +144,12 @@ static void vdpasim_handle_cvq(struct vdpasim *vdpasim) > > read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->in_iov, &ctrl, > sizeof(ctrl)); > - if (read != sizeof(ctrl)) > + if (read != sizeof(ctrl)) { > + ++errors; > break; > + } > + > + ++requests; > > switch (ctrl.class) { > case VIRTIO_NET_CTRL_MAC: > @@ -141,6 +176,12 @@ static void vdpasim_handle_cvq(struct vdpasim *vdpasim) > cvq->cb(cvq->private); > local_bh_enable(); > } > + > + u64_stats_update_begin(&net->cq_stats.syncp); > + net->cq_stats.requests += requests; > + net->cq_stats.errors += errors; > + net->cq_stats.successes += requests;
I'd say we should maintain here requests == errors + requests.
Intuitively, I'd rename local variable requests to successes, and I'd apply: net->cq_stats.requests += successes + errors; net->cq_stats.errors += errors; net->cq_stats.successes += successes;
Or am I missing something?
Thanks!
> + u64_stats_update_end(&net->cq_stats.syncp); > } > > static void vdpasim_net_work(struct work_struct *work) > @@ -148,8 +189,10 @@ static void vdpasim_net_work(struct work_struct *work) > struct vdpasim *vdpasim = container_of(work, struct vdpasim, work); > struct vdpasim_virtqueue *txq = &vdpasim->vqs[1]; > struct vdpasim_virtqueue *rxq = &vdpasim->vqs[0]; > + struct vdpasim_net *net = sim_to_net(vdpasim); > ssize_t read, write; > - int pkts = 0; > + u64 tx_pkts = 0, rx_pkts = 0, tx_bytes = 0, rx_bytes = 0; > + u64 rx_drops = 0, rx_overruns = 0, rx_errors = 0, tx_errors = 0; > int err; > > spin_lock(&vdpasim->lock); > @@ -168,14 +211,21 @@ static void vdpasim_net_work(struct work_struct *work) > while (true) { > err = vringh_getdesc_iotlb(&txq->vring, &txq->out_iov, NULL, > &txq->head, GFP_ATOMIC); > - if (err <= 0) > + if (err <= 0) { > + if (err) > + ++tx_errors; > break; > + } > > + ++tx_pkts; > read = vringh_iov_pull_iotlb(&txq->vring, &txq->out_iov, > vdpasim->buffer, > PAGE_SIZE); > > + tx_bytes += read; > + > if (!receive_filter(vdpasim, read)) { > + ++rx_drops; > vdpasim_net_complete(txq, 0); > continue; > } > @@ -183,19 +233,25 @@ static void vdpasim_net_work(struct work_struct *work) > err = vringh_getdesc_iotlb(&rxq->vring, NULL, &rxq->in_iov, > &rxq->head, GFP_ATOMIC); > if (err <= 0) { > + ++rx_overruns; > vdpasim_net_complete(txq, 0); > break; > } > > write = vringh_iov_push_iotlb(&rxq->vring, &rxq->in_iov, > vdpasim->buffer, read); > - if (write <= 0) > + if (write <= 0) { > + ++rx_errors; > break; > + } > + > + ++rx_pkts; > + rx_bytes += write; > > vdpasim_net_complete(txq, 0); > vdpasim_net_complete(rxq, write); > > - if (++pkts > 4) { > + if (tx_pkts > 4) { > schedule_work(&vdpasim->work); > goto out; > } > @@ -203,6 +259,145 @@ static void vdpasim_net_work(struct work_struct *work) > > out: > spin_unlock(&vdpasim->lock); > + > + u64_stats_update_begin(&net->tx_stats.syncp); > + net->tx_stats.pkts += tx_pkts; > + net->tx_stats.bytes += tx_bytes; > + net->tx_stats.errors += tx_errors; > + u64_stats_update_end(&net->tx_stats.syncp); > + > + u64_stats_update_begin(&net->rx_stats.syncp); > + net->rx_stats.pkts += rx_pkts; > + net->rx_stats.bytes += rx_bytes; > + net->rx_stats.drops += rx_drops; > + net->rx_stats.errors += rx_errors; > + net->rx_stats.overruns += rx_overruns; > + u64_stats_update_end(&net->rx_stats.syncp); > +} > + > +static int vdpasim_net_get_stats(struct vdpasim *vdpasim, u16 idx, > + struct sk_buff *msg, > + struct netlink_ext_ack *extack) > +{ > + struct vdpasim_net *net = sim_to_net(vdpasim); > + u64 rx_pkts, rx_bytes, rx_errors, rx_overruns, rx_drops; > + u64 tx_pkts, tx_bytes, tx_errors, tx_drops; > + u64 cq_requests, cq_successes, cq_errors; > + unsigned int start; > + int err = -EMSGSIZE; > + > + switch(idx) { > + case 0: > + do { > + start = u64_stats_fetch_begin(&net->rx_stats.syncp); > + rx_pkts = net->rx_stats.pkts; > + rx_bytes = net->rx_stats.bytes; > + rx_errors = net->rx_stats.errors; > + rx_overruns = net->rx_stats.overruns; > + rx_drops = net->rx_stats.drops; > + } while (u64_stats_fetch_retry(&net->rx_stats.syncp, start)); > + > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "rx packets")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + rx_pkts, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "rx bytes")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + rx_bytes, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "rx errors")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + rx_errors, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "rx overrunss")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + rx_overruns, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "rx drops")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + rx_drops, VDPA_ATTR_PAD)) > + break; > + err = 0; > + break; > + case 1: > + do { > + start = u64_stats_fetch_begin(&net->tx_stats.syncp); > + tx_pkts = net->tx_stats.pkts; > + tx_bytes = net->tx_stats.bytes; > + tx_errors = net->tx_stats.errors; > + tx_drops = net->tx_stats.drops; > + } while (u64_stats_fetch_retry(&net->tx_stats.syncp, start)); > + > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "tx packets")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + tx_pkts, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "tx bytes")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + tx_bytes, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "tx errors")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + tx_errors, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "tx drops")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + tx_drops, VDPA_ATTR_PAD)) > + break; > + err = 0; > + break; > + case 2: > + do { > + start = u64_stats_fetch_begin(&net->cq_stats.syncp); > + cq_requests = net->cq_stats.requests; > + cq_successes = net->cq_stats.successes; > + cq_errors = net->cq_stats.errors; > + } while (u64_stats_fetch_retry(&net->cq_stats.syncp, start)); > + > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "cvq requests")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + cq_requests, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "cvq successes")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + cq_successes, VDPA_ATTR_PAD)) > + break; > + if (nla_put_string(msg, VDPA_ATTR_DEV_VENDOR_ATTR_NAME, > + "cvq errors")) > + break; > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, > + cq_errors, VDPA_ATTR_PAD)) > + break; > + err = 0; > + break; > + default: > + err = -EINVAL; > + break; > + } > + > + return err; > } > > static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config) > @@ -239,6 +434,7 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > const struct vdpa_dev_set_config *config) > { > struct vdpasim_dev_attr dev_attr = {}; > + struct vdpasim_net *net; > struct vdpasim *simdev; > int ret; > > @@ -249,10 +445,11 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > dev_attr.nvqs = VDPASIM_NET_VQ_NUM; > dev_attr.ngroups = VDPASIM_NET_GROUP_NUM; > dev_attr.nas = VDPASIM_NET_AS_NUM; > - dev_attr.alloc_size = sizeof(struct vdpasim); > + dev_attr.alloc_size = sizeof(struct vdpasim_net); > dev_attr.config_size = sizeof(struct virtio_net_config); > dev_attr.get_config = vdpasim_net_get_config; > dev_attr.work_fn = vdpasim_net_work; > + dev_attr.get_stats = vdpasim_net_get_stats; > dev_attr.buffer_size = PAGE_SIZE; > > simdev = vdpasim_create(&dev_attr, config); > @@ -265,6 +462,12 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > if (ret) > goto reg_err; > > + net = sim_to_net(simdev); > + > + u64_stats_init(&net->tx_stats.syncp); > + u64_stats_init(&net->rx_stats.syncp); > + u64_stats_init(&net->cq_stats.syncp); > + > return 0; > > reg_err: > -- > 2.25.1 >
| |