Re: [virtio-comment] [RFC PATCH v4 2/2] virtio-vsock: SOCK_SEQPACKET description

[Stefan Hajnoczi <stefanha@redhat.com>]

On Fri, Mar 26, 2021 at 12:02:50PM +0300, Arseny Krasnov wrote:
> This adds description of SOCK_SEQPACKET socket type
> support for virtio-vsock.
> 
> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> ---
>  virtio-vsock.tex | 65 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index ad57f9d..c366de7 100644
> --- a/virtio-vsock.tex
> +++ b/virtio-vsock.tex
> @@ -17,6 +17,10 @@ \subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
>  \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
>  
>  There are currently no feature bits defined for this device.

^ This line is now out of date :)

> +\begin{description}
> +\item VIRTIO_VSOCK_F_SEQPACKET (0) SOCK_SEQPACKET socket type is
> +    supported.
> +\end{description}
>  
>  \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
>  
> @@ -98,6 +102,10 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
>  #define VIRTIO_VSOCK_OP_CREDIT_UPDATE  6
>  /* Request the peer to send the credit info to us */
>  #define VIRTIO_VSOCK_OP_CREDIT_REQUEST 7
> +/* Message begin for SOCK_SEQPACKET */
> +#define VIRTIO_VSOCK_OP_SEQ_BEGIN      8
> +/* Message end for SOCK_SEQPACKET */
> +#define VIRTIO_VSOCK_OP_SEQ_END        9

The struct virtio_vsock_hdr->flags field is le32 and currently unused.
Could 24 bits be used for a unique message id and 8 bits for flags? 1
flag bit could be used for end-of-message and the remaining 7 bits could
be reserved. That way SEQ_BEGIN and SEQ_END are not necessary. Pressure
on the virtqueue would be reduced and performance should be comparable
to SOCK_STREAM.

>  \end{lstlisting}
>  
>  \subsubsection{Virtqueue Flow Control}\label{sec:Device Types / Socket Device / Device Operation / Virtqueue Flow Control}
> @@ -135,15 +143,16 @@ \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 (VIRTIO_VSOCK_TYPE_STREAM)
> -for stream socket types.
> +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.
>  
>  \begin{lstlisting}
> -#define VIRTIO_VSOCK_TYPE_STREAM 1
> +#define VIRTIO_VSOCK_TYPE_STREAM    1
> +#define VIRTIO_VSOCK_TYPE_SEQPACKET 2
>  \end{lstlisting}
>  
>  Stream sockets provide in-order, guaranteed, connection-oriented delivery
> -without message boundaries.
> +without message boundaries. Seqpacket sockets also provide message boundaries.

"also" is ambiguous, I guess it means "is the same except ..." here. I
suggest writing out the characteristics of seqpacket sockets in full:

  Seqpacket sockets provide in-order, guaranteed, connection-oriented
  delivery with message boundaries.

If "also" is intended to mean that message boundaries are optional, then
my understanding is that they are mandatory, not optional. Seqpacket
sockets deliver entire messages and therefore communication is
impossible without message boundaries.

>  
>  \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
> @@ -170,8 +179,10 @@ \subsubsection{Buffer Space Management}\label{sec:Device Types / Socket Device /
>  communicating updates any time a change in buffer space occurs.
>  
>  \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.
> +VIRTIO_VSOCK_OP_RW data packets as well as VIRTIO_VSOCK_OP_SEQ_BEGIN and
> +VIRTIO_VSOCK_OP_SEQ_END message boundary 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.
> @@ -244,6 +255,48 @@ \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{Seqpacket Sockets}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets}
> +
> +\paragraph{Message boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Message boundaries}
> +
> +Seqpacket sockets differ from stream sockets only in data transmission way: in

s/in data transmission way/in how data is transmitted/

