On Mon, Feb 28, 2022 at 08:17:49PM +0800, Xuan Zhuo wrote:
> On Mon, 28 Feb 2022 06:40:33 -0500, "Michael S. Tsirkin" <[email protected]>
> wrote:
> > On Tue, Feb 15, 2022 at 03:47:43PM +0800, Xuan Zhuo wrote:
> > > 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.
> > >
> > > Signed-off-by: Xuan Zhuo <[email protected]>
> > > ---
> > > conformance.tex | 2 +
> > > content.tex | 135 ++++++++++++++++++++++++++++++++++++++++++++++--
> > > 2 files changed, 134 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/conformance.tex b/conformance.tex
> > > index 42f8537..950924e 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -142,6 +142,7 @@ \section{Conformance Targets}\label{sec:Conformance /
> > > Conformance Targets}
> > > \item \ref{drivernormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Automatic receive steering in multiqueue
> > > mode}
> > > \item \ref{drivernormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Offloads State Configuration / Setting
> > > Offloads State}
> > > \item \ref{drivernormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Receive-side scaling (RSS) }
> > > +\item \ref{drivernormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Device stats}
> > > \end{itemize}
> > >
> > > \conformance{\subsection}{Block Driver
> > > Conformance}\label{sec:Conformance / Driver Conformance / Block Driver
> > > Conformance}
> > > @@ -401,6 +402,7 @@ \section{Conformance Targets}\label{sec:Conformance /
> > > Conformance Targets}
> > > \item \ref{devicenormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Gratuitous Packet Sending}
> > > \item \ref{devicenormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Automatic receive steering in multiqueue
> > > mode}
> > > \item \ref{devicenormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS
> > > processing}
> > > +\item \ref{devicenormative:Device Types / Network Device / Device
> > > Operation / Control Virtqueue / Device stats}
> > > \end{itemize}
> > >
> > > \conformance{\subsection}{Block Device
> > > Conformance}\label{sec:Conformance / Device Conformance / Block Device
> > > Conformance}
> > > diff --git a/content.tex b/content.tex
> > > index c6f116c..818275d 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -3092,6 +3092,9 @@ \subsection{Feature bits}\label{sec:Device Types /
> > > Network Device / Feature bits
> > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > > channel.
> > >
> > > +\item[VIRTIO_NET_F_CTRL_STATS(55)] Device can provide device-level
> > > statistics
> > > + to the driver through the control channel.
> > > +
> > > \item[VIRTIO_NET_F_HOST_USO (56)] Device can receive USO packets. Unlike
> > > UFO
> > > (fragmenting the packet) the USO splits large UDP packet
> > > to several segments when each of these smaller packets has UDP header.
> > > @@ -3137,6 +3140,7 @@ \subsubsection{Feature bit
> > > requirements}\label{sec:Device Types / Network Device
> > > \item[VIRTIO_NET_F_GUEST_ANNOUNCE] Requires VIRTIO_NET_F_CTRL_VQ.
> > > \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
> > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
> > > +\item[VIRTIO_NET_F_CTRL_STATS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or
> > > VIRTIO_NET_F_HOST_TSO6.
> > > \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > \end{description}
> > > @@ -4015,6 +4019,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > > Types / Network Device / Devi
> > > u8 command;
> > > u8 command-specific-data[];
> > > u8 ack;
> > > + u8 command-specific-data-reply[];
> > > };
> > >
> > > /* ack values */
> > > @@ -4023,9 +4028,13 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > > Types / Network Device / Devi
> > > \end{lstlisting}
> > >
> > > The \field{class}, \field{command} and command-specific-data are set by
> > > the
> > > -driver, and the device sets the \field{ack} byte. There is little it can
> > > -do except issue a diagnostic if \field{ack} is not
> > > -VIRTIO_NET_OK.
> > > +driver, and the device sets the \field{ack} byte and optionally
> > > +\field{command-specific-data-reply}. There is little the driver can
> > > +do except issue a diagnostic if \field{ack} is not VIRTIO_NET_OK.
> > > +
> > > +These commands VIRTIO_NET_CTRL_STATS_GET_DEV,
> > > +VIRTIO_NET_CTRL_STATS_GET_CTRL_VQ, and
> > > VIRTIO_NET_CTRL_STATS_GET_QUEUE_PAIR contain
> > > +\field{command-specific-data-reply}.
> > >
> > > \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network
> > > Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
> > > \label{sec:Device Types / Network Device / Device Operation / Control
> > > Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
> > > @@ -4471,6 +4480,126 @@ \subsubsection{Control
> > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > according to the native endian of the guest rather than
> > > (necessarily when not using the legacy interface) little-endian.
> > >
> > > +\paragraph{Device stats}\label{sec:Device Types / Network Device /
> > > Device Operation / Control Virtqueue / Device stats}
> > > +
> > > +If the VIRTIO_NET_F_CTRL_STATS feature is negotiated, the driver can
> > > +get device stats from the device in \field{command-specific-data-reply}.
> > > +
> > > +To get the stats, the following definitions are used:
> > > +\begin{lstlisting}
> > > +#define VIRTIO_NET_CTRL_STATS 6
> > > +#define VIRTIO_NET_CTRL_STATS_GET_DEV 0
> > > +#define VIRTIO_NET_CTRL_STATS_GET_CTRL_VQ 1
> > > +#define VIRTIO_NET_CTRL_STATS_GET_QUEUE_PAIR 2
> > > +\end{lstlisting}
> > > +
> > > +The following layout structures are used:
> > > +
> > > +\field{command-specific-data}
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_stats_queue_pair {
> > > + le64 queue_pair_index;
> > > +}
> > > +\end{lstlisting}
> > > +
> > > +\field{command-specific-data-reply}
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_reply_stats_dev {
> > > + le64 mac_unicast_num; /* The number of unicast mac addresses. */
> > > + le64 mac_multicast_num; /* The number of multicast mac addresses. */
> > > + le64 vlan_num; /* The number of vlan. */
> > > +}
> > > +
> > > +struct virtio_net_ctrl_reply_stats_cvq {
> > > + le64 command_num; /* The number of commands, including
> > > the current command. */
> > > + le64 ok_num; /* The number of commands (including
> > > the current command) where the ack was VIRTIO_NET_OK. */
> > > +}
> > > +
> > > +struct virtio_net_ctrl_reply_stats_queue_pair {
> > > + /* rx stats */
> > > + le64 rx_packets; /* The number of packets received by
> > > device (not the packets passed to the guest), including the dropped
> > > packets by device. */
> > > + le64 rx_bytes; /* The number of bytes received by device
> > > (not the packets passed to the guest), including the dropped packets by
> > > device. */
> > > +
> > > + le64 rx_notification; /* The number of notifications from
> > > driver. */
> > > + le64 rx_interrupt; /* The number of interrupts generated by
> > > device. */
> >
> > maybe device notifications/driver notifications?
>
>
> OK.
>
> >
> > > +
> > > + le64 rx_drop; /* The number of packets dropped by the rx
> > > queue. Contains all kinds of packet drop. */
> > > + le64 rx_drop_overruns; /* The number of packets dropped by the rx
> > > queue when no more descriptors were available. */
> > > +
> > > + le64 rx_csum_valid; /* The number of packets with
> > > VIRTIO_NET_HDR_F_DATA_VALID. */
> > > + le64 rx_csum_partial; /* The number of packets with
> > > VIRTIO_NET_HDR_F_NEEDS_CSUM. */
> >
> > So why not rx_needs_csum?
>
> The name comes from other manufacturers.
It's from linux, but consistency comes first. Either rename the flag or
the field.
> >
> > > + le64 rx_csum_bad; /* The number of packets with abnormal
> > > csum. */
> > > + le64 rx_csum_none; /* The number of packets without hardware
> > > csum. */
> >
> > these are only counted when rx checksum enabled and bad are then dropped
>
> When rx checksum is enabled, some packages (such as user-defined, no tcp/udp)
> the device cannot checksum.
all of this needs to be in the spec.
>
> >
> > > +
> > > + le64 rx_gso_packets; /* The number of GSO packets written into
> > > the RX VQ buffers. */
> >
> > still unclear what does this count exactly, and when.
>
> I think jason's opinion is right, I shouldn't use comments to explain. Some
> complex cases can be better explained using description/item.
>
> This is the case in many cases below.
>
> >
> > > + le64 rx_gso_bytes; /* The number of bytes(excluding the
> > > virtio net header) of the GSO packets written into the RX VQ buffers. */
> > > + le64 rx_reset; /* The number of queue resets. */
> > > +
> >
> > I'm not sure how we'll extend these, given there's no space between RX
> > and TX. Is there value in correlating rx and tx stats in a single
> > command, given they are on separate VQs?
> >
> > > + /* tx stats */
> > > + le64 tx_packets; /* The number of packets sent by device
> > > (not the packets received from the guest), excluding the dropped packets
> > > by device. */
> > > + le64 tx_bytes; /* The number of bytes sent by device (not
> > > the packets received from the guest), excluding the dropped packets by
> > > device. */
> > > +
> > > + le64 tx_notification; /* The number of notifications from
> > > driver. */
> > > + le64 tx_interrupt; /* The number of interrupts generated by
> > > device. */
> > > +
> > > + le64 tx_drop; /* The number of packets dropped by the tx
> > > queue. Contains all kinds of packet drop. */
> > > + le64 tx_drop_malformed; /* The number of packets dropped when the
> > > descriptor is in an error state. */
> >
> > Seems to imply some kind of error handling. A bit vague, I think we
> > discussed this.
> >
> > > +
> > > + le64 tx_csum_none; /* The number of packets that doesn't
> > > require hardware csum. */
> >
> > that didn't require?
> >
> > > + le64 tx_csum_partial; /* The number of packets that requires
> > > hardware csum. */
> >
> > required?
> >
> >
> >
> > > +
> > > + le64 tx_gso_packets; /* The number of GSO packets that the
> > > device received from the driver. */
> > > + le64 tx_gso_bytes; /* The number of bytes(excluding the
> > > virtio net header) of GSO packets that the device received from the
> > > driver. */
> >
> > all of the above should be more explicit about which packet fields and which
> > values are meant.
> >
> > > + le64 tx_reset; /* The number of queue resets. */
> >
> > of the tx queue specifically? depends on the relevant flag?
> >
> > > +}
> > > +\end{lstlisting}
> > > +
> > > +See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues /
> > > Virtqueue Reset}
> > > +for more about \field{rx_reset} and \field{tx_reset}.
> > > +
> > > +All device stats are divided into three categories:
> > > +\begin{itemize}
> > > + \item the stats of the device.
> > > + \begin{description}
> > > + \item[command] VIRTIO_NET_CTRL_STATS_GET_DEV
> > > + \item[command-specific-data-reply]
> > > virtio_net_ctrl_reply_stats_dev
> > > + \end{description}
> >
> > usefulness of this one is unclear.
>
> I want to remove dev related statistics in the next version.
>
> >
> > > +
> > > + \item the stats of the controlq.
> > > + \begin{description}
> > > + \item[command] VIRTIO_NET_CTRL_STATS_GET_CTRL_VQ
> > > + \item[command-specific-data-reply]
> > > virtio_net_ctrl_reply_stats_cvq
> > > + \end{description}
> > > +
> > > + \item the stats of the queue pair.
> >
> > of a specific queue pair
> >
> > > This contains the stats of rx and tx.
> > > + \begin{description}
> > > + \item[command] VIRTIO_NET_CTRL_STATS_GET_QUEUE_PAIR
> > > + \item[command-specific-data] virtio_net_ctrl_stats_queue_pair
> > > + \item[command-specific-data-reply]
> > > virtio_net_ctrl_reply_stats_queue_pair
> > > + \end{description}
> > > +\end{itemize}
> > > +
> > > +
> > > +\devicenormative{\subparagraph}{Device stats}{Device Types / Network
> > > Device / Device Operation / Control Virtqueue / Device stats}
> > > +
> > > +If VIRTIO_NET_F_CTRL_STATS has been negotiated, the device MUST support
> > > the
> > > +VIRTIO_NET_CTRL_STATS_GET_DEV, VIRTIO_NET_CTRL_STATS_GET_CTRL_VQ and
> > > +VIRTIO_NET_CTRL_STATS_GET_QUEUE_PAIR commands.
> >
> > why a single flag then?
>
>
> Sorry, I didn't understand. ^_^
why do we have a single flag to guard availability of multiple
commands?
>
> >
> > > +
> > > +If \field{queue_pair_index} is out of range for a
> > > +VIRTIO_NET_CTRL_STATS_GET_QUEUE_PAIR command, the device MUST set
> > > \field{ack} in
> > > +virtio_net_ctrl to VIRTIO_NET_ERR.
> > > +
> > > +If VIRTIO_NET_F_GUEST_CSUM is not successfully negotiated, all csum
> > > related
> > > +metrics MUST be 0.
> >
> > why would tx counters be affected by this, as opposed to
> > VIRTIO_NET_F_CSUM? "csum related" means what?
> >
> > > +
> > > +GSO packet refers to the packet of gso_type != VIRTIO_NET_HDR_GSO_NONE.
> >
> > Let's write english not pseudo code by preference.
> >
> > GSO packets refers to the number of packets with \field{gso_type} other
> > than VIRTIO_NET_HDR_GSO_NONE
> >
> > Also this kind of explanation belongs near where the term is used, not in a
> > separate paragraph.
> >
> > > +All statistics on GSO MUST be based on the GSO packets.
> >
> > The meaning of this is not very clear.
> >
> > > +
> > > +\drivernormative{\subparagraph}{Device stats}{Device Types / Network
> > > Device / Device Operation / Control Virtqueue / Device stats}
> > > +
> > > +\field{command-specific-data} MUST be empty for
> > > VIRTIO_NET_CTRL_STATS_GET_DEV
> > > +and VIRTIO_NET_CTRL_STATS_GET_CTRL_VQ.
> >
> > we need to give some thought to how we will extend these things.
> > how about we say that buffers can have extra space and it should
> > be ignored by device and driver respectively?
>
>
> As replied in another email, my previous discussion with jason was to add new
> commands or features to implement extensions.
>
> Thanks very much.
>
Then the numbers aren't queried atomically though. Might be problematic.
> >
> > > \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> > > Types / Network Device / Legacy Interface: Framing Requirements}
> > > --
> > > 2.31.0
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [email protected]
> > For additional commands, e-mail: [email protected]
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
