[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 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?
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]