On Tue, Mar 16, 2021 at 09:56:44PM +0000, jiang.wang wrote:
> Add supports for datagram type for virtio-vsock. Datagram
> sockets are connectionless and unreliable. To avoid contention
> with stream and other sockets, add two more virtqueues and
> a new feature bit to identify if those two new queues exist or not.
> 
> Also add descriptions for resouce management of datagram, which
> does not use the existing credit update mechanism associated with
> stream sockets.
> 
> Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
> ---
>  virtio-vsock.tex | 72 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 62 insertions(+), 10 deletions(-)
> 
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index da7e641..a2ff0a4 100644
> --- a/virtio-vsock.tex
> +++ b/virtio-vsock.tex
> @@ -9,14 +9,27 @@ \subsection{Device ID}\label{sec:Device Types / Socket Device / Device ID}
>  
>  \subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
>  \begin{description}
> +\item[0] stream rx
> +\item[1] stream tx
> +\item[2] dgram rx
> +\item[3] dgram tx
> +\item[4] event
> +\end{description}
> +The virtio socket device uses 5 queues if feature bit VIRTIO_VSOCK_DRGAM is set. Otherwise, it
> +only uses 3 queues, as the following.
> +
> +\begin{description}
>  \item[0] rx
>  \item[1] tx
>  \item[2] event
>  \end{description}
>  
> +
>  \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
>  
> -There are currently no feature bits defined for this device.
> +\begin{description}
> +\item[VIRTIO_VSOCK_DGRAM (0)] Device has support for datagram sockets type.
> +\end{description}

By convention feature bits are named VIRTIO_<device>_F_<feature>. Please
add the "_F_".

>  
>  \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
>  
> @@ -76,6 +89,7 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
>  	le32 flags;
>  	le32 buf_alloc;
>  	le32 fwd_cnt;
> +	le64 timestamp;

Adding this field breaks old devices and drivers. Please make this field
conditional on the dgram socket type or the VIRTIO_VSOCK_F_DGRAM feature
bit.

Also, are all the other fields still used with dgram sockets? Maybe you
can use a union instead to reuse some space?

