[virtio-dev] Re: [PATCH] virtio-vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
On Tue, Aug 29, 2023 at 06:21:35PM -0400, Michael S. Tsirkin wrote:
> On Tue, Aug 29, 2023 at 09:29:45PM +, Bobby Eshleman wrote:
> > This adds support for datagrams to the virtio-vsock device.
> >
> > virtio-vsock already supports stream and seqpacket types. The existing
> > message types and header fields are extended to support datagrams.
> > Semantic differences between the flow types are stated, as well as any
> > additional requirements for devices and drivers implementing this
> > feature.
> >
> > Signed-off-by: Bobby Eshleman
> > ---
> > device-types/vsock/description.tex | 95 +++---
> > 1 file changed, 88 insertions(+), 7 deletions(-)
> >
> > diff --git a/device-types/vsock/description.tex
> > b/device-types/vsock/description.tex
> > index 7d91d159872f..638dca8e5da1 100644
> > --- a/device-types/vsock/description.tex
> > +++ b/device-types/vsock/description.tex
> > @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket
> > Device / Feature bits}
> > \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
> > \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
> > \item[VIRTIO_VSOCK_F_NO_IMPLIED_STREAM (2)] stream socket type is not
> > implied.
> > +\item[VIRTIO_VSOCK_F_DGRAM (3)] datagram socket type is supported.
> > \end{description}
> >
> > \drivernormative{\subsubsection}{Feature bits}{Device Types / Socket
> > Device / Feature bits}
> > @@ -167,17 +168,22 @@ \subsubsection{Addressing}\label{sec:Device Types /
> > Socket Device / Device Opera
> > consists of a (cid, port number) tuple. The header fields used for this are
> > \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
> >
> > -Currently stream and seqpacket sockets are supported. \field{type} is 1
> > (VIRTIO_VSOCK_TYPE_STREAM)
> > -for stream socket types, and 2 (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket
> > socket types.
> > +
> > +Currently stream, seqpacket, and datagram sockets are supported.
> > \field{type} is
> > +1 (VIRTIO_VSOCK_TYPE_STREAM) for stream socket types, 2
> > (VIRTIO_VSOCK_TYPE_SEQPACKET) for
> > +seqpacket socket types, and 3 (VIRTIO_VSOCK_TYPE_DGRAM) for datagram
> > socket types.
> >
> > \begin{lstlisting}
> > #define VIRTIO_VSOCK_TYPE_STREAM1
> > #define VIRTIO_VSOCK_TYPE_SEQPACKET 2
> > +#define VIRTIO_VSOCK_TYPE_DGRAM 3
> > \end{lstlisting}
> >
> > Stream sockets provide in-order, guaranteed, connection-oriented delivery
> > without message boundaries. Seqpacket sockets provide in-order, guaranteed,
> > -connection-oriented delivery with message and record boundaries.
> > +connection-oriented delivery with message and record boundaries. Datagram
> > +sockets provide connection-less, best-effort delivery of messages, with no
> > +order or reliability guarantees.
> >
> > \subsubsection{Buffer Space Management}\label{sec:Device Types / Socket
> > Device / Device Operation / Buffer Space Management}
> > \field{buf_alloc} and \field{fwd_cnt} are used for buffer space management
> > of
> > @@ -203,16 +209,19 @@ \subsubsection{Buffer Space
> > Management}\label{sec:Device Types / Socket Device /
> > previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This allows
> > communicating updates any time a change in buffer space occurs.
> >
> > +\field{buf_alloc} and \field{fwd_cnt} are reserved for future use by
> > datagram
> > +sockets. These fields are not used for datagram buffer space management.
>
> no limits on datagram size?
>
In the Linux proof-of-concept, it is 64KB. I can add that here too.
> also with no flow control at all there's a performance problem:
> imagine queue gets full. now buffers begin to be dropped.
> problem is, dropping is faster than delivering to application.
> so now application sees its packets consumed super quickly
> and starts sending even faster.
> not good for performance.
>
> yes datagram expects some way to drop packets but just disabling flow
> control completely is not the right thing to do I think.
>
On the LKML I discussed using congestion notification as a way to handle
this situation, but deferred it to a future feature bit. I can build
it in from the beginning though.
>
> > +
> > \drivernormative{\paragraph}{Device Operation: Buffer Space
> > Management}{Device Types / Socket Device / Device Operation / Buffer Space
> > Management}
> > -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> > -sufficient free buffer space for the payload.
> > +For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets MUST only
> > be
> > +transmitted when the peer has sufficient free buffer space for the payload.
> >
> > All packets associated with a stream flow MUST contain valid information in
> > \field{buf_alloc} and \field{fwd_cnt} fields.
> >
> > \devicenormative{\paragraph}{Device Operation: Buffer Space
> > Management}{Device Types / Socket Device / Device Operation /
[virtio-dev] Re: [PATCH] virtio-vsock: add VIRTIO_VSOCK_F_DGRAM feature bit
On Tue, Aug 29, 2023 at 09:29:45PM +, Bobby Eshleman wrote:
> This adds support for datagrams to the virtio-vsock device.
>
> virtio-vsock already supports stream and seqpacket types. The existing
> message types and header fields are extended to support datagrams.
> Semantic differences between the flow types are stated, as well as any
> additional requirements for devices and drivers implementing this
> feature.
>
> Signed-off-by: Bobby Eshleman
> ---
> device-types/vsock/description.tex | 95 +++---
> 1 file changed, 88 insertions(+), 7 deletions(-)
>
> diff --git a/device-types/vsock/description.tex
> b/device-types/vsock/description.tex
> index 7d91d159872f..638dca8e5da1 100644
> --- a/device-types/vsock/description.tex
> +++ b/device-types/vsock/description.tex
> @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket
> Device / Feature bits}
> \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
> \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
> \item[VIRTIO_VSOCK_F_NO_IMPLIED_STREAM (2)] stream socket type is not
> implied.
> +\item[VIRTIO_VSOCK_F_DGRAM (3)] datagram socket type is supported.
> \end{description}
>
> \drivernormative{\subsubsection}{Feature bits}{Device Types / Socket Device
> / Feature bits}
> @@ -167,17 +168,22 @@ \subsubsection{Addressing}\label{sec:Device Types /
> Socket Device / Device Opera
> consists of a (cid, port number) tuple. The header fields used for this are
> \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
>
> -Currently stream and seqpacket sockets are supported. \field{type} is 1
> (VIRTIO_VSOCK_TYPE_STREAM)
> -for stream socket types, and 2 (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket
> socket types.
> +
> +Currently stream, seqpacket, and datagram sockets are supported.
> \field{type} is
> +1 (VIRTIO_VSOCK_TYPE_STREAM) for stream socket types, 2
> (VIRTIO_VSOCK_TYPE_SEQPACKET) for
> +seqpacket socket types, and 3 (VIRTIO_VSOCK_TYPE_DGRAM) for datagram socket
> types.
>
> \begin{lstlisting}
> #define VIRTIO_VSOCK_TYPE_STREAM1
> #define VIRTIO_VSOCK_TYPE_SEQPACKET 2
> +#define VIRTIO_VSOCK_TYPE_DGRAM 3
> \end{lstlisting}
>
> Stream sockets provide in-order, guaranteed, connection-oriented delivery
> without message boundaries. Seqpacket sockets provide in-order, guaranteed,
> -connection-oriented delivery with message and record boundaries.
> +connection-oriented delivery with message and record boundaries. Datagram
> +sockets provide connection-less, best-effort delivery of messages, with no
> +order or reliability guarantees.
>
> \subsubsection{Buffer Space Management}\label{sec:Device Types / Socket
> Device / Device Operation / Buffer Space Management}
> \field{buf_alloc} and \field{fwd_cnt} are used for buffer space management of
> @@ -203,16 +209,19 @@ \subsubsection{Buffer Space
> Management}\label{sec:Device Types / Socket Device /
> previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This allows
> communicating updates any time a change in buffer space occurs.
>
> +\field{buf_alloc} and \field{fwd_cnt} are reserved for future use by datagram
> +sockets. These fields are not used for datagram buffer space management.
no limits on datagram size?
also with no flow control at all there's a performance problem:
imagine queue gets full. now buffers begin to be dropped.
problem is, dropping is faster than delivering to application.
so now application sees its packets consumed super quickly
and starts sending even faster.
not good for performance.
yes datagram expects some way to drop packets but just disabling flow
control completely is not the right thing to do I think.
> +
> \drivernormative{\paragraph}{Device Operation: Buffer Space
> Management}{Device Types / Socket Device / Device Operation / Buffer Space
> Management}
> -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> -sufficient free buffer space for the payload.
> +For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets MUST only be
> +transmitted when the peer has sufficient free buffer space for the payload.
>
> All packets associated with a stream flow MUST contain valid information in
> \field{buf_alloc} and \field{fwd_cnt} fields.
>
> \devicenormative{\paragraph}{Device Operation: Buffer Space
> Management}{Device Types / Socket Device / Device Operation / Buffer Space
> Management}
> -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> -sufficient free buffer space for the payload.
> +For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets MUST only be
> +transmitted when the peer has sufficient free buffer space for the payload.
>
> All packets associated with a stream flow MUST contain valid information in
> \field{buf_alloc} and \field{fwd_cnt} fields.
> @@ -299,6 +308,78 @@ \subsubsection{Seqpacket Sockets}\label{sec:Device Types
[virtio-dev] RE: [virtio-comment] RE: RE: [PATCH v15] virtio-net: support device stats
> From: Xuan Zhuo
> Sent: Tuesday, August 29, 2023 12:48 PM
>
> For this line:
> "If the reply header structure and actual stats are split properly, it will
> be easy
> to add the query by the owner device."
>
> So for the header structure, is there some specific requirements?
TLV type, length, is only two fields needed and to align to 64-bit.
struct virtio_element_type {
u16 type;
u16 rsvd;
u32 length;
};
Above generic type if defined, that we can possibly use across many virtio
devices.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: RE: [PATCH v15] virtio-net: support device stats
> From: Xuan Zhuo
> 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_GSO3
> > > +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_BASIC 4
> > > +#define
