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

[Arseny Krasnov <arseny.krasnov@kaspersky.com>]

On 13.04.2021 22:55, Michael S. Tsirkin wrote:
> 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?

I think  there is no practical sense in SOCK_STREAM bit, because SOCK_SEQPACKET

is stream + message boundaries and potential DGRAM is completely different

thing. Of course i can implement it in my patches and also add it to spec patch, but  i see only

esthetic in this: all three socket types have own feature bits.

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

Sorry, i don't understand. boundary is bit in flags of RW packet. These RW

packets are inserted to rx queue of socket, in the same way as for stream

socket. The only difference is that user is not woken up until boundary found.

All credit account logic works same in both stream and seqpacket.

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