>  };
>  \end{lstlisting}
>  
> @@ -121,11 +135,14 @@ \subsubsection{Virtqueue Flow Control}\label{sec:Device Types / Socket Device /
>  packets.  With additional resources, it becomes possible to process incoming
>  packets even when outgoing packets cannot be sent.
>  
> -Eventually even the additional resources will be exhausted and further
> +For stream type, eventually even the additional resources will be exhausted and further
>  processing is not possible until the other side processes the virtqueue that
>  it has neglected.  This stop to processing prevents one side from causing
>  unbounded resource consumption in the other side.
>  
> +For datagram type, the additional resources have a fixed size limit to prevent
> +unbounded resource consumption.
> +
>  \drivernormative{\paragraph}{Device Operation: Virtqueue Flow Control}{Device Types / Socket Device / Device Operation / Virtqueue Flow Control}
>  
>  The rx virtqueue MUST be processed even when the tx virtqueue is full so long as there are additional resources available to hold packets outside the tx virtqueue.
> @@ -140,12 +157,15 @@ \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 only stream sockets are supported. \field{type} is 1 for stream
> -socket types.
> +Currently stream and datagram (dgram) sockets are supported. \field{type} is 1 for stream
> +socket types. \field{type} is 3 for dgram socket types.
>  
>  Stream sockets provide in-order, guaranteed, connection-oriented delivery
>  without message boundaries.
>  
> +Datagram socekts provide connectionless unreliable messages of

s/socekts/sockets/

> +a fixed maximum length.
> +
>  \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
>  stream sockets. The guest and the device publish how much buffer space is
> @@ -170,20 +190,41 @@ \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.
>  
> +For datagram sockets, \field{buf_alloc} is also used on the rx side. Unlike stream 
> +sockets, dgram sockets do not use VIRTIO_VSOCK_OP_CREDIT_UPDATE or 
> +VIRTIO_VSOCK_OP_CREDIT_REQUEST packets. The dgram sender does not check if
> +the \field{buf_alloc} of the peer is full or not. If it is full, the packet
> +will be dropped. To limit resouce usage of the sender, \field{dgram_tx_bytes}

s/resouce/resource/

\field{dgram_tx_bytes} is not included in any struct definition?

> +is used for each VM and host. Only payload bytes are counted and header bytes are not

Please use the VIRTIO specification terms "driver" and "device" instead
of VM and host.

It's not clear to me what "used" means. What exactly do the driver and
device need to do?

> +included. If \field{dgram_tx_bytes} is equal to VIRTIO_VSOCK_MAX_DGRAM_BUF_SIZE,
> +the send will fail.

Does the device reject dgram packets or does the driver refuse to send
more dgram packets?

> +
> +For dgram, since \field{dgram_tx_byte} is shared within a VM or host, a tx_timeout and
> +a timer are used for each outgoing packet. If a packet is not delivered within
> +tx_timeout, it will be dropped to make free space for other dgram sockets.

What does this mean? Does the timeout cause the device to drop packets
that haven't been handled yet (e.g. read by a host userspace process)?

POSIX Sockets have a receive socket buffer (rcvbuf) that is used for
memory accounting and dropping packets. Operating systems implementing
POSIX Sockets would use that to decide when incoming packets are
dropped. It seems like dgram_tx_byte does something similar at the
device level and I'm not sure why it's necessary?

In the POSIX Sockets model the virtio-vsock tx virtqueue is processed by
the device and packets are read into socket rcvbuf. They do not stay in
the virtqueue. This involves an extra data copy but it keeps the
virtqueue as empty as possible so that further communication is
possible.

> +
>  \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 sockets, VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> +sufficient free buffer space for the payload. For dgram sockets, VIRTIO_VSOCK_OP_RW data packets
> +can be transmitted when the peer buffer is full. Then the pacekt will be dropped.

s/can/MAY/

s/pacekt/packet/

"Then the packet will be dropped" is not clear to me. Is it saying the
device drops packets when buffer space has exceeded?

>  
>  All packets associated with a stream flow MUST contain valid information in
>  \field{buf_alloc} and \field{fwd_cnt} fields.
>  
> +All packets associated with a dgram flow MUST contain valid information in
> +\field{timestamp} field, which will be used to check for tx timeout.

What are the units of the timestamp field?

> +
>  \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 sockets, VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> +sufficient free buffer space for the payload. For dgram sockets, VIRTIO_VSOCK_OP_RW data packets
> +can be transmitted when the peer buffer is full. Then the pacekt will be dropped.
>  
>  All packets associated with a stream flow MUST contain valid information in
>  \field{buf_alloc} and \field{fwd_cnt} fields.
>  
> +All packets associated with a dgram flow MUST contain valid information in
> +\field{timestamp} field.
> +
>  \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / Device Operation / Receive and Transmit}
>  The driver queues outgoing packets on the tx virtqueue and incoming packet
>  receive buffers on the rx virtqueue. Packets are of the following form:
> @@ -203,14 +244,14 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De
>  The \field{guest_cid} configuration field MUST be used as the source CID when
>  sending outgoing packets.
>  
> -A VIRTIO_VSOCK_OP_RST reply MUST be sent if a packet is received with an
> +For stream socekts, A VIRTIO_VSOCK_OP_RST reply MUST be sent if a packet is received with an
>  unknown \field{type} value.
>  
>  \devicenormative{\paragraph}{Device Operation: Receive and Transmit}{Device Types / Socket Device / Device Operation / Receive and Transmit}
>  
>  The \field{guest_cid} configuration field MUST NOT contain a reserved CID as listed in \ref{sec:Device Types / Socket Device / Device configuration layout}.
>  
> -A VIRTIO_VSOCK_OP_RST reply MUST be sent if a packet is received with an
> +For stream sockets, A VIRTIO_VSOCK_OP_RST reply MUST be sent if a packet is received with an
>  unknown \field{type} value.
>  
>  \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device Operation / Stream Sockets}
> @@ -240,6 +281,17 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
>  destination) address tuple for a new connection while the other peer is still
>  processing the old connection.
>  
> +\subsubsection{Datagram Sockets}\label{sec:Device Types / Socket Device / Device Operation / Stream Sockets}
> +
> +Datagram (dgram) sockets are connectionless and unreliable. The sender just send 
> +a message to the peer and hope it will be delivered. If there is no socket binds the 
> +address on the other end, or the transmision or receving buffers are full, the packets 
> +will be droped. Each packet may have a source cid and 

s/dropped/

Please stick to UDP semantics as much as possible so that applications
can be ported easily and developers aren't surprised by unexpected
behavior. UDP packets sent to a destination that has no listen socket
result in a Connection Refused error. vsock dgrams should behave in the
same way.

> +source port, the receiver can use them to send back a reply message.

The VIRTIO specification avoids using the word "may" (as well as "must"
and "should") outside the normative sections of the specification.

It's unclear what this sentence means: can the driver set the source cid
and source port to zero or an arbitary number if it does not need a
reply? (I guess the answer is "no" but the sentence implies setting the
source cid and source port is optional.)

> +
> +Dgram sockets preserve the message boundaries. A message is either sent or dropped.

What does "a message is either sent or dropped" mean? Does it mean
messages are delivered once or not at all, but they are never
duplicated?