Re: RE: RE: [PATCH v16] virtio-net: support device stats

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Mon, 4 Sep 2023 08:00:36 +0000, Parav Pandit <parav@nvidia.com> wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Monday, September 4, 2023 1:04 PM
>
> > > > +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
> > > > +statistics can be obtained.
> > > > +
> > > UFO is deprecated feature in the OS that supported it.
> > > And unlikely to be used in any new OS.
> > > So I think one should drop the UFO for now from above.
> >
> > But, the virtio spec supports it. So I think we should include it here.
> >
> UFO and receive GSO is bit different.
> And UFO has near to zero use.
> So if this is really needed, one can implement such counter later when UFO is actually implemented and used.


I agree.


>
> > > > +\begin{description}
> > > > +    \item [tx_gso_packets]
> > > > +        The number of the GSO packets sent by device that are not cut to
> > small
> > > > +        packets.
> > > > +
> > > Above is a strange counter.
> > > Do you have an example of it along with _split counter below?
> > > I am probably misunderstanding it.
> >
> > On one host, a vm can send a gso packet without cutting to other vm.
> >
> Do you mean some sw entity has sent the GSO packet directly such as 64KB SKB without creating segments?

YES.


> If so, how will the receive device can receive such large packet when the posted buffer is not worth the 64KB size?

The virtio-net merge mode.

>
> > >
> > > > +    \item [tx_gso_bytes]
> > > > +        The bytes of the GSO packets sent by device that are not cut to small
> > > > +        packets.
> > > > +
> > > > +    \item [tx_gso_packets_split]
> > > > +        The number of the GSO packets that been cut into small packets.
> > > > +
> > > Which has been cut into small packets.
> > >
> > > > +    \item [tx_gso_bytes_split]
> > > > +        The bytes of the GSO packets that been cut into small packets.
> > > > +
> > > > +    \item [tx_gso_segments]
> > > > +        The number of segments separated from the GSO packets.
> > > > +
> > > > +    \item [tx_gso_segments_bytes]
> > > > +        The bytes of segments separated from the GSO packets.
> > > > +\end{description}
> > > > +
> > > > +\subparagraph{Receiveq Speed Statistic}\label{sec:Device Types /
> > > > +Network Device / Device Operation / Control Virtqueue / Device
> > > > +Statistic / Receiveq Speed Statistic}
> > > > +
> > > > +The structure corresponding to the receiveq speed statistics is
> > > > 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 real speed exceeds the speed allowance, some packets will
> > > > +be dropped by the device.
> > > > +
> > > Above description regarding "real speed" is confusing.
> > > I think you wanted to say,
> > >
> > > When the received packets rate exceeds the negotiated speed, some packets
> > may be dropped by the device.
> > >
> > > > +\begin{lstlisting}
> > > > +struct virtio_net_stats_rx_speed {
> > > > +    struct virtio_net_stats_reply_hdr hdr;
> > > > +
> > > > +    le64 rx_packets_allowance_exceeded;
> > > > +    le64 rx_bytes_allowance_exceeded; }; \end{lstlisting}
> > > > +
> > > > +The packets described below were all presented on the specified virtqueue.
> > > > +\begin{description}
> > > > +    \item [rx_packets_allowance_exceeded]
> > > > +        The number of the packets dropped by device due to the real speed
> > > > +        exceeding the speed allowance.
> > > > +
> > > Similar rewording like above to drop "real".
> > >
> > > > +    \item [rx_bytes_allowance_exceeded]
> > > > +        The bytes of the packets dropped by device due to the real speed
> > > > +        exceeding the speed allowance.
> > > > +
> > > > +\end{description}
> > > > +
> > > > +\subparagraph{Transmitq Speed Statistic}\label{sec:Device Types /
> > > > +Network Device / Device Operation / Control Virtqueue / Device
> > > > +Statistic / Transmitq Speed Statistic}
> > > > +
> > > > +The structure corresponding to the transmitq speed statistics is
> > > > +virtio_net_stats_tx_speed. The corresponding type is
> > > > +VIRTIO_NET_STATS_TYPE_TX_SPEED. This is for the transmitq.
> > > > +
> > > > +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 real speed exceeds the speed allowance, some packets will
> > > > +be dropped by the device.
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_net_stats_tx_speed {
> > > > +    struct virtio_net_stats_reply_hdr hdr;
> > > > +
> > > > +    le64 tx_packets_allowance_exceeded;
> > > > +    le64 tx_bytes_allowance_exceeded; }; \end{lstlisting}
> > > > +
> > > > +The packets described below were all presented on the specified virtqueue.
> > > > +\begin{description}
> > > > +    \item [tx_packets_allowance_exceeded]
> > > > +        The number of the packets dropped by device due to the real speed
> > > > +        exceeding the speed allowance.
> > > > +
> > > > +    \item [tx_bytes_allowance_exceeded]
> > > > +        The bytes of the packets dropped by device due to the real speed
> > > > +        exceeding the speed allowance.
> > > > +
> > > > +\end{description}
> > > > +
> > > > +\devicenormative{\subparagraph}{Device Statistic}{Device Types /
> > > > +Network Device / Device Operation / Control Virtqueue / Device
> > > > +Statistic} The device MUST replies to the command
> > > > +VIRTIO_NET_CTRL_STATS_QUERY with the struct
> > virtio_net_stats_capabilities.
> > > > \field{supported_stats_types} includes all the statistic types
> > > > supported by the device.
> > > > +
> > > > +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_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{virtio_net_ctrl_queue_stats}.
> > > > +\end{itemize}
> > > > +
> > > > +The device MUST write the requested statistic structures in
> > > s/statistic/statistics
> > >
> > > > +\field{command-specific-result} in the order specified by the
> > > > +structure virtio_net_ctrl_queue_stats. If the \field{types_bitmap}
> > > > +indicates
> > > Use \field{..}
> > >
> > > > +multiple statistics, the replies order by the type value from small to large.
> > > > +
> > > These replies are ordered by the type value from small to large.
> > > I am not sure device needs to order them.
> >
> > Now, the hdr of statistics has the type field.
> >
> > We can remove this.
> >
> We still need the type field, just need to remove the ordering need.


Yes. I think so.

Thanks.