[PATCH v12 0/3] virtio-vsock: SOCK_SEQPACKET description

[PATCH v12 0/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella]

v12:
- added statement about supporting F_STREAM when F_SEQPACKET is negotiated
  [cohuck, mst]

v11: https://lists.oasis-open.org/archives/virtio-comment/202201/msg00027.html
- reworked "Message and record boundaries" paragraph [stefanha]

Linux kernel and QEMU already merged SOCK_SEQPACKET support,
so I'm resending Arseny's patches to have consistent virtio-spec
and implementation.

I added patch 2, following the discussion about F_STREAM feature bit:
https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results

Thanks,
Stefano

Arseny Krasnov (2):
  virtio-vsock: use C style defines for constants
  virtio-vsock: SOCK_SEQPACKET description

Stefano Garzarella (1):
  virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit

 virtio-vsock.tex | 88 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH v12 1/3] virtio-vsock: use C style defines for constants

[Stefano Garzarella]

From: Arseny Krasnov <arseny.krasnov@kaspersky.com>

This:
1) Replaces enums with C style "defines", because
   use of enums is not documented, while "defines"
   are widely used in spec.
2) Adds defines for some constants.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 virtio-vsock.tex | 54 +++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index da7e641..ad57f9d 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -86,23 +86,18 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
 operation constants:
 
 \begin{lstlisting}
-enum {
-	VIRTIO_VSOCK_OP_INVALID = 0,
-
-	/* Connect operations */
-	VIRTIO_VSOCK_OP_REQUEST = 1,
-	VIRTIO_VSOCK_OP_RESPONSE = 2,
-	VIRTIO_VSOCK_OP_RST = 3,
-	VIRTIO_VSOCK_OP_SHUTDOWN = 4,
-
-	/* To send payload */
-	VIRTIO_VSOCK_OP_RW = 5,
-
-	/* Tell the peer our credit info */
-	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
-	/* Request the peer to send the credit info to us */
-	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
-};
+#define VIRTIO_VSOCK_OP_INVALID        0
+/* Connect operations */
+#define VIRTIO_VSOCK_OP_REQUEST        1
+#define VIRTIO_VSOCK_OP_RESPONSE       2
+#define VIRTIO_VSOCK_OP_RST            3
+#define VIRTIO_VSOCK_OP_SHUTDOWN       4
+/* To send payload */
+#define VIRTIO_VSOCK_OP_RW             5
+/* Tell the peer our credit info */
+#define VIRTIO_VSOCK_OP_CREDIT_UPDATE  6
+/* Request the peer to send the credit info to us */
+#define VIRTIO_VSOCK_OP_CREDIT_REQUEST 7
 \end{lstlisting}
 
 \subsubsection{Virtqueue Flow Control}\label{sec:Device Types / Socket Device / Device Operation / Virtqueue Flow Control}
@@ -140,8 +135,12 @@ \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 for stream
-socket types.
+Currently only stream sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
+for stream socket types.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_TYPE_STREAM 1
+\end{lstlisting}
 
 Stream sockets provide in-order, guaranteed, connection-oriented delivery
 without message boundaries.
@@ -222,10 +221,15 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
 insufficient resources to establish the connection.
 
 When a connected socket receives VIRTIO_VSOCK_OP_SHUTDOWN the header
-\field{flags} field bit 0 indicates that the peer will not receive any more
-data and bit 1 indicates that the peer will not send any more data.  These
-hints are permanent once sent and successive packets with bits clear do not
-reset them.
+\field{flags} field bit VIRTIO_VSOCK_SHUTDOWN_F_RECEIVE (bit 0) set indicates
+that the peer will not receive any more data and bit VIRTIO_VSOCK_SHUTDOWN_F_SEND
+(bit 1) set indicates that the peer will not send any more data.  These hints are
+permanent once sent and successive packets with bits clear do not reset them.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_SHUTDOWN_F_RECEIVE 0
+#define VIRTIO_VSOCK_SHUTDOWN_F_SEND    1
+\end{lstlisting}
 
 The VIRTIO_VSOCK_OP_RST packet aborts the connection process or forcibly
 disconnects a connected socket.
