Re: [PATCH v10 3/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella <sgarzare@redhat.com>]

On Mon, Jan 10, 2022 at 11:19:27AM +0000, Stefan Hajnoczi wrote:
>On Wed, Jan 05, 2022 at 05:35:05PM +0100, Stefano Garzarella wrote:
>> From: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>>
>> This adds description of SOCK_SEQPACKET socket type
>> support for virtio-vsock.
>>
>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  virtio-vsock.tex | 39 +++++++++++++++++++++++++++++++++++----
>>  1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>> index bf015ac..9fa93f2 100644
>> --- a/virtio-vsock.tex
>> +++ b/virtio-vsock.tex
>> @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
>>
>>  \begin{description}
>>  \item VIRTIO_VSOCK_F_STREAM (0) stream socket type is supported.
>> +\item VIRTIO_VSOCK_F_SEQPACKET (1) seqpacket socket type is supported.
>>  \end{description}
>>
>>  \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
>> @@ -139,15 +140,17 @@ \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 provide in-order, guaranteed,
>> +connection-oriented delivery with message and record 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
>> @@ -248,6 +251,34 @@ \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 and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
>> +Two types of boundaries supported: message and record boundaries.
>
>s/boundaries supported/boundaries are supported/

Will fix.

>
>> +
>> +A message contains data sent by a single system call. Message boundary means
>> +that data of single send system call is guaranteed to be read wholly by single
>> +receive system call. If receive buffer is not enough, then out of size data
>> +will be dropped.
>
>The VIRTIO spec tends to focus on the hardware interface and avoids
>implementation details and application APIs like system calls. Maybe
>system calls are an important concept here but I think this could be
>rephrased in terms of "operations":
>
>  A message contains data sent in a single operation. ...

Okay, I agree to use "operations" instead of "system calls".

>
>The "Message boundary" sentence isn't clear to me. I guess it's saying
>that messages are never split into smaller pieces from the perspective
>of read operations? This seems like an implementation detail and the
>device would work fine if a different implementation supported read
>operations that split messages into multiple read buffers - it doesn't
>make a different from the hardware interface perspective.

Yes, I would like to remove this part and simply say that the message
can be split into multiple RW packets and rearrange the paragraph to
say right away that the last packet will have the EOM bit set.

>
>"If receive buffer is not enough, then out of size data ..." -> "If the
>receive buffer is not large enough, then data exceeding the buffer is
>dropped."
>
>I think this receive buffer refers to the read(2)/recv(2) syscall data
>buffer and not the virtio-vsock credit limit? So this statement relates
>to the behavior of SOCK_SEQPACKET, which is outside the scope of the
>VIRTIO spec (it's not part of the hardware interface and it's a
>Linux/POSIX-specific Sockets API feature). I think it can be dropped
>from the spec.

Yep, I'll drop it.


So the paragraph will be like this:

@@ -248,6 +251,27 @@ \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 and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
+Two types of boundaries are supported: message and record boundaries.
+
+A message contains data sent in a single operation. A single message can be
+split into multiple RW packets.
+To provide message boundaries, last RW packet of each message has
+VIRTIO_VSOCK_SEQ_EOM bit (bit 0) set in the \field{flags} of packet's header.
+
+Record is any number of subsequent messages, where last message is sent with POSIX
+MSG_EOR flag set. Record boundary means that receiver gets MSG_EOR flag set
+in the corresponding message where sender set it.
+To provide record boundaries, last RW packet of each record has VIRTIO_VSOCK_SEQ_EOR
+bit (bit 1) set in the \field{flags} of packet's header.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_SEQ_EOM (1 << 0)
+#define VIRTIO_VSOCK_SEQ_EOR (1 << 1)
+\end{lstlisting}
+
  \subsubsection{Device Events}\label{sec:Device Types / Socket Device / Device Operation / Device Events}

  Certain events are communicated by the device to the driver using the event

Thanks,
Stefano