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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 13, 2021 at 05:22:44PM +0300, Arseny Krasnov wrote:
> 
> On 13.04.2021 16:10, Michael S. Tsirkin wrote:
> > On Tue, Apr 13, 2021 at 03:53:29PM +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 | 26 +++++++++++++++++++++-----
> >>  1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> >> index ad57f9d..00e59cc 100644
> >> --- a/virtio-vsock.tex
> >> +++ b/virtio-vsock.tex
> >> @@ -16,7 +16,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.
> >> +\begin{description}
> >> +\item VIRTIO_VSOCK_F_SEQPACKET (0) SOCK_SEQPACKET socket type is
> >> +    supported.
> > Does it make sense to only support seqpacket and not stream?
> > I am guessing not since seqpacket is more or less
> > a superset ...
> 
> You mean, this sentence must be "Both SOCK_SEQPACKET and SOCK_STREAM types
> 
> are supported"?


No. I am asking whether we want a feature bit for SOCK_STREAM too?


> >
> >> +\end{description}
> >>  
> >>  \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
> >>  
> >> @@ -135,15 +138,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 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
> >> @@ -244,6 +249,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{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}
> >> +
> >> +To provide message boundaries, last RW packet of each message has VIRTIO_VSOCK_SEQ_EOR
> >> +bit set in the \field{flags} of packet's header.
> >> +
> >> +\begin{lstlisting}
> >> +#define VIRTIO_VSOCK_SEQ_EOR 1
> >> +\end{lstlisting}
> >
> > Problem is, where is device going to hold this boundary?
> > Currently device discards the header just keeping the payload
> > around.
> > I suspect we need to have some kind of header per message
> > maintained by the device and
> > accounted for as part of the the credit accounting.
> 
> IIUC, in Linux, header's are copied by vhost between buffer
> 
> in virtqueue and buffer in virtio vsock driver(host to guest transmission and
> 
> guest to host transmission). E.g. values of header's field are visible by driver.

The question is about credit accounting.  With stream we can add as
little as a single byte with no overhead. No so with seqpacket each
boundary adds memory overhead.  This memory needs to be accounted for.
How much I'm not sure. Maybe it needs to be negotiated during
connection?



> >
> >
> >> +
> >>  \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
> >> -- 
> >> 2.25.1
> >