* [virtio-dev] [PATCH v9] virtio-net: support device stats
@ 2022-02-15 7:47 Xuan Zhuo
2022-02-24 11:09 ` Xuan Zhuo
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Xuan Zhuo @ 2022-02-15 7:47 UTC (permalink / raw)
To: virtio-dev; +Cc: jasowang, Michael S. Tsirkin
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 <xuanzhuo@linux.alibaba.com>
---
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. */
+
+ 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. */
+ le64 rx_csum_bad; /* The number of packets with abnormal csum. */
+ le64 rx_csum_none; /* The number of packets without hardware csum. */
+
+ le64 rx_gso_packets; /* The number of GSO packets written into the RX VQ buffers. */
+ 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. */
+
+ /* 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. */
+
+ le64 tx_csum_none; /* The number of packets that doesn't require hardware csum. */
+ le64 tx_csum_partial; /* The number of packets that requires hardware csum. */
+
+ 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. */
+ le64 tx_reset; /* The number of queue resets. */
+}
+\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}
+
+ \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. 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.
+
+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.
+
+GSO packet refers to the packet of gso_type != VIRTIO_NET_HDR_GSO_NONE.
+All statistics on GSO MUST be based on the GSO packets.
+
+\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.
\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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [virtio-dev] [PATCH v9] virtio-net: support device stats
2022-02-15 7:47 [virtio-dev] [PATCH v9] virtio-net: support device stats Xuan Zhuo
@ 2022-02-24 11:09 ` Xuan Zhuo
2022-02-28 9:07 ` [virtio-dev] " Jason Wang
2022-02-28 11:40 ` Michael S. Tsirkin
2 siblings, 0 replies; 9+ messages in thread
From: Xuan Zhuo @ 2022-02-24 11:09 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: jasowang, Michael S. Tsirkin, virtio-dev
On Tue, 15 Feb 2022 15:47:43 +0800, Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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 <xuanzhuo@linux.alibaba.com>
hi Michael, Jason, any comment for this?
Thanks.
> ---
> 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. */
> +
> + 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. */
> + le64 rx_csum_bad; /* The number of packets with abnormal csum. */
> + le64 rx_csum_none; /* The number of packets without hardware csum. */
> +
> + le64 rx_gso_packets; /* The number of GSO packets written into the RX VQ buffers. */
> + 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. */
> +
> + /* 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. */
> +
> + le64 tx_csum_none; /* The number of packets that doesn't require hardware csum. */
> + le64 tx_csum_partial; /* The number of packets that requires hardware csum. */
> +
> + 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. */
> + le64 tx_reset; /* The number of queue resets. */
> +}
> +\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}
> +
> + \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. 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.
> +
> +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.
> +
> +GSO packet refers to the packet of gso_type != VIRTIO_NET_HDR_GSO_NONE.
> +All statistics on GSO MUST be based on the GSO packets.
> +
> +\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.
>
> \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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* [virtio-dev] Re: [PATCH v9] virtio-net: support device stats
2022-02-15 7:47 [virtio-dev] [PATCH v9] virtio-net: support device stats Xuan Zhuo
2022-02-24 11:09 ` Xuan Zhuo
@ 2022-02-28 9:07 ` Jason Wang
2022-02-28 11:21 ` Xuan Zhuo
2022-02-28 11:40 ` Michael S. Tsirkin
2 siblings, 1 reply; 9+ messages in thread
From: Jason Wang @ 2022-02-28 9:07 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Virtio-Dev, Michael S. Tsirkin
On Tue, Feb 15, 2022 at 3:47 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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 <xuanzhuo@linux.alibaba.com>
> ---
> 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. */
> +}
This looks more like the states instead of the stats? And the driver
should have the knowledge of those. Or is there any chance that the
device knowledge can differ from the driver's?
> +
> +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. */
> +
> + 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. */
> + le64 rx_csum_bad; /* The number of packets with abnormal csum. */
> + le64 rx_csum_none; /* The number of packets without hardware csum. */
> +
> + le64 rx_gso_packets; /* The number of GSO packets written into the RX VQ buffers. */
> + 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. */
> +
> + /* 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. */
> +
> + le64 tx_csum_none; /* The number of packets that doesn't require hardware csum. */
> + le64 tx_csum_partial; /* The number of packets that requires hardware csum. */
s/requires/require/
> +
> + 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. */
> + le64 tx_reset; /* The number of queue resets. */
> +}
> +\end{lstlisting}
I wonder if it's better to use description/item to explain the fields
one by one instead of using comments.
E.g for some fields, we need to explain that the value is valid only
if some specific feature is negotiated (e.g gso or reset)?
Rethink of the structure, I also wonder if it's better to restructure
the stats to
struct stats{
/* the counters that doesn't need any features */
/* the counters that needs CSUM */
/* the counters that needs GSO */
/* the counters that needs reset */
}
Or even split them into different commands. The reason is that the
current design requires the knowledge of a feature even if it doesn't
support it.
> +
> +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}
> +
> + \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. 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.
> +
> +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.
It's better to move this to the guidance part.
> +
> +GSO packet refers to the packet of gso_type != VIRTIO_NET_HDR_GSO_NONE.
We'd better use English instead of "!=" here.
> +All statistics on GSO MUST be based on the GSO packets.
As discussed above, we need to also add the part of reset.
Thanks
> +
> +\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.
>
> \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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [virtio-dev] Re: [PATCH v9] virtio-net: support device stats
2022-02-28 9:07 ` [virtio-dev] " Jason Wang
@ 2022-02-28 11:21 ` Xuan Zhuo
2022-02-28 11:55 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Xuan Zhuo @ 2022-02-28 11:21 UTC (permalink / raw)
To: Jason Wang; +Cc: Virtio-Dev, Michael S. Tsirkin
On Mon, 28 Feb 2022 17:07:54 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Feb 15, 2022 at 3:47 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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 <xuanzhuo@linux.alibaba.com>
> > ---
> > 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. */
> > +}
>
> This looks more like the states instead of the stats? And the driver
> should have the knowledge of those. Or is there any chance that the
> device knowledge can differ from the driver's?
I'm really not sure if we want these stats, or if we just leave dev stats empty.
>
> > +
> > +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. */
> > +
> > + 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. */
> > + le64 rx_csum_bad; /* The number of packets with abnormal csum. */
> > + le64 rx_csum_none; /* The number of packets without hardware csum. */
> > +
> > + le64 rx_gso_packets; /* The number of GSO packets written into the RX VQ buffers. */
> > + 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. */
> > +
> > + /* 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. */
> > +
> > + le64 tx_csum_none; /* The number of packets that doesn't require hardware csum. */
> > + le64 tx_csum_partial; /* The number of packets that requires hardware csum. */
>
> s/requires/require/
>
> > +
> > + 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. */
> > + le64 tx_reset; /* The number of queue resets. */
> > +}
> > +\end{lstlisting}
>
> I wonder if it's better to use description/item to explain the fields
> one by one instead of using comments.
>
> E.g for some fields, we need to explain that the value is valid only
> if some specific feature is negotiated (e.g gso or reset)?
>
> Rethink of the structure, I also wonder if it's better to restructure
> the stats to
>
> struct stats{
> /* the counters that doesn't need any features */
> /* the counters that needs CSUM */
> /* the counters that needs GSO */
> /* the counters that needs reset */
> }
>
> Or even split them into different commands. The reason is that the
> current design requires the knowledge of a feature even if it doesn't
> support it.
I think this is a good idea.
>
> > +
> > +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}
> > +
> > + \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. 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.
> > +
> > +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.
>
> It's better to move this to the guidance part.
>
> > +
> > +GSO packet refers to the packet of gso_type != VIRTIO_NET_HDR_GSO_NONE.
>
> We'd better use English instead of "!=" here.
>
> > +All statistics on GSO MUST be based on the GSO packets.
>
> As discussed above, we need to also add the part of reset.
OK.
Thanks.
>
> Thanks
>
> > +
> > +\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.
> >
> > \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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [virtio-dev] Re: [PATCH v9] virtio-net: support device stats
2022-02-15 7:47 [virtio-dev] [PATCH v9] virtio-net: support device stats Xuan Zhuo
2022-02-24 11:09 ` Xuan Zhuo
2022-02-28 9:07 ` [virtio-dev] " Jason Wang
@ 2022-02-28 11:40 ` Michael S. Tsirkin
2022-02-28 12:17 ` Xuan Zhuo
2 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2022-02-28 11:40 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtio-dev, jasowang
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 <xuanzhuo@linux.alibaba.com>
> ---
> 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?
> +
> + 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?
> + 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
> +
> + le64 rx_gso_packets; /* The number of GSO packets written into the RX VQ buffers. */
still unclear what does this count exactly, and when.
> + 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.
> +
> + \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?
> +
> +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?
> \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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [virtio-dev] Re: [PATCH v9] virtio-net: support device stats
2022-02-28 11:21 ` Xuan Zhuo
@ 2022-02-28 11:55 ` Michael S. Tsirkin
2022-02-28 12:10 ` Xuan Zhuo
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2022-02-28 11:55 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Jason Wang, Virtio-Dev
On Mon, Feb 28, 2022 at 07:21:39PM +0800, Xuan Zhuo wrote:
> > struct stats{
> > /* the counters that doesn't need any features */
> > /* the counters that needs CSUM */
> > /* the counters that needs GSO */
> > /* the counters that needs reset */
> > }
> >
> > Or even split them into different commands. The reason is that the
> > current design requires the knowledge of a feature even if it doesn't
> > support it.
>
>
> I think this is a good idea.
The issue is this: some stats are only useful if they are correlated.
For example, # of rx interrupts is not useful unless you also know the #
of packets. Similar for GSO things. Is it necessary to correlate TX and
RX events? It might be if driver polls TX ring on an RX interrupt. In
all these cases we need a single command to return multiple stats for
atomicity. From that POV having a single struct is great, however if we
do it like this then it is not clear how are we going to extend it. Do
we just add fields at the end? Then the clean separation between RX and
TX fields is lost. Is it always enough to query a single pair of VQs?
E.g. when querying RSS, might we want to correlate between multiple
queues to see that traffic is balanced?
So maybe a good design will get an array of structures describing things
to query and return an array of structures describing the result.
--
MST
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* [virtio-dev] Re: [PATCH v9] virtio-net: support device stats
2022-02-28 11:55 ` Michael S. Tsirkin
@ 2022-02-28 12:10 ` Xuan Zhuo
0 siblings, 0 replies; 9+ messages in thread
From: Xuan Zhuo @ 2022-02-28 12:10 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, Virtio-Dev
On Mon, 28 Feb 2022 06:55:27 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Feb 28, 2022 at 07:21:39PM +0800, Xuan Zhuo wrote:
> > > struct stats{
> > > /* the counters that doesn't need any features */
> > > /* the counters that needs CSUM */
> > > /* the counters that needs GSO */
> > > /* the counters that needs reset */
> > > }
> > >
> > > Or even split them into different commands. The reason is that the
> > > current design requires the knowledge of a feature even if it doesn't
> > > support it.
> >
> >
> > I think this is a good idea.
>
> The issue is this: some stats are only useful if they are correlated.
> For example, # of rx interrupts is not useful unless you also know the #
> of packets. Similar for GSO things. Is it necessary to correlate TX and
> RX events? It might be if driver polls TX ring on an RX interrupt. In
> all these cases we need a single command to return multiple stats for
> atomicity. From that POV having a single struct is great, however if we
> do it like this then it is not clear how are we going to extend it. Do
> we just add fields at the end? Then the clean separation between RX and
> TX fields is lost.
It should still be convenient to add new commands or feature flags.
> Is it always enough to query a single pair of VQs?
> E.g. when querying RSS, might we want to correlate between multiple
> queues to see that traffic is balanced?
>
> So maybe a good design will get an array of structures describing things
> to query and return an array of structures describing the result.
Thanks for your analysis, I think you are right.
Thanks.
>
> --
> MST
>
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support device stats
2022-02-28 11:40 ` Michael S. Tsirkin
@ 2022-02-28 12:17 ` Xuan Zhuo
2022-02-28 13:56 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Xuan Zhuo @ 2022-02-28 12:17 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtio-dev, jasowang
On Mon, 28 Feb 2022 06:40:33 -0500, "Michael S. Tsirkin" <mst@redhat.com> 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 <xuanzhuo@linux.alibaba.com>
> > ---
> > 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.
>
> > + 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.
>
> > +
> > + 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. ^_^
>
> > +
> > +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.
>
> > \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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support device stats
2022-02-28 12:17 ` Xuan Zhuo
@ 2022-02-28 13:56 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2022-02-28 13:56 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtio-dev, jasowang
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" <mst@redhat.com> 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 <xuanzhuo@linux.alibaba.com>
> > > ---
> > > 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: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-02-28 13:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-15 7:47 [virtio-dev] [PATCH v9] virtio-net: support device stats Xuan Zhuo
2022-02-24 11:09 ` Xuan Zhuo
2022-02-28 9:07 ` [virtio-dev] " Jason Wang
2022-02-28 11:21 ` Xuan Zhuo
2022-02-28 11:55 ` Michael S. Tsirkin
2022-02-28 12:10 ` Xuan Zhuo
2022-02-28 11:40 ` Michael S. Tsirkin
2022-02-28 12:17 ` Xuan Zhuo
2022-02-28 13:56 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox