Re: [virtio-comment] [PATCH v5 1/2] virtio-vsock: add description for datagram type

2022-03-06 Thread Michael S. Tsirkin
On Sat, Mar 05, 2022 at 01:25:44AM +, Bobby Eshleman wrote:
> On Thu, Mar 03, 2022 at 02:15:24AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Mar 03, 2022 at 03:29:31AM +, Bobby Eshleman wrote:
> > > On Wed, Mar 02, 2022 at 05:09:58PM +0100, Stefano Garzarella wrote:
> > > > Hi Bobby,
> > > > Sorry for the delay, but I saw these patches today.
> > > > Please, can you keep me in CC?
> > > > 
> > > 
> > > Hey Stefano, sorry about that. I'm not sure how I lost your CC on this
> > > one. I'll make sure you are there moving forward.
> > > 
> > > I want to mention that I'm taking a look at
> > > https://gitlab.com/vsock/vsock/-/issues/1 in parallel with my dgram work
> > > here. After sending out this series we noticed potential overlap between
> > > the two issues. The additional dgram queues may become redundant if a
> > > fairness mechanism that solves issue #1 above also applies to
> > > connection-less protocols (similar to how the TC subsystem works). I've
> > > just begun sorting out potential solutions so no hard results yet. Just
> > > putting on your radar that the proposal here in v5 may be impacted if my
> > > investigation into issue #1 yields something adequate.
> > 
> > 
> > Well not mergeable, but datagram is upstream in Linux, is it not?
> > So we do want it in the spec IMHO, even if in the future there
> > will be a better protocol.
> > 
> 
> As of right now, it is not upstream in Linux. The virtio transport just passes
> -EOPNOTSUPP up the stack when the sock invokes it.
> 
> I think what you're thinking of is the vsock dgram in VMWare's vmci and
> Hyper-V. They support dgrams, but are not compatible with virtio (e.g., don't
> use virtqueues).
> 
> -Bobby

Oh no, I was thinking about SEQPACKET actually. Which has the same issue
I noted on another thread with memory accounting as datagram by the way.

-- 
MST

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [virtio-comment] [PATCH v5 1/2] virtio-vsock: add description for datagram type

2022-03-03 Thread Michael S. Tsirkin
On Thu, Mar 03, 2022 at 03:29:31AM +, Bobby Eshleman wrote:
> On Wed, Mar 02, 2022 at 05:09:58PM +0100, Stefano Garzarella wrote:
> > Hi Bobby,
> > Sorry for the delay, but I saw these patches today.
> > Please, can you keep me in CC?
> > 
> 
> Hey Stefano, sorry about that. I'm not sure how I lost your CC on this
> one. I'll make sure you are there moving forward.
> 
> I want to mention that I'm taking a look at
> https://gitlab.com/vsock/vsock/-/issues/1 in parallel with my dgram work
> here. After sending out this series we noticed potential overlap between
> the two issues. The additional dgram queues may become redundant if a
> fairness mechanism that solves issue #1 above also applies to
> connection-less protocols (similar to how the TC subsystem works). I've
> just begun sorting out potential solutions so no hard results yet. Just
> putting on your radar that the proposal here in v5 may be impacted if my
> investigation into issue #1 yields something adequate.


With respect to datagram, there is actually another issue which also
exists for stream but is smaller there - per message overhead is not
accounted for.  For stream we can work around that by copying payload
data.  For datagram we can't as we need to preserve message boundaries.
One way to address that is to add config for host/guest per-message
overhead, and have sender decrement fwd counter by that value for
each message sent.

-- 
MST

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [virtio-comment] [PATCH v5 1/2] virtio-vsock: add description for datagram type

2022-03-03 Thread Stefano Garzarella

On Thu, Mar 03, 2022 at 03:29:31AM +, Bobby Eshleman wrote:

On Wed, Mar 02, 2022 at 05:09:58PM +0100, Stefano Garzarella wrote:

Hi Bobby,
Sorry for the delay, but I saw these patches today.
Please, can you keep me in CC?



Hey Stefano, sorry about that. I'm not sure how I lost your CC on this
one. I'll make sure you are there moving forward.


No problem :-)



I want to mention that I'm taking a look at
https://gitlab.com/vsock/vsock/-/issues/1 in parallel with my dgram work
here. After sending out this series we noticed potential overlap between
the two issues. The additional dgram queues may become redundant if a
fairness mechanism that solves issue #1 above also applies to
connection-less protocols (similar to how the TC subsystem works). I've
just begun sorting out potential solutions so no hard results yet. Just
putting on your radar that the proposal here in v5 may be impacted if my
investigation into issue #1 yields something adequate.


