Re: [virtio-dev] Re: [PATCH v11] virtio-net: support device stats

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Tue, 8 Mar 2022 14:36:02 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Mon, Mar 7, 2022 at 5:07 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Mon, 7 Mar 2022 15:58:03 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > å 2022/3/2 äå4:52, Xuan Zhuo åé:
> > > > This patch allows the driver to obtain some statistics from the device.
> > > >
> > > > In the back-end implementation, we can count a lot of such information,
> > > > which can be used for debugging and judging the running status of the
> > > > back-end. We hope to directly display it to the user through ethtool.
> > > >
> > > > To get stats atomically, try to get stats for all queue pairs in one
> > > > command.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > v11:
> > > > 1. Use michael's advice to introduce virtio_net_ctrl_queue_stats to get vq stats
> > > >     based on vq num and type
> > > > 2. split stats structure
>
> [...]
>
> > > What happens if driver tries to query RX stats through a TX index?
> >
> > Device setting ack to VIRTIO_NET_ERR.
> >
> > I relegated this case to the next case. "The type of vq does not match \field{type}."
> >
> > I will explain in the next release:
> >
> >   \item The type of vq does not match \field{type}. Such as the driver tries to query RX stats through a TX index.
> >
> > >
> > >
> > > > +    \item The type of vq does not match \field{type}.
> > > > +    \item The feature corresponding to the specified \field{type} was not successfully
> > > > +        negotiated.
> > > > +    \item The size of \field{command-specific-data-reply} is less than the sum
> > > > +        of \field{length}.
> > >
> > >
> > > I'm not sure I get here, I guess this proposal allows the driver to
> > > query an array of stats. So I guess it means the size of required stats
> > > instead of the size of \field{command-specific-data-reply}.
> >
> > The size of \field{command-specific-data-reply} refers to the size of memory
> > allocated by the driver for \field{command-specific-data-reply}.
> >
> > Sorry, I will make it clearer in the next version.
> >
> > >
> > >
> > > > +\end{itemize}
> > > > +
> > > > +The device MUST write the requested stats structures in
> > > > +\field{command-specific-data-reply} in the order specified by the structure
> > > > +virtio_net_ctrl_queue_stats.
> > >
> > >
> > > Do we need per stat error code here? Or device can simply fail a batch
> > > of query if one of those fails?
> >
> > Device can simply fail a batch of query if one of those fails.
>
> We need clarify, and I guess we should use MUST instead of MAY without
> a per stat error value.
>
> >
> > >
> > >
> > > > +
> > > > +The size of each stats MUST be less than or equal to the corresponding
> > > > +\field{length}, but the space it occupies MUST be equal to the corresponding
> > > > +\field{length}.
> > >
> > >
> > > I wonder how the length trick can work here. Is this used for extension?
> > > If yes, how can driver know a suitable length?
> >
> > It only allows the possibility of expansion.
> >
> > The driver must set the length based on the size of the stats it knows.
>
> Ok, this seems like a way to make a new driver that can run on an old device.
>
> But then I wonder if it would be simpler to just use fixed length and
> just extend the types.

Yes, we can require that the length value must be the size of stats, which would
be simpler. Extend the new stats types in the future with the new types.

I think it's ok.

Thanks.

>
> Thanks
>
> > It seems that I am not very clear on this point. I will make this clear in the
> > next version.
> >
> > Of course, it is allowed to set a larger length. As long as the driver allocates
> > the corresponding memory for it.
> >
> >
> > > What happens if device support more stats?
> >
> > "The size of each stats MUST be less than or equal to the corresponding \field{length}."
> >
> > >
> > >
> > > > +
> > > > +\drivernormative{\subparagraph}{Device Stats}{Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats}
> > > > +
> > > > +When a driver tries to obtain a certain stats, it MUST confirm that the relevant
> > > > +feature negotiation is successful.
> > > > +
> > > > +\field{type} in struct virtio_net_ctrl_queue_stats MUST correspond to the vq
> > > > +specified by \field{queue_num}.
> > > > +
> > > > +\field{length} in struct virtio_net_ctrl_queue_stats MUST be greater than or
> > > > +equal to the size of the structure corresponding to \field{type}. It MUST be a
> > > > +multiple of 8.
> > >
> > >
> > > Why do we need the padding here?
> >
> > This is to ensure structure alignment.
> >
> > >
> > >
> > > > +
> > > > +The size of \field{command-specific-data-reply} allocated by the driver MUST be
> > > > +greater than or equal to the sum of \field{legnth} in struct
> > >
> > >
> > > typo.
> >
> > I will fix it.
> >
> > >
> > >
> > > > +virtio_net_ctrl_queue_stats.
> > >
> > >
> > > Any value that we can allocate more than the sum of the length?
> >
> > It is theoretically allowed, as long as the allocated space is greater than the
> > sum of length, there will be no error.
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > >
> > > > +
> > > > +When the driver reads the response, it MUST read
> > > > +\field{command-specific-data-reply} one by one based on the set \field{length}
> > > > +and \field{type}.
> > > >
> > > >   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> > > >   Types / Network Device / Legacy Interface: Framing Requirements}
> > > > --
> > > > 2.31.0
> > > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
>