> +stream sockets all data is sent using only VIRTIO_VSOCK_OP_RW packets. In
> +seqpacket sockets, to provide message boundaries, every sequence of
> +VIRTIO_VSOCK_OP_RW packets of each message MUST be headed with
> +VIRTIO_VSOCK_OP_SEQ_BEGIN and tailed with VIRTIO_VSOCK_OP_SEQ_END packets.
> +Both VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END packets MUST carry

s/MUST//

MUST/MAY/CAN/SHOULD/etc are only used in normative sections of the spec.

> +special structure in payload:

s/carry special structure in payload/carry the following payload/

> +
> +\begin{lstlisting}
> +struct virtio_vsock_seq_hdr {
> +	__le32  msg_id;
> +	__le32  msg_len;
> +};
> +\end{lstlisting}
> +
> +This data MUST be placed starting from the first byte of payload and no more

s/MUST be/is/

s/starting from/starting at/

> +data is allowed to be beyond it. Also payload of such packet MUST be transmitted

s/to be//

s/MUST be/is/

Which "payload" is this sentence referring to? The previous sentence
referred to the payload of
VIRTIO_VSOCK_OP_SEQ_BEGIN/VIRTIO_VSOCK_OP_SEQ_END packets, but here I'm
not sure if the text is referring to VIRTIO_VSOCK_OP_RW packets.

> +without fragmentation. \field{len} of packet header MUST be set to the size of

The word "fragmentation" is not used in the Socket device section and
it's not clear to me what it means. It seems like SEQ_BEGIN is followed
by one or more RW packets and then SEQ_END. But does splitting a message
across multiple RW packets count as "fragmentation"?

s/\field{len} of packet header/The packet header \field{len} field/

s/MUST be/is/

> +the struct virtio_vsock_seq_hdr.
> +
> +\field{msg_id} is value to identify message. It MUST be same for pair of

s/is value to identify message/is a unique valid to identify a message/

s/MUST be/is the/

s/for pair/for a pair/

> +VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END of one message, and MUST

s/of one message/packets related to one message/

> +differ in next messages. \field{msg_len} contains message length. This header

"MUST differ in next messages" can be removed here. If we describe
msg_id as "unique" above the the intent is clear and sentence can be
added to the driver normative section indicating that all msg_id values
in use at a given time MUST be unique.

s/contains message length/contains the message length/

> +is used to check integrity of each message: message is valid if length of data

s/check integrity/check the integrity/

s/if length/if the total length/

(I suggest adding "total" to make it clear that the lengths of all RW
packets is summed. This rules out the interpretation that each RW
packet's length must be msg_len.)

> +in VIRTIO_VSOCK_OP_RW packets is equal to \field{msg_len} of prepending

s/in VIRTIO_VSOCK_OP_RW packets/in the VIRTIO_VSOCK_OP_RW packets/

s/prepending/the corresponding/

> +VIRTIO_VSOCK_OP_SEQ_BEGIN and \field{msg_id} of VIRTIO_VSOCK_OP_SEQ_END is equal
> +to \field{msg_id} of VIRTIO_VSOCK_OP_SEQ_BEGIN.
> +
> +\paragraph{POSIX MSG_EOR flag}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / MSG_EOR flag}
> +
> +When message is sent using one of POSIX functions: send(), sendto() or sendmsg() and
> +MSG_EOR flag is set in \field{flags} parameter of these functions, then all VIRTIO_VSOCK_OP_RW
> +packets of this message MUST have \field{flags} field in header set to special constanst:
> +
> +\begin{lstlisting}
> +#define VIRTIO_VSOCK_RW_EOR 1
> +\end{lstlisting}

I'm not familiar with MSG_EOR. It seems to be a hint to send buffered
data with Linux TCP sockets and it's not implemented for Linux AF_UNIX
sockets.

How is MSG_EOR useful if SEQ_END already deliminates message boundaries?

It seems like passing the MSG_EOR flag in this way provides an
additional layer on top of message boundaries? It's not clear to me that
MSG_EOR has to work like that according to POSIX. Do you have a link to
the POSIX spec pages that describe how MSG_EOR works?

Attachment: signature.asc
Description: PGP signature