From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
To: David Edmondson <david.edmondson@oracle.com>
Cc: virtio-dev@lists.oasis-open.org, jasowang@redhat.com,
"Michael S. Tsirkin" <mst@redhat.com>,
Parav Pandit <parav@nvidia.com>,
virtio-comment@lists.oasis-open.org
Subject: [virtio-dev] Re: [PATCH v14] virtio-net: support device stats
Date: Mon, 7 Aug 2023 19:00:58 +0800 [thread overview]
Message-ID: <1691406058.5846093-1-xuanzhuo@linux.alibaba.com> (raw)
In-Reply-To: <m2h6pbnusm.fsf@oracle.com>
On Mon, 07 Aug 2023 09:03:37 +0100, David Edmondson <david.edmondson@oracle.com> wrote:
> On Monday, 2023-07-31 at 15:27:22 +08, Xuan Zhuo wrote:
> > This patch allows the driver to obtain some statistics from the device.
> >
> > In the device implementation, we can count a lot of such information,
> > which can be used for debugging and judging the running status of the
> > device. We hope to directly display it to the user through ethtool.
> >
> > To get stats atomically, try to get stats for all queue pairs in one
> > command.
> >
> > If the feature is negotiated, the device must support all the stats
> > listed in this commit. If we want add new stats in future, one new
> > feature should be introduced.
>
> Did you consider a query mechanism to avoid the need for a proliferation
> of feature bits?
Please ignore this.
There is a mail with 'RESET', that is the real v14.
Yes. The 'real v14' supports the suppored_stats. We can support migration by
that. Then we will not need new feature for the new stats.
I will fix this log.
I am ok for other comments. Will fix in next version.
Thanks
>
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > v14:
> > * introduce supported_stats to config space
> > * add header(vq_index, size, type) to each reply stats
> > * add ref to the tx GSO
> >
> > device-types/net/description.tex | 365 +++++++++++++++++++++++-
> > device-types/net/device-conformance.tex | 1 +
> > device-types/net/driver-conformance.tex | 1 +
> > 3 files changed, 364 insertions(+), 3 deletions(-)
> >
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index 76585b0..fd7160a 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -88,6 +88,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_DEVICE_STATS(50)] Device can provide device-level statistics
> > + to the driver through the control channel.
> > +
> > \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> >
> > \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > @@ -1156,6 +1159,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 */
> > @@ -1164,9 +1168,11 @@ \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.
> > +
> > +The command VIRTIO_NET_CTRL_STATS_GET contains \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
> > @@ -1805,6 +1811,359 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > Upon reset, a device MUST initialize all coalescing parameters to 0.
> >
> > +\paragraph{Device Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats}
>
> "Stats" is an abbreviation - in the prose it would be better to use
> "statistics" consistently.
>
> > +
> > +If the VIRTIO_NET_F_DEVICE_STATS feature is negotiated, the driver can obtain
> > +device stats from the device by using the following command.
> > +
> > +Different types of virtqueues have different stats. The stats of the receiveq
> > +are different from those of the transmitq.
> > +
> > +The stats of a certain type of virtqueue are also divided into multiple types
> > +because different types require different features. This enables the expansion
> > +of new stats.
> > +
> > +At one time, the driver can obtain the stats of one or multiple virtqueues.
> > +Additionally, the driver can obtain multiple type stats of each virtqueue.
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_NET_CTRL_STATS 7
> > +#define VIRTIO_NET_CTRL_STATS_GET 0
> > +\end{lstlisting}
> > +
> > +To obtain device stats, use the VIRTIO_NET_CTRL_STATS_GET command with the
> > +\field{command-specific-data} containing the virtio_net_ctrl_queue_stats
> > +structure. The result is returned in the \field{command-specific-data-reply}.
> > +
> > +The following structure is used in \field{command-specific-data}:
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_queue_stats {
> > + struct {
> > + u16 vq_index;
> > + u48 padding;
>
> u8 padding[6] ? Not sure that u48 is used anywhere else.
>
> > +
> > +#define VIRTIO_NET_STATS_TYPE_CVQ (1 << 0)
> > +
> > +#define VIRTIO_NET_STATS_TYPE_RX_BASIC (1 << 0)
> > +#define VIRTIO_NET_STATS_TYPE_RX_CSUM (1 << 1)
> > +#define VIRTIO_NET_STATS_TYPE_RX_GSO (1 << 2)
> > +
> > +#define VIRTIO_NET_STATS_TYPE_TX_BASIC (1 << 0)
> > +#define VIRTIO_NET_STATS_TYPE_TX_CSUM (1 << 1)
> > +#define VIRTIO_NET_STATS_TYPE_TX_GSO (1 << 2)
> > +
> > + u64 types;
> > + } stats[];
> > +};
> > +\end{lstlisting}
> > +
> > +The following structures are used in \field{command-specific-data-reply}:
> > +\begin{lstlisting}
> > +struct virtio_net_stats_cvq {
> > + le64 command_num;
> > + le64 ok_num;
> > +};
> > +
> > +struct virtio_net_stats_rx_basic {
> > + le64 rx_packets;
> > + le64 rx_bytes;
> > +
> > + le64 rx_notification;
> > + le64 rx_interrupt;
> > +
> > + le64 rx_drop;
> > + le64 rx_drop_overruns;
> > + le64 rx_drop_busy;
> > +};
> > +
> > +struct virtio_net_stats_rx_csum {
> > + le64 rx_csum_valid;
> > + le64 rx_needs_csum;
> > + le64 rx_csum_bad;
> > + le64 rx_csum_none;
> > +};
> > +
> > +struct virtio_net_stats_rx_gso {
> > + le64 rx_gso_packets;
> > + le64 rx_gso_bytes;
> > + le64 rx_gso_packets_coalesced;
> > + le64 rx_gso_bytes_coalesced;
> > + le64 rx_gso_segments;
> > + le64 rx_gso_segments_bytes;
> > +};
> > +
> > +struct virtio_net_stats_tx_basic {
> > + le64 tx_packets;
> > + le64 tx_bytes;
> > +
> > + le64 tx_notification;
> > + le64 tx_interrupt;
> > +
> > + le64 tx_drop;
> > + le64 tx_drop_malformed;
> > +
> > + le64 tx_drop_busy;
> > +};
> > +
> > +struct virtio_net_stats_tx_csum {
> > + le64 tx_csum_none;
> > + le64 tx_needs_csum;
> > +};
> > +
> > +struct virtio_net_stats_tx_gso {
> > + le64 tx_gso_packets;
> > + le64 tx_gso_bytes;
> > + le64 tx_gso_packets_split;
> > + le64 tx_gso_bytes_split;
> > + le64 tx_gso_segments;
> > + le64 tx_gso_segments_bytes;
> > +};
> > +
> > +\end{lstlisting}
> > +
> > +\begin{description}
> > + \item [vq_index]
> > + The index of the virtqueue to obtain the stats.
> > +
> > + \item [types]
> > + This is a bitmask of the types of stats to be obtained. Therefore, a
> > + \field{struct stats} inside virtio_net_ctrl_queue_stats may instruct
> > + multiple stats replies for the virtqueue.
> > +\end{description}
> > +
> > +\subparagraph{Controlq Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats / Controlq Stats}
> > +
> > +The structure corresponding to the controlq stats is virtio_net_stats_cvq.
> > +
> > +\begin{description}
> > + \item [command_num]
> > + The number of commands including the current command.
> > +
> > + \item [ok_num]
> > + The number of commands (including the current command) where the ack was VIRTIO_NET_OK.
> > +\end{description}
> > +
> > +
> > +\subparagraph{Receiveq Basic Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats / Receiveq Basic Stats}
> > +
> > +The structure corresponding to the receiveq basic stats is virtio_net_stats_rx_basic.
> > +
> > +Receiveq basic stats doesn't require any feature. As long as the device supports
>
> s/doesn't/does not/
>
> > +VIRTIO_NET_F_DEVICE_STATS, the following are the receiveq basic stats.
> > +
> > +The packets described below are all steered to a specific virtqueue.
>
> "steered" seems like a loaded term. Perhaps "The packets described below
> were all presented on the specified virtqueue."?
>
> > +\begin{description}
> > + \item [rx_packets]
> > + This is the number of packets received by the device (not the packets
> > + passed to the guest). The count includes the packets dropped by the
> > + device.
> > +
> > + \item [rx_bytes]
> > + This is the bytes of packets received by the device (not the packets
> > + passed to the guest). The count includes the packets dropped by the
> > + device.
> > +
> > + \item [rx_notification]
> > + The number of driver notifications received by device for this receiveq.
> > +
> > + \item [rx_interrupt]
> > + The number of device interrupts for this receiveq.
> > +
> > + \item [rx_drop]
> > + This is the number of packets dropped by the device. The count includes
> > + all types of packets dropped by the device.
> > +
> > + \item [rx_drop_overruns]
> > + This is the number of packets dropped by the device when no more
> > + descriptors were available.
> > +
> > + \item [rx_drop_busy]
> > + This is the number of packets dropped by the device when the device is
> > + busy.
> > +
> > +\end{description}
> > +
> > +\subparagraph{Transmitq Basic Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats / Transmitq Basic Stats}
> > +
> > +The structure corresponding to VIRTIO_NET_STATS_TYPE_TX_BASIC is virtio_net_stats_tx_basic.
> > +
> > +Transmitq basic stats doesn't require any feature. As long as the device supports
> > +VIRTIO_NET_F_DEVICE_STATS, the following are the transmitq basic stats.
> > +
> > +The packets described below are all from a specific virtqueue.
> > +\begin{description}
> > + \item [tx_packets]
> > + This is the number of packets sent by the device (not the packets
> > + got from the driver).
> > +
> > + \item [tx_bytes]
> > + This is the bytes of packets sent by the device (not the packets
> > + got from the driver).
> > +
> > + \item [tx_notification]
> > + The number of driver notifications for this transmitq.
> > +
> > + \item [tx_interrupt]
> > + The number of device interrupts for this transmitq.
> > +
> > + \item [tx_drop]
> > + The number of packets dropped by the device. The count includes all
> > + types of packets dropped by the device.
> > +
> > + \item [tx_drop_malformed]
> > + The number of packets dropped by the device, when the descriptor is in
> > + an error state. For example, the buffer is too short.
> > +
> > + \item [tx_drop_busy]
> > + The number of packets dropped by the device, when the device is busy.
> > +
> > +\end{description}
> > +
> > +\subparagraph{Receiveq CSUM Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats / Receiveq CSUM Stats}
> > +
> > +The structure corresponding to VIRTIO_NET_STATS_TYPE_RX_CSUM is virtio_net_stats_rx_csum.
> > +
> > +Only after the VIRTIO_NET_F_GUEST_CSUM is negotiated, the receiveq csum stats
> > +can be obtained.
> > +
> > +The packets described below are all steered to a specific virtqueue.
> > +\begin{description}
> > + \item [rx_csum_valid]
> > + The number of packets with VIRTIO_NET_HDR_F_DATA_VALID.
> > +
> > + \item [rx_needs_csum]
> > + The number of packets with VIRTIO_NET_HDR_F_NEEDS_CSUM.
> > +
> > + \item [rx_csum_bad]
> > + The number of packets with abnormal csum.
> > +
> > + \item [rx_csum_none]
> > + The number of packets without hardware csum. The packet here refers to
> > + the non-TCP/UDP packet that the backend cannot recognize.
> > +
> > +\end{description}
> > +
> > +\subparagraph{Transmitq CSUM Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats / Transmitq CSUM Stats}
> > +
> > +The structure corresponding to VIRTIO_NET_STATS_TYPE_TX_CSUM is virtio_net_stats_tx_csum.
> > +
> > +Only after the VIRTIO_NET_F_CSUM is negotiated, the transmitq csum stats can be
> > +obtained.
> > +
> > +The following are the transmitq csum stats:
> > +
> > +The packets described below are all from a specific virtqueue.
> > +\begin{description}
> > + \item [tx_csum_none]
> > + The number of packets that didn't require hardware csum.
> > +
> > + \item [tx_needs_csum]
> > + The number of packets that required hardware csum.
> > +
> > +\end{description}
> > +
> > +\subparagraph{Receiveq GSO Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats / Receiveq GSO Stats}
> > +
> > +The structure corresponding to VIRTIO_NET_STATS_TYPE_RX_GSO is virtio_net_stats_rx_gso.
> > +
> > +If one or more of the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, or
> > +VIRTIO_NET_F_GUEST_UFO have been negotiated, the receiveq GSO stats can be
> > +obtained.
> > +
> > +GSO packets refer to packets passed by the device to the driver where
> > +\field{gso_type} is not VIRTIO_NET_HDR_GSO_NONE.
> > +
> > +The packets described below are all steered to a specific virtqueue.
> > +\begin{description}
> > + \item [rx_gso_packets]
> > + The number of the GSO packets received by device.
> > +
> > + \item [rx_gso_bytes]
> > + The bytes of the GSO packets received by device.
> > +
> > + \item [rx_gso_packets_coalesced]
> > + The number of the GSO packets coalesced by device.
> > +
> > + \item [rx_gso_bytes_coalesced]
> > + The bytes of the GSO packets coalesced by device.
> > +
> > + \item [rx_gso_segments]
> > + The number of the segments that make up GSO packets.
> > +
> > + \item [rx_gso_segments_bytes]
> > + The bytes of the segments that make up GSO packets.
> > +
> > +\end{description}
> > +
> > +\subparagraph{Transmitq GSO Stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats / Transmitq GSO Stats}
> > +
> > +The structure corresponding to VIRTIO_NET_STATS_TYPE_TX_GSO is virtio_net_stats_tx_gso.
> > +
> > +If one or more of the VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6,
> > +VIRTIO_NET_F_HOST_USO or VIRTIO_NET_F_HOST_UFO options have
> > +been negotiated, the transmitq GSO stats can be obtained.
> > +
> > +GSO packets refer to packets passed by the driver to the device where
> > +\field{gso_type} is not VIRTIO_NET_HDR_GSO_NONE.
> > +
> > +The packets described below are all from a specific virtqueue.
> > +\begin{description}
> > + \item [tx_gso_packets]
> > + The number of the GSO packets sent by device that are not split to small
> > + packets.
> > +
> > + \item [tx_gso_bytes]
> > + The bytes of the GSO packets sent by device that are not split to small
> > + packets.
> > +
> > + \item [tx_gso_packets_split]
> > + The number of the GSO packets that been split to small packets.
> > +
> > + \item [tx_gso_bytes_split]
> > + The bytes of the GSO packets that been split to small packets.
> > +
> > + \item [tx_gso_segments]
> > + The number of segments split from the GSO packets.
> > +
> > + \item [tx_gso_segments_bytes]
> > + The bytes of segments split from the GSO packets.
> > +\end{description}
> > +
> > +\devicenormative{\subparagraph}{Device Stats}{Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats}
> > +
> > +If virtio_net_ctrl_queue_stats is incorrect (such as the following), the device
> > +MUST set \field{ack} to VIRTIO_NET_ERR. Even if there is only one error,
> > +the device MUST fail the entire command.
> > +\begin{itemize}
> > + \item \field{vq_index} exceeds the queue range.
> > + \item \field{types} contains unknown types.
> > + \item The type of vq does not match \field{types}. E.g. the driver tries to query
> > + receiveq stats by the index of a transmitq.
>
> How would a device detect this, given that the bits used to express the
> statistics type overlap?
>
> If I ask for TX_CSUM on an RX queue, I'm going to get the RX_CSUM stats
> and the device cannot be aware of the discrepancy.
>
> This might be better as:
>
> \item One or more of the bits present in \field{types} is not valid for
> the specified virtqueue.
>
> > + \item The feature corresponding to the specified \field{types} was not negotiated.
> > + \item The size of the buffer allocated by the driver for \field{command-specific-data-reply}
> > + is less than the total size of the stats specialed by
> > + \field{virtio_net_ctrl_queue_stats}.
> > +\end{itemize}
> > +
> > +The device MUST write the requested stats structures in
> > +\field{command-specific-data-reply} in the order specified by the structure
> > +virtio_net_ctrl_queue_stats. If the \field{types} instructs multiple stats, the
> > +replies order by the type value from small to large.
>
> How are the multiple replies padded? "There is no additional padding
> between structures." would be sufficient, I think.
>
> > +
> > +\drivernormative{\subparagraph}{Device Stats}{Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats}
> > +
> > +When a driver tries to obtain a certain stats, it MUST confirm that the relevant
> > +features are negotiated.
> > +
> > +\field{types} in struct virtio_net_ctrl_queue_stats MUST correspond to the vq
> > +specified by \field{vq_index}.
> > +
> > +The \field{command-specific-data-reply} buffer allocated by the driver MUST be
> > +able to hold all the stats specified by virtio_net_ctrl_queue_stats.
> > +
> > +When the driver reads the replies, it MUST read
> > +\field{command-specific-data-reply} one by one based on the \field{types}.
>
> Not clear why this is here? The driver should be able to consume the
> result however it chooses.
>
> > +
> > \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> > Types / Network Device / Legacy Interface: Framing Requirements}
> >
> > diff --git a/device-types/net/device-conformance.tex b/device-types/net/device-conformance.tex
> > index f88f48b..a0c63d6 100644
> > --- a/device-types/net/device-conformance.tex
> > +++ b/device-types/net/device-conformance.tex
> > @@ -15,4 +15,5 @@
> > \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 / Notifications Coalescing}
> > \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
> > +\item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats}
> > \end{itemize}
> > diff --git a/device-types/net/driver-conformance.tex b/device-types/net/driver-conformance.tex
> > index 9d853d9..2f1c674 100644
> > --- a/device-types/net/driver-conformance.tex
> > +++ b/device-types/net/driver-conformance.tex
> > @@ -15,4 +15,5 @@
> > \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 / Notifications Coalescing}
> > \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
> > +\item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Device Stats}
> > \end{itemize}
> > --
> > 2.32.0.3.g01195cf9f
> >
> > This publicly archived list offers a means to provide input to the
> > OASIS Virtual I/O Device (VIRTIO) TC.
> >
> > In order to verify user consent to the Feedback License terms and
> > to minimize spam in the list archive, subscription is required
> > before posting.
> >
> > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > List help: virtio-comment-help@lists.oasis-open.org
> > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > Committee: https://www.oasis-open.org/committees/virtio/
> > Join OASIS: https://www.oasis-open.org/join/
> --
> Tonight I'm gonna bury that horse in the ground.
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
prev parent reply other threads:[~2023-08-07 11:08 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-31 7:27 [virtio-dev] [PATCH v14] virtio-net: support device stats Xuan Zhuo
2023-07-31 7:48 ` Xuan Zhuo
2023-08-07 8:03 ` [virtio-dev] " David Edmondson
2023-08-07 11:00 ` Xuan Zhuo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1691406058.5846093-1-xuanzhuo@linux.alibaba.com \
--to=xuanzhuo@linux.alibaba.com \
--cc=david.edmondson@oracle.com \
--cc=jasowang@redhat.com \
--cc=mst@redhat.com \
--cc=parav@nvidia.com \
--cc=virtio-comment@lists.oasis-open.org \
--cc=virtio-dev@lists.oasis-open.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).