Oh, this would be great!




On Thu, Feb 24, 2022 at 10:15:46PM +, beshleman.dev...@gmail.com wrote:
> From: Jiang Wang 
>


... snip ...


>
> virtio-vsock.tex | 63 +++-
> 1 file changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index d79984d..1a66a1b 100644
> --- a/virtio-vsock.tex
> +++ b/virtio-vsock.tex
> @@ -9,11 +9,26 @@ \subsection{Device ID}\label{sec:Device Types / Socket 
Device / Device ID}
>
> \subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
> \begin{description}
> -\item[0] rx
> -\item[1] tx
> +\item[0] stream rx
> +\item[1] stream tx
> +\item[2] datagram rx
> +\item[3] datagram tx
> +\item[4] event
> +\end{description}
> +The virtio socket device uses 5 queues if feature bit VIRTIO_VSOCK_F_DRGAM 
is set. Otherwise, it
> +only uses 3 queues, as the following.

We are also adding a new flag (VIRTIO_VSOCK_F_NO_IMPLIED_STREAM) to
provide the possibility to support for example only dgrams.

So I think we should consider the case where we have only DGRAM queues
(and it will be similar to the stream only case so 3 queues).

Maybe we could describe this part better and say that if we have both
STREAM (or SEQPACKET) and DGRAM set we have 5 queues, otherwise
only 3 queues.



Roger that.