@@ -248,9 +252,7 @@ \subsubsection{Device Events}\label{sec:Device Types / Socket Device / Device Op
 The event buffer is as follows:
 
 \begin{lstlisting}
-enum virtio_vsock_event_id {
-        VIRTIO_VSOCK_EVENT_TRANSPORT_RESET = 0,
-};
+#define VIRTIO_VSOCK_EVENT_TRANSPORT_RESET 0
 
 struct virtio_vsock_event {
         le32 id;
-- 
2.31.1

[virtio-comment] [PATCH v12 2/3] virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit

[Stefano Garzarella]

Initially vsock devices only supported stream sockets, but now
we are adding support for new types (i.e. SEQPACKET, DGRAM).

Since some devices may not want to support stream sockets, we add
a feature bit for this type.

For backward compatibility, if no feature bit is set, only stream
socket type is supported.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 virtio-vsock.tex | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index ad57f9d..23a1eeb 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -16,7 +16,11 @@ \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.
+If no feature bit is set, only stream socket type is supported.
+
+\begin{description}
+\item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
+\end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
 
-- 
2.31.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[PATCH v12 3/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella]

From: Arseny Krasnov <arseny.krasnov@kaspersky.com>

This adds description of SOCK_SEQPACKET socket type
support for virtio-vsock.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/132
Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
[reworked "Message and record boundaries" paragraph]
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v12:
 - added statement about supporting F_STREAM when F_SEQPACKET is negotiated
   [cohuck, mst]
---
 virtio-vsock.tex | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index 23a1eeb..d79984d 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -17,9 +17,12 @@ \subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
 \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
 
 If no feature bit is set, only stream socket type is supported.
+If VIRTIO_VSOCK_F_SEQPACKET has been negotiated, the device MAY act
+as if VIRTIO_VSOCK_F_STREAM has also been negotiated.
 
 \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 +142,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 +253,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
-- 
2.31.1

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

[Stefano Garzarella]

Cornelia, Michael,
if you are happy with this version, can we start the vote to have these 
changes in virtio-spec 1.2?

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/132

Thanks,
Stefano

On Thu, Jan 13, 2022 at 05:51:59PM +0100, Stefano Garzarella wrote:
>v12:
>- added statement about supporting F_STREAM when F_SEQPACKET is negotiated
>  [cohuck, mst]
>
>v11: https://lists.oasis-open.org/archives/virtio-comment/202201/msg00027.html
>- reworked "Message and record boundaries" paragraph [stefanha]
>
>Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>so I'm resending Arseny's patches to have consistent virtio-spec
>and implementation.
>
>I added patch 2, following the discussion about F_STREAM feature bit:
>https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>
>Thanks,
>Stefano
>
>Arseny Krasnov (2):
>  virtio-vsock: use C style defines for constants
>  virtio-vsock: SOCK_SEQPACKET description
>
>Stefano Garzarella (1):
>  virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit
>
> virtio-vsock.tex | 88 +++++++++++++++++++++++++++++++++---------------
> 1 file changed, 60 insertions(+), 28 deletions(-)
>
>-- 
>2.31.1
>

[virtio-comment] Re: [PATCH v12 0/3] virtio-vsock: SOCK_SEQPACKET description

[Cornelia Huck]

On Fri, Jan 14 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:

> Cornelia, Michael,
> if you are happy with this version, can we start the vote to have these 
> changes in virtio-spec 1.2?

Looks good to me. Unless there are objections, I'll start the vote later
today.

>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/132
>
> Thanks,
> Stefano
>
> On Thu, Jan 13, 2022 at 05:51:59PM +0100, Stefano Garzarella wrote:
>>v12:
>>- added statement about supporting F_STREAM when F_SEQPACKET is negotiated
>>  [cohuck, mst]
>>
>>v11: https://lists.oasis-open.org/archives/virtio-comment/202201/msg00027.html
>>- reworked "Message and record boundaries" paragraph [stefanha]
>>
>>Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>>so I'm resending Arseny's patches to have consistent virtio-spec
>>and implementation.
>>
>>I added patch 2, following the discussion about F_STREAM feature bit:
>>https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>>
>>Thanks,
>>Stefano
>>
>>Arseny Krasnov (2):
>>  virtio-vsock: use C style defines for constants
>>  virtio-vsock: SOCK_SEQPACKET description
>>
>>Stefano Garzarella (1):
>>  virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit
>>
>> virtio-vsock.tex | 88 +++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 60 insertions(+), 28 deletions(-)
>>
>>-- 
>>2.31.1
>>


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 05:51:59PM +0100, Stefano Garzarella wrote:
> v12:
> - added statement about supporting F_STREAM when F_SEQPACKET is negotiated
>   [cohuck, mst]
> 
> v11: https://lists.oasis-open.org/archives/virtio-comment/202201/msg00027.html
> - reworked "Message and record boundaries" paragraph [stefanha]
> 
> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> so I'm resending Arseny's patches to have consistent virtio-spec
> and implementation.
> 
> I added patch 2, following the discussion about F_STREAM feature bit:
> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
> 
> Thanks,
> Stefano

Was going to vote on this and was reviewing for the last time, when I
detected a problem with SEQPACKET.

Specifically, with STREAM for flow control management purposes we only
count payload bytes since it is always possible to copy all data into a
single buffer.

Not so with SEQPACKET where in the worst case it is possible to have
single byte messages, each consuming multiple bytes of meta-data to
track message boundaries.  This does work with the current proposal
simply by publishing a smaller buffer to the other side, e.g. with a 64
byte header we'd publish a 1K buffer and in practice it will occupy up
to 65K. Tolerable, and there's nothing new here. OK.


However, just today I noticed this in the unix man page for SEQPACKET:

 SOCK_SEQPACKET
              Provides a sequenced, reliable, two-way connection-based
              data transmission path for datagrams of fixed maximum
              length; 


The point here being that it needs to be limited so userspace knows how
large is a message it can send - since dropping messages is not allowed
this has to happen upfront.
And I noticed that our text does not mention the maximum length.
So I asked myself what is the maximum length for vsock.

After some poking around I realized that the largest message we can
accept has to be capped to buf_alloc. However this makes the buffer size
visible to userspace and so conflicts with the idea of limiting
it for memory management purposes as described above.

So it looks like we need to add a seqpacket message header overhead
field in the config space, and if present take that into account.

I am not sure what to do about it at this point.
Together with guests assuming this implies stream this looks a bit much.

Sorry that I just noticed this part now, I guess better late than never.
Given it's deployed anyway, I guess we can put it in the spec as is,
however in that case I guess we should just document what's there,
maybe add some text explaining that it will be superceded
in a future version. But that *also* will mean we should
make it imply STREAM support since that is what happens in
the field. Maybe rename this to VIRTIO_VSOCK_F_SEQPACKET_COMPAT
or something like this.

Thoughts?


> Arseny Krasnov (2):
>   virtio-vsock: use C style defines for constants
>   virtio-vsock: SOCK_SEQPACKET description
> 
> Stefano Garzarella (1):
>   virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit
> 
>  virtio-vsock.tex | 88 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 28 deletions(-)
> 
> -- 
> 2.31.1

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

[Stefano Garzarella]

On Fri, Jan 14, 2022 at 6:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jan 13, 2022 at 05:51:59PM +0100, Stefano Garzarella wrote:
> > v12:
> > - added statement about supporting F_STREAM when F_SEQPACKET is negotiated
> >   [cohuck, mst]
> >
> > v11: https://lists.oasis-open.org/archives/virtio-comment/202201/msg00027.html
> > - reworked "Message and record boundaries" paragraph [stefanha]
> >
> > Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> > so I'm resending Arseny's patches to have consistent virtio-spec
> > and implementation.
> >
> > I added patch 2, following the discussion about F_STREAM feature bit:
> > https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
> >
> > Thanks,
> > Stefano
>
> Was going to vote on this and was reviewing for the last time, when I
> detected a problem with SEQPACKET.
>
> Specifically, with STREAM for flow control management purposes we only
> count payload bytes since it is always possible to copy all data into a
> single buffer.
>
> Not so with SEQPACKET where in the worst case it is possible to have
> single byte messages, each consuming multiple bytes of meta-data to
> track message boundaries.  This does work with the current proposal
> simply by publishing a smaller buffer to the other side, e.g. with a 64
> byte header we'd publish a 1K buffer and in practice it will occupy up
> to 65K. Tolerable, and there's nothing new here. OK.
>
>
> However, just today I noticed this in the unix man page for SEQPACKET:
>
>  SOCK_SEQPACKET
>               Provides a sequenced, reliable, two-way connection-based
>               data transmission path for datagrams of fixed maximum
>               length;
>
>
> The point here being that it needs to be limited so userspace knows how
> large is a message it can send - since dropping messages is not allowed
> this has to happen upfront.
> And I noticed that our text does not mention the maximum length.
> So I asked myself what is the maximum length for vsock.

@Arseny feel free to correct me, but IIRC we had a discussion about it.  
I can't find the thread, but I remember Arseny found something different 
in the POSIX spec.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/socket.html

SOCK_SEQPACKET
    Provides sequenced, reliable, bidirectional, connection-mode 
    transmission paths for records. A record can be sent using one or 
    more output operations and received using one or more input 
    operations, but a single operation never transfers part of more than 
    one record. Record boundaries are visible to the receiver via the 
    MSG_EOR flag.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/recvmsg.html

    The recvmsg() function shall return the total length of the message.  
    For message-based sockets, such as SOCK_DGRAM and SOCK_SEQPACKET, 
    the entire message shall be read in a single operation. If a message 
    is too long to fit in the supplied buffers, and MSG_PEEK is not set 
    in the flags argument, the excess bytes shall be discarded, and 
    MSG_TRUNC shall be set in the msg_flags member of the msghdr 
    structure. For stream-based sockets, such as SOCK_STREAM, message 
    boundaries shall be ignored. In this case, data shall be returned to 
    the user as soon as it becomes available, and no data shall be 
    discarded.

From this description it seems that SEQPACKET has no fixed size and 
receiver if it is not sure that its buffer is big enough, it can use 
MSG_PEEK. However MSG_TRUNC would allow to take only a part of it.

>
> After some poking around I realized that the largest message we can
> accept has to be capped to buf_alloc. However this makes the buffer size
> visible to userspace and so conflicts with the idea of limiting
> it for memory management purposes as described above.

Yep, I think this is the real limit and IIRC the current implementation 
checks how much space is available in the other peer before queuing the 
packet, if there is no space it returns an error to the user.

The user can control it (also for STREAM) through the 
SO_VM_SOCKETS_BUFFER_SIZE sockopt.
(there is a maximum allowed, but it seems to high... I'll check it)

>
> So it looks like we need to add a seqpacket message header overhead
> field in the config space, and if present take that into account.

Yes, and we should do that with stream as well when we don't merge 
packets into a single buffer, there's a bug reported about that that we 
need to fix sooner or later:
    https://bugzilla.kernel.org/show_bug.cgi?id=215329

>
> I am not sure what to do about it at this point.
> Together with guests assuming this implies stream this looks a bit much.
>
> Sorry that I just noticed this part now, I guess better late than never.

Sure :-)

> Given it's deployed anyway, I guess we can put it in the spec as is,
> however in that case I guess we should just document what's there,
> maybe add some text explaining that it will be superceded
> in a future version. But that *also* will mean we should
> make it imply STREAM support since that is what happens in
> the field. Maybe rename this to VIRTIO_VSOCK_F_SEQPACKET_COMPAT
> or something like this.
>
> Thoughts?

Aside from the issue of accounting for the header in memory consumption, 
I think the rest is fine, so maybe we can leave it that way and in the 
future add header overhead field in the config space like you said.

Stefano

[virtio-comment] Re: [PATCH v12 0/3] virtio-vsock: SOCK_SEQPACKET description

[Arseny Krasnov]

On 17.01.2022 12:33, Stefano Garzarella wrote:
>
> On Fri, Jan 14, 2022 at 6:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Thu, Jan 13, 2022 at 05:51:59PM +0100, Stefano Garzarella wrote:
>>> v12:
>>> - added statement about supporting F_STREAM when F_SEQPACKET is negotiated
>>>   [cohuck, mst]
>>>
>>> v11: https://lists.oasis-open.org/archives/virtio-comment/202201/msg00027.html
>>> - reworked "Message and record boundaries" paragraph [stefanha]
>>>
>>> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>>> so I'm resending Arseny's patches to have consistent virtio-spec
>>> and implementation.
>>>
>>> I added patch 2, following the discussion about F_STREAM feature bit:
>>> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>>>
>>> Thanks,
>>> Stefano
>> Was going to vote on this and was reviewing for the last time, when I
>> detected a problem with SEQPACKET.
>>
>> Specifically, with STREAM for flow control management purposes we only
>> count payload bytes since it is always possible to copy all data into a
>> single buffer.
>>
>> Not so with SEQPACKET where in the worst case it is possible to have
>> single byte messages, each consuming multiple bytes of meta-data to
>> track message boundaries.  This does work with the current proposal
>> simply by publishing a smaller buffer to the other side, e.g. with a 64
>> byte header we'd publish a 1K buffer and in practice it will occupy up
>> to 65K. Tolerable, and there's nothing new here. OK.
>>
>>
>> However, just today I noticed this in the unix man page for SEQPACKET:
>>
>>  SOCK_SEQPACKET
>>               Provides a sequenced, reliable, two-way connection-based
>>               data transmission path for datagrams of fixed maximum
>>               length;
>>
>>
>> The point here being that it needs to be limited so userspace knows how
>> large is a message it can send - since dropping messages is not allowed
>> this has to happen upfront.
>> And I noticed that our text does not mention the maximum length.
>> So I asked myself what is the maximum length for vsock.
> @Arseny feel free to correct me, but IIRC we had a discussion about it.  
> I can't find the thread, but I remember Arseny found something different 
> in the POSIX spec.

Yes, i used POSIX spec to implement SEQPACKET support, also things from POSIX

make me to add MSG_EOR support, IIRC

>
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/socket.html
>
> SOCK_SEQPACKET
>     Provides sequenced, reliable, bidirectional, connection-mode 
>     transmission paths for records. A record can be sent using one or 
>     more output operations and received using one or more input 
>     operations, but a single operation never transfers part of more than 
>     one record. Record boundaries are visible to the receiver via the 
>     MSG_EOR flag.
>
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/recvmsg.html
>
>     The recvmsg() function shall return the total length of the message.  
>     For message-based sockets, such as SOCK_DGRAM and SOCK_SEQPACKET, 
>     the entire message shall be read in a single operation. If a message 
>     is too long to fit in the supplied buffers, and MSG_PEEK is not set 
>     in the flags argument, the excess bytes shall be discarded, and 
>     MSG_TRUNC shall be set in the msg_flags member of the msghdr 
>     structure. For stream-based sockets, such as SOCK_STREAM, message 
>     boundaries shall be ignored. In this case, data shall be returned to 
>     the user as soon as it becomes available, and no data shall be 
>     discarded.
>
> From this description it seems that SEQPACKET has no fixed size and 
> receiver if it is not sure that its buffer is big enough, it can use 
> MSG_PEEK. However MSG_TRUNC would allow to take only a part of it.
>
>> After some poking around I realized that the largest message we can
>> accept has to be capped to buf_alloc. However this makes the buffer size
>> visible to userspace and so conflicts with the idea of limiting
>> it for memory management purposes as described above.
> Yep, I think this is the real limit and IIRC the current implementation 
> checks how much space is available in the other peer before queuing the 
> packet, if there is no space it returns an error to the user.
Yes, if message length is bigger than 'peer_buf_alloc', then EMSGSIZE is returned
>
> The user can control it (also for STREAM) through the 
> SO_VM_SOCKETS_BUFFER_SIZE sockopt.
> (there is a maximum allowed, but it seems to high... I'll check it)
>
>> So it looks like we need to add a seqpacket message header overhead
>> field in the config space, and if present take that into account.
> Yes, and we should do that with stream as well when we don't merge 
> packets into a single buffer, there's a bug reported about that that we 
> need to fix sooner or later:
>     https://bugzilla.kernel.org/show_bug.cgi?id=215329
>
>> I am not sure what to do about it at this point.
>> Together with guests assuming this implies stream this looks a bit much.
>>
>> Sorry that I just noticed this part now, I guess better late than never.
> Sure :-)
>
>> Given it's deployed anyway, I guess we can put it in the spec as is,
>> however in that case I guess we should just document what's there,
>> maybe add some text explaining that it will be superceded
>> in a future version. But that *also* will mean we should
>> make it imply STREAM support since that is what happens in
>> the field. Maybe rename this to VIRTIO_VSOCK_F_SEQPACKET_COMPAT
>> or something like this.
>>
>> Thoughts?
> Aside from the issue of accounting for the header in memory consumption, 
> I think the rest is fine, so maybe we can leave it that way and in the 
> future add header overhead field in the config space like you said.
>
> Stefano
>
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/