Stefan Hajnoczi
2021-Mar-29 16:11 UTC
[virtio-comment] [RFC PATCH v4 2/2] virtio-vsock: SOCK_SEQPACKET description
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 at 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 9The 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: ins/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 carrys/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 mores/MUST be/is/ s/starting from/starting at/> +data is allowed to be beyond it. Also payload of such packet MUST be transmitteds/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 ofThe 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 ofs/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 MUSTs/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 datas/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 prependings/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? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210329/4a4e3a84/attachment.sig>