> \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
> stream sockets. The guest and the device publish how much buffer space is
> @@ -170,7 +193,7 @@ \subsubsection{Buffer Space Management}\label{sec:Device 
Types / Socket Device /
> u32 peer_free = peer_buf_alloc - (tx_cnt - peer_fwd_cnt);
> \end{lstlisting}
>
> -If there is insufficient buffer space, the sender waits until virtqueue 
buffers
> +For stream sockets, if there is insufficient buffer space, the sender waits 
until virtqueue buffers

stream and seqpacket

> are returned and checks \field{buf_alloc} and \field{fwd_cnt} again. Sending
> the VIRTIO_VSOCK_OP_CREDIT_REQUEST packet queries how much buffer space is
> available. The reply to this query is a VIRTIO_VSOCK_OP_CREDIT_UPDATE packet.
> @@ -178,22 +201,32 @@ \subsubsection{Buffer Space 
Management}\label{sec:Device Types / Socket Device /
> previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This allows
> communicating updates any time a change in buffer space occurs.
>
> +Unlike stream sockets, dgram sockets do not use VIRTIO_VSOCK_OP_CREDIT_UPDATE
> +or VIRTIO_VSOCK_OP_CREDIT_REQUEST packets. The dgram buffer management is 
split
> +into two parts: senders and receivers. For senders, the packet is dropped if 
the
> +virtqueue is full. For receivers, the packet is dropped if there is no space
> +in the receive buffer.
> +
> \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.
> +For stream sockets, VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted 
when the peer has

stream and seqpacket

> +sufficient free buffer space for the payload. For dgram sockets, 
VIRTIO_VSOCK_OP_RW data packets
> +MAY be transmitted when the peer rx buffer is full. Then the packet will be 
dropped by the peer,
> +and driver will not get any notification.
>
> All packets associated with a stream flow MUST contain valid information in
> \field{buf_alloc} and \field{fwd_cnt} fields.
>
> \devicenormative{\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.
> +For stream sockets, VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted 
when the peer has


Re: [virtio-comment] [PATCH v5 1/2] virtio-vsock: add description for datagram type

2022-03-02 Thread Michael S. Tsirkin
On Thu, Mar 03, 2022 at 03:29:31AM +, Bobby Eshleman wrote:
> On Wed, Mar 02, 2022 at 05:09:58PM +0100, Stefano Garzarella wrote:
> > Hi Bobby,
> > Sorry for the delay, but I saw these patches today.
> > Please, can you keep me in CC?
> > 
> 
> Hey Stefano, sorry about that. I'm not sure how I lost your CC on this
> one. I'll make sure you are there moving forward.
> 
> I want to mention that I'm taking a look at
> https://gitlab.com/vsock/vsock/-/issues/1 in parallel with my dgram work
> here. After sending out this series we noticed potential overlap between
> the two issues. The additional dgram queues may become redundant if a
> fairness mechanism that solves issue #1 above also applies to
> connection-less protocols (similar to how the TC subsystem works). I've
> just begun sorting out potential solutions so no hard results yet. Just
> putting on your radar that the proposal here in v5 may be impacted if my
> investigation into issue #1 yields something adequate.


Well not mergeable, but datagram is upstream in Linux, is it not?
So we do want it in the spec IMHO, even if in the future there
will be a better protocol.

> > On Thu, Feb 24, 2022 at 10:15:46PM +, beshleman.dev...@gmail.com wrote:
> > > From: Jiang Wang 
> > > 
> 
> ... snip ...
> 
> > > 
> > > virtio-vsock.tex | 63 
> > > +++-
> > > 1 file changed, 53 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > > index d79984d..1a66a1b 100644
> > > --- a/virtio-vsock.tex
> > > +++ b/virtio-vsock.tex
> > > @@ -9,11 +9,26 @@ \subsection{Device ID}\label{sec:Device Types / Socket 
> > > Device / Device ID}
> > > 
> > > \subsection{Virtqueues}\label{sec:Device Types / Socket Device / 
> > > Virtqueues}
> > > \begin{description}
> > > -\item[0] rx
> > > -\item[1] tx
> > > +\item[0] stream rx
> > > +\item[1] stream tx
> > > +\item[2] datagram rx
> > > +\item[3] datagram tx
> > > +\item[4] event
> > > +\end{description}
> > > +The virtio socket device uses 5 queues if feature bit 
> > > VIRTIO_VSOCK_F_DRGAM is set. Otherwise, it
> > > +only uses 3 queues, as the following.
> > 
> > We are also adding a new flag (VIRTIO_VSOCK_F_NO_IMPLIED_STREAM) to
> > provide the possibility to support for example only dgrams.
> > 
> > So I think we should consider the case where we have only DGRAM queues
> > (and it will be similar to the stream only case so 3 queues).
> > 
> > Maybe we could describe this part better and say that if we have both
> > STREAM (or SEQPACKET) and DGRAM set we have 5 queues, otherwise
> > only 3 queues.
> > 
> 
> Roger that.
> 
> > > \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
> > > stream sockets. The guest and the device publish how much buffer space is
> > > @@ -170,7 +193,7 @@ \subsubsection{Buffer Space 
> > > Management}\label{sec:Device Types / Socket Device /
> > > u32 peer_free = peer_buf_alloc - (tx_cnt - peer_fwd_cnt);
> > > \end{lstlisting}
> > > 
> > > -If there is insufficient buffer space, the sender waits until virtqueue 
> > > buffers
> > > +For stream sockets, if there is insufficient buffer space, the sender 
> > > waits until virtqueue buffers
> > 
> > stream and seqpacket
> > 
> > > are returned and checks \field{buf_alloc} and \field{fwd_cnt} again. 
> > > Sending
> > > the VIRTIO_VSOCK_OP_CREDIT_REQUEST packet queries how much buffer space is
> > > available. The reply to this query is a VIRTIO_VSOCK_OP_CREDIT_UPDATE 
> > > packet.
> > > @@ -178,22 +201,32 @@ \subsubsection{Buffer Space 
> > > Management}\label{sec:Device Types / Socket Device /
> > > previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This allows
> > > communicating updates any time a change in buffer space occurs.
> > > 
> > > +Unlike stream sockets, dgram sockets do not use 
> > > VIRTIO_VSOCK_OP_CREDIT_UPDATE
> > > +or VIRTIO_VSOCK_OP_CREDIT_REQUEST packets. The dgram buffer management 
> > > is split
> > > +into two parts: senders and receivers. For senders, the packet is 
> > > dropped if the
> > > +virtqueue is full. For receivers, the packet is dropped if there is no 
> > > space
> > > +in the receive buffer.
> > > +
> > > \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.
> > > +For stream sockets, VIRTIO_VSOCK_OP_RW data packets MUST only be 
> > > transmitted when the peer has
> > 
> > stream and seqpacket
> > 
> > > +sufficient free buffer space for the payload. For dgram sockets, 
> > > VIRTIO_VSOCK_OP_RW data packets
> > > +MAY be transmitted when the peer rx buffer is full. Then the packet will 
> > 

Re: [virtio-comment] [PATCH v5 1/2] virtio-vsock: add description for datagram type

2022-03-02 Thread Stefano Garzarella

Hi Bobby,
Sorry for the delay, but I saw these patches today.
Please, can you keep me in CC?

On Thu, Feb 24, 2022 at 10:15:46PM +, beshleman.dev...@gmail.com wrote:

From: Jiang Wang 

Add supports for datagram type for virtio-vsock. Datagram
sockets are connectionless and unreliable. To avoid contention
with stream and other sockets, add two more virtqueues and
a new feature bit to identify if those two new queues exist or not.

Also add descriptions for resource management of datagram, which
does not use the existing credit update mechanism associated with
stream sockets.

Signed-off-by: Jiang Wang 
Signed-off-by: Bobby Eshleman 
---

V2: Addressed the comments for the previous version.
V3: Add description for the mergeable receive buffer.
V4: Add a feature bit for stream and reserver a bit for seqpacket.
   Fix mrg_rxbuf related sentences.
V5: Rebase onto head (change to more nicely accompany seqpacket changes).
   Remove statement about no feature bits being set (already made by seqpacket 
patches).
   Clarify \field{type} declaration.
   Use words "sender/receiver" instead of "tx/rx" for clarity, and other prose 
changes.
   Directly state that full buffers result in dropped packets.
   Change verbs to present tense.
   Clarify if-else pairs (e.g., "If XYZ is negotiated" is followed by "If XYZ
   is not negotiated" instead of "Otherwise").
   Mergeable buffer changes are split off into a separate patch.

virtio-vsock.tex | 63 +++-
1 file changed, 53 insertions(+), 10 deletions(-)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index d79984d..1a66a1b 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -9,11 +9,26 @@ \subsection{Device ID}\label{sec:Device Types / Socket Device 
/ Device ID}

\subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
\begin{description}
-\item[0] rx
-\item[1] tx
+\item[0] stream rx
+\item[1] stream tx
+\item[2] datagram rx
+\item[3] datagram tx
+\item[4] event
+\end{description}
+The virtio socket device uses 5 queues if feature bit VIRTIO_VSOCK_F_DRGAM is 
set. Otherwise, it
+only uses 3 queues, as the following.


We are also adding a new flag (VIRTIO_VSOCK_F_NO_IMPLIED_STREAM) to
provide the possibility to support for example only dgrams.

So I think we should consider the case where we have only DGRAM queues
(and it will be similar to the stream only case so 3 queues).

Maybe we could describe this part better and say that if we have both
STREAM (or SEQPACKET) and DGRAM set we have 5 queues, otherwise
only 3 queues.


+
+\begin{description}
+\item[0] stream rx
+\item[1] stream tx
\item[2] event
\end{description}

+When behavior differs between stream and datagram rx/tx virtqueues
+their full names are used. Common behavior is simply described in
+terms of rx/tx virtqueues and applies to both stream and datagram
+virtqueues.
+
\subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}

If no feature bit is set, only stream socket type is supported.
@@ -23,6 +38,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.
+\item[VIRTIO_VSOCK_F_DGRAM (2)] datagram socket type is supported.
\end{description}

\subsection{Device configuration layout}\label{sec:Device Types / Socket Device 
/ Device configuration layout}
@@ -109,6 +125,9 @@ \subsection{Device Operation}\label{sec:Device Types / 
Socket Device / Device Op

\subsubsection{Virtqueue Flow Control}\label{sec:Device Types / Socket Device / 
Device Operation / Virtqueue Flow Control}

+Flow control applies to stream sockets; datagram sockets do not have
+flow control.
+
The tx virtqueue carries packets initiated by applications and replies to
received packets.  The rx virtqueue carries packets initiated by the device and
replies to previously transmitted packets.
@@ -142,18 +161,22 @@ \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 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.
+Currently stream, seqpacket, and dgram sockets are supported. \field{type} is 
1 (VIRTIO_VSOCK_TYPE_STREAM)
+for stream socket types, 2 (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket socket 
types, and 3 (VIRTIO_VSOCK_TYPE_DGRAM) for dgram socket types.

\begin{lstlisting}
#define VIRTIO_VSOCK_TYPE_STREAM1
#define VIRTIO_VSOCK_TYPE_SEQPACKET 2
+#define VIRTIO_VSOCK_TYPE_DGRAM 3
\end{lstlisting}

Stream sockets provide in-order, guaranteed, connection-oriented delivery
without message boundaries. Seqpacket sockets provide