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

[Parav Pandit <parav@nvidia.com>]

Hi Xuan,

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Thursday, September 7, 2023 11:36 AM

I am sorry I missed few small comments in v16.
Please find it below.

[..]

> +\begin{lstlisting}
> +struct virtio_net_ctrl_queue_stats {
> +       struct {
> +           le16 vq_index;
> +           le16 reserved[3];
> +           le64 types_bitmap[1];
> +       } stats[];
> +};
> +
> +struct virtio_net_stats_reply_hdr {
> +#define VIRTIO_NET_STATS_TYPE_REPLY_CVQ       32
> +
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_BASIC  0
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_CSUM   1
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_GSO    2
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_SPEED  3
> +
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_BASIC  16
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_CSUM   17
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_GSO    18
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_SPEED  19
> +    u8 type;
> +    u8 reserved;
> +    le16 vq_index;
> +    le32 size;
> +}
> +\end{lstlisting}
> +
> +To obtain device statistics, use the VIRTIO_NET_CTRL_STATS_GET command
> +with the \field{command-specific-data} which is in the format of
> +\field{struct virtio_net_ctrl_queue_stats}. When the command completes
> +successfully, \field{command-specific-result} contains multiple
> +statistic results, each statistic result has the \field{struct
> +virtio_net_stats_reply_hdr} as the header.
> +
> +The fields of the \field{struct virtio_net_ctrl_queue_stats}:
> +\begin{description}
> +    \item [vq_index]
> +        The index of the virtqueue to obtain the statistics.
> +
> +    \item [types_bitmap]
> +        This is a bitmask of the types of statistics to be obtained. Therefore, a
> +        \field{stats} inside \field{struct virtio_net_ctrl_queue_stats} may
> +        indicate multiple statistic replies for the virtqueue.
> +\end{description}
> +
> +The fields of the \field{struct virtio_net_stats_reply_hdr}:
> +\begin{description}
> +    \item [type]
> +        The type of the reply statistic.
> +
> +    \item [vq_index]
> +        The virtqueue index of the reply statistic.
> +
> +    \item [size]
> +        The size of bytes of the reply statistic.
I missed to review this before.
We need to tell if the size contains the reply header or not.
So, how about,

The number of bytes for the statistics entry including size of \field{struct virtio_net_stats_reply_hdr}.

[..]

> +    \item [tx_gso_segments]
> +        The number of segments cutted from GSO packets.
> +
s/cutted/prepared

> +    \item [tx_gso_segments_bytes]
> +        The bytes of segments cutted from GSO packets.
> +

> +The structure corresponding to the receiveq speed statistics is
> +\field{struct virtio_net_stats_rx_speed}. The corresponding type is
> +VIRTIO_NET_STATS_TYPE_RX_SPEED. This is for the receiveq.
> +
> +The device has the allowance for the speed. If
> +VIRTIO_NET_F_SPEED_DUPLEX has been negotiated, the driver can get this
> +by \field{speed}. When the received packets rate exceeds the
> +\field{speed}, some packets may be dropped by the device.

For 100Gbps speed, at 1500 bytes, 'packet rate' is roughly 8 mpps.
So what you wanted to say here is the packet bit rate instead of packet rate.

Please change from packet rate to packet bitrate.

> +\devicenormative{\subparagraph}{Device Statistics}{Device Types /
> +Network Device / Device Operation / Control Virtqueue / Device
> +Statistics} The device MUST replies to the command
s/replies/reply

I think you want to add the line, 
When feature bit F_STATS is negotiated, the device MUST reply ...

> +VIRTIO_NET_CTRL_STATS_QUERY with the \field{struct
> virtio_net_stats_capabilities}. \field{supported_stats_types} includes all the
> statistic types supported by the device.
> +
> +If \field{struct 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_bitmap} contains unknown types.
> +    \item One or more of the bits present in \field{types_bitmap} is not valid
> +        for the specified virtqueue.
> +    \item The feature corresponding to the specified \field{types_bitmap} was
> +        not negotiated.
> +    \item The size of the buffer allocated by the driver for \field{command-
> specific-result}
> +        is less than the total size of the statistics specialed by
> +        \field{struct virtio_net_ctrl_queue_stats}.
> +\end{itemize}
> +
> +The device MUST set the actual size of the space occupied by the reply
s/space/bytes

> +to the \field{size} of the \field{hdr}. And the device MUST set the
> +\field{type} and the \field{vq_index} of the statistic header.
> +
> +\drivernormative{\subparagraph}{Device Statistics}{Device Types /
> +Network Device / Device Operation / Control Virtqueue / Device
> +Statistics}
> +
> +The types contained in the \field{types_bitmap} MUST be queried from
> +the device via command VIRTIO_NET_CTRL_STATS_QUERY.
> +
> +\field{types_bitmap} in \field{struct virtio_net_ctrl_queue_stats} MUST
> +be valid to the vq specified by \field{vq_index}.
> +
> +The \field{command-specific-result} buffer allocated by the driver MUST
> +be able to hold all the statistics specified by \field{struct
> +virtio_net_ctrl_queue_stats}.
> +
This may not be true when we consider backward compatibility.
For example, driver of 1.4 will allocate 100 bytes and device follows spec 1.5 which may be returning another additional 150 bytes.

So it should be rephrased as,

The \field{command-specific-result} buffer allocated by the driver may be smaller or bigger than all the statistics specified by 
\field{struct virtio_net_ctrl_queue_stats}. The device MUST fill up only upto the valid bytes.

I think you need to also add driver requirement that buffer should be at least aligned to the size of the reply header so that there is no partial entry in response.