> From: Jason Wang <jasowang@xxxxxxxxxx>
> Sent: Thursday, November 25, 2021 10:21 AM
>
> On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen <elic@xxxxxxxxxx> wrote:
> >
> > Add support for querying virtqueue statistics. Supported statistics are:
> >
> > received_desc - number of descriptors received for the virtqueue
> > completed_desc - number of descriptors completed for the virtqueue
> >
> > A descriptors using indirect buffers is still counted as 1. In
> > addition, N chained descriptors are counted correctly N times as one would
> expect.
> >
> > A new callback was added to vdpa_config_ops which provides the means
> > for the vdpa driver to return statistics results.
> >
> > The interface allows for reading all the supported virtqueues,
> > including the control virtqueue if it exists, by returning the next
> > queue index to query.
> >
> > Examples:
> > 1. Read statisitics for the virtqueue at index 1 $ vdpa dev vstats
> > show vdpa-a qidx 0
> > vdpa-a:
> > qidx 0 rx received_desc 256 completed_desc 9
> >
> > 2. Read statisitics for all the virtqueues $ vdpa dev vstats show
> > vdpa-a
> > vdpa-a:
> > qidx 0 rx received_desc 256 completed_desc 9 qidx 1 tx received_desc
> > 21 completed_desc 21 qidx 2 ctrl received_desc 0 completed_desc 0
> >
> > Signed-off-by: Eli Cohen <elic@xxxxxxxxxx>
> > ---
> > v0 -> v1:
> > Emphasize that we're dealing with vendor specific counters.
> > Terminate query loop on error
> >
> > drivers/vdpa/vdpa.c | 144 ++++++++++++++++++++++++++++++++++++++
> > include/linux/vdpa.h | 10 +++
> > include/uapi/linux/vdpa.h | 9 +++
> > 3 files changed, 163 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> > 7332a74a4b00..da658c80ff2a 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -781,6 +781,90 @@ vdpa_dev_config_fill(struct vdpa_device *vdev,
> struct sk_buff *msg, u32 portid,
> > return err;
> > }
> >
> > +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct
> > +sk_buff *msg, u16 *index) {
> > + struct vdpa_vq_stats stats;
> > + u16 idx = *index;
> > + int err;
> > +
> > + err = vdev->config->get_vq_stats(vdev, index, &stats);
> > + if (err)
> > + return err;
>
> I wonder what happens if some other vendor adds their specific stats.
> Can we then have very large stats but only few of them is necessary for a
> specific vendor? If this is ture, is this better to simply pass the msg to the
> parent instead of a structure like stats?
>
It is better to have vdpa defined vendor stats structure, so that this subsystem has well defined statistics.
If vdpa enables every vendor driver to put the messages of its choice stats will become a assorted items bag.
While it may seem great for one vendor to put whatever they want there,
it is inconvenient for end user to understand/parse those stats differently, if they are not defined by vdpa subsystem.
Additionally integrating it to monitoring tools such as prometheous becomes another pain point.
It also requires moving VDPA_ATTR_DEV_VENDOR_COMPLETED_DESC out of UAPI and defining vendor defined string which can easily go out of sync.
So I am inclined towards vendor specific stats to be well defined by vdpa subsystem like how its down in this patch.
This still allows different stats among multiple vendors or multiple generation of single vendor.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
