Re: [External] Re: [RFC v4] virtio-vsock: add description for datagram type
On Wed, Jun 09, 2021 at 08:31:27PM -0700, Jiang Wang . wrote:
On Wed, Jun 9, 2021 at 12:17 AM Stefano Garzarella wrote:
On Tue, Jun 08, 2021 at 09:22:26PM -0700, Jiang Wang . wrote:
>On Tue, Jun 8, 2021 at 6:46 AM Stefano Garzarella wrote:
>>
>> On Fri, May 28, 2021 at 04:01:18AM +, Jiang Wang 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
>> >---
>> >
>> >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.
>> >
>> > virtio-vsock.tex | 155
++-
>> > 1 file changed, 142 insertions(+), 13 deletions(-)
>> >
>> >diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>> >index da7e641..bacac3c 100644
>> >--- a/virtio-vsock.tex
>> >+++ b/virtio-vsock.tex
>> >@@ -9,14 +9,41 @@ \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
>>
>> Is there a particular reason to always have the event queue as the last
>> one?
>>
>> Maybe it's better to add the datagram queues at the bottom, so the first
>> 3 queues are always the same.
>>
>I am not sure. I think Linux kernel should be fine with what you described.
>But I am not sure about QEMU. From the code, I see virtqueue is allocated
>as an array, like following,
>
>+ #ifdef CONFIG_VHOST_VSOCK_DGRAM
>+struct vhost_virtqueue vhost_vqs[4];
>+ #else
>struct vhost_virtqueue vhost_vqs[2];
>+ #endi
I see, also vhost_dev_init() requires an array, so I agree that this is
the best approach, sorry for the noise.
Just to be sure to check that anything is working if
CONFIG_VHOST_VSOCK_DGRAM is defined, but the guest has an old driver
that doesn't support DGRAM, and viceversa.
Sure. I just want to mention that the QEMU should be consistent
with the device (host). If QEMU enabled CONFIG_VHOST_VSOCK_DGRAM,
the device also needs to enable a similar option. Than the driver can
be old or new versions.
Okay, but we should allow to run an old QEMU (without DGRAM) with a new
kernel (with DGRAM support built it) and viceversa.
The features bit are used to guarantee compatibility and to enable and
disable features at runtime depending on what the device or driver
supports.
>
>so I assume the virtqueues for tx/rx should be
>continuous? I can try to put the new queues at the end and see if it
>works or not.
>
>btw, my qemu change is here:
>https://github.com/Jiang1155/qemu/commit/6307aa7a0c347905a31f3ca6577923e2f6dd9d84
>
>> >+\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.
>> >+
>> >+\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}
>> >
>> >-There are currently no feature bits defined for this device.
>> >+\begin{description}
>> >+\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket type.
>> >+\end{description}
>> >+
>> >+\begin{description}
>> >+\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket
>> >type.
>> >+\end{description}
>> >+
>> >+\begin{description}
>> >+\item[VIRTIO_VSOCK_F_MRG_RXBUF (3)] Driver can merge receive buffers.
>> >+\end{description}
>> >+
>> >+If no feature bits are defined, then assume only VIRTIO_VSOCK_F_STREAM
>> >is set.
>>
>> I'd say more like socket streams are supported, without reference to the
>> feature bit, something like: "If no feature bits are defined, then
>> assume device only supports stream socket type."
>>
>OK.
>
>> >
>> > \subsection{Device configuration layout}\label{sec:Device Types / Socket
Device / Device configuration layout}
>> >
>> >@@ -64,6 +91,8 @@ \subsection{Device Operation}\label{sec:Device Types /
Socket Device / Device Op
>> >
>> > Packets transmitted or received contain a header before the payload:
>> >
>> >+If feature
Re: [External] Re: [RFC v4] virtio-vsock: add description for datagram type
On Wed, Jun 9, 2021 at 12:17 AM Stefano Garzarella wrote:
>
> On Tue, Jun 08, 2021 at 09:22:26PM -0700, Jiang Wang . wrote:
> >On Tue, Jun 8, 2021 at 6:46 AM Stefano Garzarella
> >wrote:
> >>
> >> On Fri, May 28, 2021 at 04:01:18AM +, Jiang Wang 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
> >> >---
> >> >
> >> >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.
> >> >
> >> > virtio-vsock.tex | 155
> >> > ++-
> >> > 1 file changed, 142 insertions(+), 13 deletions(-)
> >> >
> >> >diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> >> >index da7e641..bacac3c 100644
> >> >--- a/virtio-vsock.tex
> >> >+++ b/virtio-vsock.tex
> >> >@@ -9,14 +9,41 @@ \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
> >>
> >> Is there a particular reason to always have the event queue as the last
> >> one?
> >>
> >> Maybe it's better to add the datagram queues at the bottom, so the first
> >> 3 queues are always the same.
> >>
> >I am not sure. I think Linux kernel should be fine with what you described.
> >But I am not sure about QEMU. From the code, I see virtqueue is allocated
> >as an array, like following,
> >
> >+ #ifdef CONFIG_VHOST_VSOCK_DGRAM
> >+struct vhost_virtqueue vhost_vqs[4];
> >+ #else
> >struct vhost_virtqueue vhost_vqs[2];
> >+ #endi
>
> I see, also vhost_dev_init() requires an array, so I agree that this is
> the best approach, sorry for the noise.
>
> Just to be sure to check that anything is working if
> CONFIG_VHOST_VSOCK_DGRAM is defined, but the guest has an old driver
> that doesn't support DGRAM, and viceversa.
Sure. I just want to mention that the QEMU should be consistent
with the device (host). If QEMU enabled CONFIG_VHOST_VSOCK_DGRAM,
the device also needs to enable a similar option. Than the driver can
be old or new versions.
> >
> >so I assume the virtqueues for tx/rx should be
> >continuous? I can try to put the new queues at the end and see if it
> >works or not.
> >
> >btw, my qemu change is here:
> >https://github.com/Jiang1155/qemu/commit/6307aa7a0c347905a31f3ca6577923e2f6dd9d84
> >
> >> >+\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.
> >> >+
> >> >+\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}
> >> >
> >> >-There are currently no feature bits defined for this device.
> >> >+\begin{description}
> >> >+\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket
> >> >type.
> >> >+\end{description}
> >> >+
> >> >+\begin{description}
> >> >+\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket
> >> >type.
> >> >+\end{description}
> >> >+
> >> >+\begin{description}
> >> >+\item[VIRTIO_VSOCK_F_MRG_RXBUF (3)] Driver can merge receive buffers.
> >> >+\end{description}
> >> >+
> >> >+If no feature bits are defined, then assume only VIRTIO_VSOCK_F_STREAM
> >> >is set.
> >>
> >> I'd say more like socket streams are supported, without reference to the
> >> feature bit, something like: "If no feature bits are defined, then
> >> assume device only supports stream socket type."
> >>
> >OK.
> >
> >> >
> >> > \subsection{Device configuration layout}\label{sec:Device Types / Socket
> >> > Device / Device configuration layout}
> >> >
> >> >@@ -64,6 +91,8 @@ \subsection{Device Operation}\label{sec:Device Types /
> >> >Socket Device / Device Op
> >> >
> >> > Packets transmitted or received contain a header before the payload:
> >> >
> >> >+If feature VIRTIO_VSOCK_F_MRG_RXBUF is not negotiated, use the following
Re: [External] Re: [RFC v4] virtio-vsock: add description for datagram type
On Tue, Jun 08, 2021 at 09:22:26PM -0700, Jiang Wang . wrote:
On Tue, Jun 8, 2021 at 6:46 AM Stefano Garzarella wrote:
On Fri, May 28, 2021 at 04:01:18AM +, Jiang Wang 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
>---
>
>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.
>
> virtio-vsock.tex | 155 ++-
> 1 file changed, 142 insertions(+), 13 deletions(-)
>
>diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>index da7e641..bacac3c 100644
>--- a/virtio-vsock.tex
>+++ b/virtio-vsock.tex
>@@ -9,14 +9,41 @@ \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
Is there a particular reason to always have the event queue as the last
one?
Maybe it's better to add the datagram queues at the bottom, so the first
3 queues are always the same.
I am not sure. I think Linux kernel should be fine with what you described.
But I am not sure about QEMU. From the code, I see virtqueue is allocated
as an array, like following,
+ #ifdef CONFIG_VHOST_VSOCK_DGRAM
+struct vhost_virtqueue vhost_vqs[4];
+ #else
struct vhost_virtqueue vhost_vqs[2];
+ #endi
I see, also vhost_dev_init() requires an array, so I agree that this is
the best approach, sorry for the noise.
Just to be sure to check that anything is working if
CONFIG_VHOST_VSOCK_DGRAM is defined, but the guest has an old driver
that doesn't support DGRAM, and viceversa.
so I assume the virtqueues for tx/rx should be
continuous? I can try to put the new queues at the end and see if it
works or not.
btw, my qemu change is here:
https://github.com/Jiang1155/qemu/commit/6307aa7a0c347905a31f3ca6577923e2f6dd9d84
>+\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.
>+
>+\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}
>
>-There are currently no feature bits defined for this device.
>+\begin{description}
>+\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket type.
>+\end{description}
>+
>+\begin{description}
>+\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket
>type.
>+\end{description}
>+
>+\begin{description}
>+\item[VIRTIO_VSOCK_F_MRG_RXBUF (3)] Driver can merge receive buffers.
>+\end{description}
>+
>+If no feature bits are defined, then assume only VIRTIO_VSOCK_F_STREAM
>is set.
I'd say more like socket streams are supported, without reference to the
feature bit, something like: "If no feature bits are defined, then
assume device only supports stream socket type."
OK.
>
> \subsection{Device configuration layout}\label{sec:Device Types / Socket
Device / Device configuration layout}
>
>@@ -64,6 +91,8 @@ \subsection{Device Operation}\label{sec:Device Types /
Socket Device / Device Op
>
> Packets transmitted or received contain a header before the payload:
>
>+If feature VIRTIO_VSOCK_F_MRG_RXBUF is not negotiated, use the following
header.
>+
> \begin{lstlisting}
> struct virtio_vsock_hdr {
> le64 src_cid;
>@@ -79,6 +108,15 @@ \subsection{Device Operation}\label{sec:Device Types /
Socket Device / Device Op
> };
> \end{lstlisting}
>
>+If feature VIRTIO_VSOCK_F_MRG_RXBUF is negotiated, use the following header.
>+\begin{lstlisting}
>+struct virtio_vsock_hdr_mrg_rxbuf {
>+ struct virtio_vsock_hdr hdr;
>+ le16 num_buffers;
>+};
>+\end{lstlisting}
>+
>+
> The upper 32 bits of src_cid and dst_cid are reserved and zeroed.
>
> Most packets simply transfer data but control packets are also used for
>@@ -107,6 +145,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;
Re: [External] Re: [RFC v4] virtio-vsock: add description for datagram type
On Tue, Jun 8, 2021 at 6:46 AM Stefano Garzarella wrote:
>
> On Fri, May 28, 2021 at 04:01:18AM +, Jiang Wang 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
> >---
> >
> >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.
> >
> > virtio-vsock.tex | 155
> > ++-
> > 1 file changed, 142 insertions(+), 13 deletions(-)
> >
> >diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> >index da7e641..bacac3c 100644
> >--- a/virtio-vsock.tex
> >+++ b/virtio-vsock.tex
> >@@ -9,14 +9,41 @@ \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
>
> Is there a particular reason to always have the event queue as the last
> one?
>
> Maybe it's better to add the datagram queues at the bottom, so the first
> 3 queues are always the same.
>
I am not sure. I think Linux kernel should be fine with what you described.
But I am not sure about QEMU. From the code, I see virtqueue is allocated
as an array, like following,
+ #ifdef CONFIG_VHOST_VSOCK_DGRAM
+struct vhost_virtqueue vhost_vqs[4];
+ #else
struct vhost_virtqueue vhost_vqs[2];
+ #endi
so I assume the virtqueues for tx/rx should be
continuous? I can try to put the new queues at the end and see if it
works or not.
btw, my qemu change is here:
https://github.com/Jiang1155/qemu/commit/6307aa7a0c347905a31f3ca6577923e2f6dd9d84
> >+\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.
> >+
> >+\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}
> >
> >-There are currently no feature bits defined for this device.
> >+\begin{description}
> >+\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket type.
> >+\end{description}
> >+
> >+\begin{description}
> >+\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket
> >type.
> >+\end{description}
> >+
> >+\begin{description}
> >+\item[VIRTIO_VSOCK_F_MRG_RXBUF (3)] Driver can merge receive buffers.
> >+\end{description}
> >+
> >+If no feature bits are defined, then assume only VIRTIO_VSOCK_F_STREAM
> >is set.
>
> I'd say more like socket streams are supported, without reference to the
> feature bit, something like: "If no feature bits are defined, then
> assume device only supports stream socket type."
>
OK.
> >
> > \subsection{Device configuration layout}\label{sec:Device Types / Socket
> > Device / Device configuration layout}
> >
> >@@ -64,6 +91,8 @@ \subsection{Device Operation}\label{sec:Device Types /
> >Socket Device / Device Op
> >
> > Packets transmitted or received contain a header before the payload:
> >
> >+If feature VIRTIO_VSOCK_F_MRG_RXBUF is not negotiated, use the following
> >header.
> >+
> > \begin{lstlisting}
> > struct virtio_vsock_hdr {
> > le64 src_cid;
> >@@ -79,6 +108,15 @@ \subsection{Device Operation}\label{sec:Device Types /
> >Socket Device / Device Op
> > };
> > \end{lstlisting}
> >
> >+If feature VIRTIO_VSOCK_F_MRG_RXBUF is negotiated, use the following header.
> >+\begin{lstlisting}
> >+struct virtio_vsock_hdr_mrg_rxbuf {
> >+ struct virtio_vsock_hdr hdr;
> >+ le16 num_buffers;
> >+};
> >+\end{lstlisting}
> >+
> >+
> > The upper 32 bits of src_cid and dst_cid are reserved and zeroed.
> >
> > Most packets simply transfer data but control packets are also used for
> >@@ -107,6 +145,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
