[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] [PATCH v2] virtio-net: support device stats
On Fri, Sep 3, 2021 at 10:11 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Thu, 2 Sep 2021 15:39:44 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > å 2021/8/27 äå2:58, Xuan Zhuo åé:
> > > On Fri, 27 Aug 2021 11:45:55 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >> å 2021/8/27 äå11:22, Xuan Zhuo åé:
> > >>> On Thu, 26 Aug 2021 12:22:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >>>> å 2021/8/23 äå4:27, Xuan Zhuo åé:
> > >>>>> This patch allows the driver to obtain some statistics from the device.
> > >>>>>
> > >>>>> In the back-end implementation, we can count a lot of such information,
> > >>>>> which can be used for debugging and judging the running status of the
> > >>>>> back-end. We hope to directly display it to the user through ethtool.
> > >>>>>
> > >>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > >>>>> ---
> > >>>>> v2: All keys define by this patch. Not defined by each backend.
> > >>>>>
> > >>>>> content.tex | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >>>>> 1 file changed, 117 insertions(+)
> > >>>>>
> > >>>>> diff --git a/content.tex b/content.tex
> > >>>>> index 70a9765..b9fd10a 100644
> > >>>>> --- a/content.tex
> > >>>>> +++ b/content.tex
> > >>>>> @@ -2978,6 +2978,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.
> > >>>>> @@ -3023,6 +3026,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}
> > >>>>> @@ -3901,6 +3905,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 */
> > >>>>> @@ -3912,6 +3917,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >>>>> 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.
> > >>>>> +\field{command-specific-data-reply} is alloced by driver, then pass to device.
> > >>>>> +It is filled by the device. It is optional. These commands need to get some
> > >>>>> +information from the device.
> > >>>>>
> > >>>>> \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
> > >>>>> @@ -4390,6 +4398,115 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> > >>>>> See \ref{sec:Basic
> > >>>>> Facilities of a Virtio Device / Virtqueues / Message Framing}.
> > >>>>>
> > >>>>> +\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 by \field{command-specific-data-reply}.
> > >>>>> +
> > >>>>> +\subparagraph{Device stats keys}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device stats keys / Device stats keys}
> > >>>>> +
> > >>>>> +The keys of the device stats are defined as follows. When the device fills in
> > >>>>> +\field{command-specific-data-reply}, it MUST be written in the following order.
> > >>>>> +Subsequent newly added keys MUST be added at the end of each category.
> > >>>>> +These keys will eventually be shown to the user.
> > >>>>> +
> > >>>>> +All stats are divided into three categories:
> > >>>>> +
> > >>>>> +\begin{itemize}
> > >>>>> + \item dev stats: the stats of the device
> > >>>>> + \begin{itemize}
> > >>>>> + \item dev_freeze: The number of device freezes.
> > >>>>> + \item dev_restore: The number of device restore.
> > >>>> Have we defined freezes/restore function int the spec?
> > >>>>
> > >>>> And I think the key should be defined like a C macro as others in the spec.
> > >>> Yes, I think these two should be deleted, adding a "dev_reset" would be more
> > >>> appropriate.
> > >>>
> > >>>>> + \end{itemize}
> > >>>>> +
> > >>>>> + \item rx stats: the stats of the rx queue
> > >>>>> + \begin{itemize}
> > >>>>> + \item rx[N]_inter: The number of interrupts sent by the rx queue.
> > >>>>> + \item rx[N]_drop: The number of packets dropped by the rx queue. Contains all kinds of packet loss.
> > >>>>> + \item rx[N]_drop_overruns: The number of packets dropped by the rx queue when no more avail desc.
> > >>>>> + \item rx[N]_csum_bad: The number of packets with abnormal csum in the rx queue.
> > >>>>> + \item rx[N]_lro_packets: The number of lro packets received by rx.
> > >>>>> + \item rx[N]_lro_bytes: The number of lro bytes received by rx.
> > >>>> Let's use gso instead of lro since lro is never defined in the spec.
> > >>> OK.
> > >>>
> > >>>>> + \item rx[N]_oversize_pkts: The number of oversized packets received by the rx queue.
> > >>>>> + \end{itemize}
> > >>>>> +
> > >>>>> + \item tx stats: the stats of the tx queue
> > >>>>> + \begin{itemize}
> > >>>>> + \item tx[N]_inter: The number of interrupts sent by the tx queue.
> > >>>>> + \item tx[N]_drop: The number of packets dropped by the tx queue. Contains all kinds of packet loss.
> > >>>>> + \item tx[N]_csum: The number of packets that completes csum by the device.
> > >>>>> + \item tx[N]_tso_packets: The number of TSO packets transmitted.
> > >>>> Let's use gso instead.
> > >>>>
> > >>>>
> > >>>>> + \item tx[N]_tso_bytes: The number of TSO bytes transmitted.
> > >>>>> + \end{itemize}
> > >>>>> +\end{itemize}
> > >>>>> +
> > >>>>> +\subparagraph{Device stats get}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device stats keys / Device stats get}
> > >>>>> +
> > >>>>> +To get the stats, the following definitions are used:
> > >>>>> +\begin{lstlisting}
> > >>>>> +#define VIRTIO_NET_CTRL_STATS 6
> > >>>>> +#define VIRTIO_NET_CTRL_STATS_GET 0
> > >>>>> +\end{lstlisting}
> > >>>>> +
> > >>>>> +The following layout structure are used:
> > >>>>> +
> > >>>>> +\field{command-specific-data}
> > >>>>> +\begin{lstlisting}
> > >>>>> +le64 version;
> > >>>>> +le64 dev_stats_num;
> > >>>>> +le64 rx_num;
> > >>>>> +le64 tx_num;
> > >>>>> +le64 rx_stats_num;
> > >>>>> +le64 tx_stats_num;
> > >>>>> +\end{lstlisting}
> > >>>>> +
> > >>>>> +\field{command-specific-data-reply}
> > >>>>> +\begin{lstlisting}
> > >>>>> +le64 dev_stats_num;
> > >>>>> +le64 rx_num;
> > >>>>> +le64 tx_num;
> > >>>> Any reason for having this, can we simply use max_virtqueue_pairs?
> > >>>>
> > >>> Theoretically, rx_num and tx_num can be removed, and all use
> > >>> max_virtqueue_pairs.
> > >>> The reason for not doing this are:
> > >>>
> > >>> 1. Explicitly specify rx_num, tx_num in the command-specific-data, rather than
> > >>> implicitly use max_virtqueue_pairs because this is related to the size of the
> > >>> command-specific-data-reply allocated by the driver.
> > >>
> > >> I'm not quite sure how useful for this. E.g the device can fail if the
> > >> buffer doesn't have sufficient space in this case.
> > >>
> > >>
> > >>> 2. The number of queues that the driver may actually use is less than
> > >>> max_virtqueue_pairs, so you don't need to get the stats of all the queues.
> > >>
> > >> Yes, but it doesn't harm. E.g we have 4 max virtqueue pairs. And we do
> > >> the following changes:
> > >>
> > >> 1) use 2 qp
> > >> 2) use 4 qp
> > >> 3) use 2 qp
> > >>
> > >> After those steps we may still need to know the stats of all qps.
> > > I think, I have complicated the problem. I will delete rx_num, tx_num from
> > > command-specific-data and command-specific-data-reply in the next version.
> > >
> > > But I still think that a queue_pairs should be added to the
> > > command-specific-data to display the information indicating the maximum number
> > > of queues that can be placed in the command-specific-data-reply. Although this
> > > value should be equal to max_virtqueue_pairs. This is related to the size
> > > of command-specific-data-reply.
> >
> >
> > Re-think about this. I think we probably need to do something even more
> > simpler.
> >
> > Fetch the stats once per queue pair.
> >
> > E.g specify the queue pair index as command-specific data. And then
> > software can choose how many queue pairs it want to read?
>
> The following is the last time I replied to you, it is to use queue_pairs
> instead of rx_num, tx_num.
>
> \field{command-specific-data}
> \begin{lstlisting}
> le64 dev_stats_num;
> le64 rx_stats_num;
> le64 tx_stats_num;
> le64 queue_pairs;
> \end{lstlisting}
>
> \field{command-specific-data-reply}
> \begin{lstlisting}
> le64 dev_stats_num;
> le64 rx_stats_num;
> le64 tx_stats_num;
> le64 dev_stats[dev_stats_num];
> le64 rx_stats[rx_stats_num][queue_pairs];
> le64 tx_stats[tx_stats_num][queue_pairs];
> \end{lstlisting}
>
>
> I don't know if I understand it right, do you mean to use the following
> structure to get the information of the specified number of queue pairs based on
> queue_pair_index/queue_pair_num?
Something like this.
I think queue_pair_index should be sufficient.
Thanks
>
> queue_pair_index
> \field{command-specific-data}
> \begin{lstlisting}
> le64 dev_stats_num;
> le64 rx_stats_num;
> le64 tx_stats_num;
> le64 queue_pair_index;
> le64 queue_pair_num;
> \end{lstlisting}
>
> Thanks.
>
> >
> > Thanks
> >
> >
> > >
> > >>
> > >>> 3. In addition, I am not sure, whether to add rx_index, tx_index used to
> > >>> specify which rx/tx to start from. Then we can specify to obtain certain rx
> > >>> or tx information. I am not sure whether to add such a function.
> > >>
> > >> I think if a simple interface work we probably don't want a complicated
> > >> one. E.g the software (ethtool) can choose to expose partial stats.
> > > Yes.
> > >
> > >>
> > >>>
> > >>>
> > >>>
> > >>> The addition of tx_num and rx_num in the command-specific-data-reply is to
> > >>> consider that the driver may set rx_num and tx_num in the command-specific-data
> > >>> to be too large or too small. So the reply clearly pointed out that the actual
> > >>> number of rx and tx replies .
> > >>>
> > >>>>> +le64 rx_stats_num;
> > >>>>> +le64 tx_stats_num;
> > >>>>> +le64 dev_stats[dev_stats_num];
> > >>>>> +le64 rx_stats[rx_stats_num][rx_num];
> > >>>>> +le64 tx_stats[tx_stats_num][tx_num];
> > >>>> I guess the version implies rx_stats_num?
> > >>>>
> > >>> Sorry, I didn't understand the meaning.
> > >>
> > >> I meant, e.g the device/drivers should know how many stats that will be
> > >> fetched if a version is specified.
> > >>
> > >> Or is there a chance that we may have different rx_stats_num for one
> > >> version?
> > > First of all, as we discussed before, the "version" has been removed. If the
> > > structure of the later command-specific-data or command-specific-data-reply
> > > changes, we should add a new command.
> > >
> > > And considering that the change of stats keys is relatively a frequent event. So
> > > I consider using rx_stats_num/tx_stats_num to negotiate the number of stats
> > > between the driver and the device. Because the order of stats is fixed, and the
> > > newly added keys are always maintained at the end, so as long as we know the
> > > number of stats supported by the driver or device, we can know which keys the
> > > other party supports.
> > >
> > > Therefore, the rx_stats_num/tx_stats_num in the command-specific-data is used to
> > > inform the device, the number of stats supported by the driver, and how much
> > > space is allocated.
> > >
> > > The rx_stats_num/tx_stats_num in the command-specific-data-reply is used to tell
> > > the driver the number of stats actually supported by the device.
> > >
> > > The function of dev_stats_num is similar.
> > >
> > > \field{command-specific-data}
> > > \begin{lstlisting}
> > > le64 dev_stats_num;
> > > le64 rx_stats_num;
> > > le64 tx_stats_num;
> > > le64 queue_pairs;
> > > \end{lstlisting}
> > >
> > > \field{command-specific-data-reply}
> > > \begin{lstlisting}
> > > le64 dev_stats_num;
> > > le64 rx_stats_num;
> > > le64 tx_stats_num;
> > > le64 dev_stats[dev_stats_num];
> > > le64 rx_stats[rx_stats_num][queue_pairs];
> > > le64 tx_stats[tx_stats_num][queue_pairs];
> > > \end{lstlisting}
> > >
> > >
> > > Thanks.
> > >
> > >> Thanks
> > >>
> > >>
> > >>>>> +\end{lstlisting}
> > >>>>> +
> > >>>>> +The command VIRTIO_NET_CTRL_STATS_GET is used to obtain this information.
> > >>>>> +
> > >>>>> +Regarding the variables in \field{command-specific-data}:
> > >>>>> +\begin{itemize}
> > >>>>> + \item \field{version}: This is used to specify the version of the layout used. The current value MUST been 0.
> > >>>> I wonder if it's better to just extend the command instead of using
> > >>>> version here. E.g VIRTIO_NET_CTRL_STATS_GET_EXTRA etc.
> > >>> Yes, the new command is better than "version".
> > >>>
> > >>>>> + \item \field{dev_stats_num}: Indicates the number of dev stats supported by the driver.
> > >>>>> + \item \field{rx_num}: Indicates how many rx queue information the driver wants to receive.
> > >>>>> + \item \field{tx_num}: Indicates how many tx queue information the driver wants to receive.
> > >>>>> + \item \field{rx_stats_num}: Indicates the number of stats information the driver wants for each rx queue.
> > >>>>> + \item \field{tx_stats_num}: Indicates the number of stats information the driver wants for each tx queue.
> > >>>>> +\end{itemize}
> > >>>>> +
> > >>>>> +When the driver allocates \field{command-specific-data-reply}, it should set the
> > >>>>> +size of \field{command-specific-data-reply} based on the above values.
> > >>>>> +
> > >>>>> +\begin{lstlisting}
> > >>>>> +size = 5 * 8 + 8 * dev_stats_num + 8 * rx_num * rx_stats_num + 8 * tx_num * tx_stats_num;
> > >>>>> +\end{lstlisting}
> > >>>>> +
> > >>>>> +Regarding the variables in \field{command-specific-data-reply},
> > >>>>> +these variables(\field{dev_stats_num}, \field{rx_num}, \field{tx_num},
> > >>>>> +\field{rx_stats_num}, \field{tx_stats_num}) are set by the device and MUST be
> > >>>>> +less than or equal to the variables with the same name in
> > >>>>> +\field{command-specific-data}. These values indicate the amount actually filled
> > >>>>> +in. And the number of these implementations in
> > >>>>> +\field{command-specific-data-reply} is used as the size of the array dev_stats
> > >>>>> +and rx_stats and tx_stats.
> > >>>>> +
> > >>>>> +\field{command-specific-data-reply} is meaningful only when \field{ack} is equal to VIRTIO_NET_OK.
> > >>>>> +
> > >>>>> +\subparagraph{Legacy Interface: Device stats}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device stats / Legacy Interface: Device stats}
> > >>>>> +When using the legacy interface, transitional devices and drivers
> > >>>>> +MUST format the all variables according to the native endian of the guest rather
> > >>>>> +than (necessarily when not using the legacy interface) little-endian.
> > >>>> I'm not sure it's worth to add the support for legacy driver. Since it
> > >>>> can't support VIRTIO_NET_F_CTRL_STATS because it has only 32 bit features.
> > >>> OK. I will remove this in the next version.
> > >>>
> > >>> Thanks.
> > >>>
> > >>>> Thanks
> > >>>>
> > >>>>
> > >>>>> +
> > >>>>> \section{Block Device}\label{sec:Device Types / Block Device}
> > >>>>>
> > >>>>> The virtio block device is a simple virtual block device (ie.
> > >>>>> --
> > >>>>> 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
> > >>>>
> >
>
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]