RE: RE: [PATCH v15] virtio-net: support device stats

[Parav Pandit <parav@nvidia.com>]

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Tuesday, August 29, 2023 9:00 AM

> > Its over cvq, so lets do,
> >
> > s/channel/virtqueue
> >
> > Please rename
> > s/command-specific-data-reply/command_specific_result
> 
> 
> I agree.
> 
> But it will be "command-specific-result" for alignment with 'command-specific-
> data'.
>
Right.
 
> > > There is little the driver can do
> > > +except issue a diagnostic if \field{ack} is not VIRTIO_NET_OK.
> > > +
> > May be driver can retry the command, put to error state, issue diagnostics,
> ignore the error.
> > So There is no need to mention what driver can/cannot do, so please just
> drop the last line.
> 
> This is not new. It turns out that there is this line.
> 
:(
I see it now.

> I think, if you want to remove this, you should post a new patch.
>
It will be separate patch.
 
> >
> > > +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,424 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > > +Types /
> > > Network Device / Devi
> > >
> > >  Upon reset, a device MUST initialize all coalescing parameters to 0.
> > >
> > > +\paragraph{Device Statistic}\label{sec:Device Types / Network
> > > +Device / Device Operation / Control Virtqueue / Device Statistic}
> > > +
> > > +If the VIRTIO_NET_F_DEVICE_STATS feature is negotiated, the driver
> > > +can obtain device stats from the device by using the following command.
> > > +
> > s/device stats/device statistics at above and throughout many places below.
> > in the spec wording, better to not shorten them.
> >

> > > +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.
> > May be
> > s/At one time/In one command
> > This is to be more accurate, otherwise in one time we miss the notion of
> command.

> >
> > > +Additionally, the driver can obtain multiple type stats of each virtqueue.
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_NET_CTRL_STATS         7
> > > +#define VIRTIO_NET_CTRL_STATS_QUERY   0
> > > +#define VIRTIO_NET_CTRL_STATS_GET     1
> > > +\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}.
> > > +
> > We opted for slightly different wording for describing command specific data
> and results in aq.
> > So lets align it here.
> > Please reword above as,
> > The \field{command-specific-data} is in the format of \field{ struct
> virtio_net_ctrl_queue_stats}.
> >
> > When the command completes successfully, \field{command_specific_result}
> is in the format of \field{struct virtio_net_stats_capabilites}.
> >
> > > +The following structure is used in \field{command-specific-data}:
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_queue_stats {
> > > +       struct {
> > > +           le16 vq_index;
> > > +           le16 padding[3];
> > > +           le64 types[1];
> > > +       } stats[];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The following structures are used in \field{command-specific-data-reply}:
> > > +\begin{lstlisting}
> > > +struct virtio_net_stats_capabilites {
> > > +
> > > +#define VIRTIO_NET_STATS_TYPE_CVQ       (1 << 0)
> > > +#define VIRTIO_NET_STATS_TYPE_RX_BASIC  (1 << 1)
> > > +#define VIRTIO_NET_STATS_TYPE_RX_CSUM   (1 << 2)
> > > +#define VIRTIO_NET_STATS_TYPE_RX_GSO    (1 << 3)
> > > +#define VIRTIO_NET_STATS_TYPE_TX_BASIC  (1 << 4)
> > > +#define VIRTIO_NET_STATS_TYPE_TX_CSUM   (1 << 5)
> > > +#define VIRTIO_NET_STATS_TYPE_TX_GSO    (1 << 6)
> > > +    le64 supported_stats_types[1];
> > > +}
> > > +
> > It would be best to split the command and description into two. One for the
> capabilities bit map.
> > And second for the actual stats header.
> > This avoids the mix up between the two.
> >
> > > +struct virtio_net_stats_reply_hdr {
> > > +    le16 size;
> > > +    le16 vq_index;
> > > +
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_CVQ       0
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_BASIC  1
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_CSUM   2
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_GSO    3
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_BASIC  4
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_CSUM   5
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_GSO    6
> > > +    u8 type;
> > If the query command input has it, we likely don't need it in the response.
> 
> YES.
> 
> But the driver may need to handle multiple reply stats. I think that will be easy
> for the driver to handle so many stats with the vq_index and type.
>
Do you mean one command result returns multiple stats in one response?
And if so, and the types in the _data is bitmap, better to rename it as types_bmap or types_bitmap.

> > > +    \item [rx_drop_busy]
> > > +        This is the number of packets dropped by the device when the device
> is
> > > +        busy.
> > > +
> > I feel this is bit abstract counter and qualify the device as busy is tricky. Lets
> move it out of the basic counter.
> 
> I will rethink about this.
>
Ok.

> > We need to add the capabilities and query them from the AQ too.
> > If the reply header structure and actual stats are split properly, it will be easy
> to add the query by the owner device.
> 
> YES. I agree.
> 
> But I would to know is there some specific requirements?
>
Not sure I understand the question.
We have this requirement in the requirements document of 1.4 at [1].

[1] https://lists.oasis-open.org/archives/virtio-comment/202308/msg00260.html