From: Stefan Hajnoczi <stefanha@redhat.com> To: Jiang Wang <jiang.wang@bytedance.com> Cc: cong.wang@bytedance.com, duanxiongchun@bytedance.com, mst@redhat.com, cohuck@redhat.com, virtualization@lists.linux-foundation.org, xieyongji@bytedance.com, chaiwen.cc@bytedance.com, virtio-comment@lists.oasis-open.org, asias@redhat.com, arseny.krasnov@kaspersky.com, jhansen@vmware.com Subject: Re: [PATCH v5] virtio-vsock: add description for datagram type Date: Thu, 2 Sep 2021 15:07:23 +0100 [thread overview] Message-ID: <YTDam7jdjRz686bp@stefanha-x1.localdomain> (raw) In-Reply-To: <20210610181203.979686-1-jiang.wang@bytedance.com> [-- Attachment #1.1: Type: text/plain, Size: 3852 bytes --] On Thu, Jun 10, 2021 at 06:12:03PM +0000, Jiang Wang wrote: > 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 <jiang.wang@bytedance.com> > --- Overall this looks good. The tricky thing will be implementing dgram sockets in a way that minimizes dropped packets and provides some degree of fairness between senders. Those are implementation issues though and not visible at the device specification level. > diff --git a/virtio-vsock.tex b/virtio-vsock.tex > index da7e641..26a62ac 100644 > --- a/virtio-vsock.tex > +++ b/virtio-vsock.tex > @@ -9,14 +9,37 @@ \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. s/as the following/as follows:/ > + > +\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. Is this really bit 2 or did you mean bit 1 (value 0x2)? What happens to the virtqueue layout when VIRTIO_VSOCK_F_DGRAM is present and VIRTIO_VSOCK_F_STREAM is absent? The virtqueue section above implies that VIRTIO_VSOCK_F_STREAM is always present. > +\end{description} > + > +If no feature bits are defined, assume device only supports stream socket type. It's cleaner to define VIRTIO_VSOCK_F_NO_STREAM (0) instead. When the bit is set the stream socket type is not available and the stream_rx/tx virtqueues are absent. This way it's not necessary to define special behavior depending on certain combinations of feature bits. > \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / Device Operation / Receive and Transmit} > -The driver queues outgoing packets on the tx virtqueue and incoming packet > +The driver queues outgoing packets on the tx virtqueue and allocates incoming packet > receive buffers on the rx virtqueue. Packets are of the following form: This change seems unrelated to dgram sockets. I don't think adding the word "allocates" makes things clearer or more precise. The driver may reuse receive buffers rather than allocating fresh buffers. I suggest dropping this change. > > \begin{lstlisting} > @@ -195,6 +235,7 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De > }; > \end{lstlisting} > > + > Virtqueue buffers for outgoing packets are read-only. Virtqueue buffers for > incoming packets are write-only. > Unnecessary whitespace change. Please drop. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 183 bytes --] _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
WARNING: multiple messages have this Message-ID (diff)
From: Stefan Hajnoczi <stefanha@redhat.com> To: Jiang Wang <jiang.wang@bytedance.com> Cc: mst@redhat.com, cohuck@redhat.com, virtio-comment@lists.oasis-open.org, virtualization@lists.linux-foundation.org, asias@redhat.com, sgarzare@redhat.com, arseny.krasnov@kaspersky.com, cong.wang@bytedance.com, duanxiongchun@bytedance.com, xieyongji@bytedance.com, chaiwen.cc@bytedance.com, jhansen@vmware.com, jasowang@redhat.com Subject: Re: [PATCH v5] virtio-vsock: add description for datagram type Date: Thu, 2 Sep 2021 15:07:23 +0100 [thread overview] Message-ID: <YTDam7jdjRz686bp@stefanha-x1.localdomain> (raw) In-Reply-To: <20210610181203.979686-1-jiang.wang@bytedance.com> [-- Attachment #1: Type: text/plain, Size: 3852 bytes --] On Thu, Jun 10, 2021 at 06:12:03PM +0000, Jiang Wang wrote: > 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 <jiang.wang@bytedance.com> > --- Overall this looks good. The tricky thing will be implementing dgram sockets in a way that minimizes dropped packets and provides some degree of fairness between senders. Those are implementation issues though and not visible at the device specification level. > diff --git a/virtio-vsock.tex b/virtio-vsock.tex > index da7e641..26a62ac 100644 > --- a/virtio-vsock.tex > +++ b/virtio-vsock.tex > @@ -9,14 +9,37 @@ \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. s/as the following/as follows:/ > + > +\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. Is this really bit 2 or did you mean bit 1 (value 0x2)? What happens to the virtqueue layout when VIRTIO_VSOCK_F_DGRAM is present and VIRTIO_VSOCK_F_STREAM is absent? The virtqueue section above implies that VIRTIO_VSOCK_F_STREAM is always present. > +\end{description} > + > +If no feature bits are defined, assume device only supports stream socket type. It's cleaner to define VIRTIO_VSOCK_F_NO_STREAM (0) instead. When the bit is set the stream socket type is not available and the stream_rx/tx virtqueues are absent. This way it's not necessary to define special behavior depending on certain combinations of feature bits. > \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / Device Operation / Receive and Transmit} > -The driver queues outgoing packets on the tx virtqueue and incoming packet > +The driver queues outgoing packets on the tx virtqueue and allocates incoming packet > receive buffers on the rx virtqueue. Packets are of the following form: This change seems unrelated to dgram sockets. I don't think adding the word "allocates" makes things clearer or more precise. The driver may reuse receive buffers rather than allocating fresh buffers. I suggest dropping this change. > > \begin{lstlisting} > @@ -195,6 +235,7 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De > }; > \end{lstlisting} > > + > Virtqueue buffers for outgoing packets are read-only. Virtqueue buffers for > incoming packets are write-only. > Unnecessary whitespace change. Please drop. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2021-09-02 14:07 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-10 18:12 [PATCH v5] virtio-vsock: add description for datagram type Jiang Wang 2021-06-10 18:12 ` [virtio-comment] " Jiang Wang 2021-09-01 1:13 ` Michael S. Tsirkin 2021-09-01 1:13 ` Michael S. Tsirkin 2021-09-01 1:24 ` [External] " Jiang Wang . 2021-09-01 1:24 ` Jiang Wang . 2021-09-03 7:58 ` Michael S. Tsirkin 2021-09-03 7:58 ` Michael S. Tsirkin 2021-09-02 14:07 ` Stefan Hajnoczi [this message] 2021-09-02 14:07 ` Stefan Hajnoczi 2021-09-03 0:08 ` [virtio-comment] " Jiang Wang . 2021-09-03 0:08 ` Jiang Wang . 2021-09-03 7:22 ` Stefano Garzarella 2021-09-03 7:22 ` Stefano Garzarella 2021-09-03 7:57 ` Michael S. Tsirkin 2021-09-03 7:57 ` Michael S. Tsirkin 2021-09-03 10:51 ` Stefano Garzarella 2021-09-03 10:51 ` Stefano Garzarella
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YTDam7jdjRz686bp@stefanha-x1.localdomain \ --to=stefanha@redhat.com \ --cc=arseny.krasnov@kaspersky.com \ --cc=asias@redhat.com \ --cc=chaiwen.cc@bytedance.com \ --cc=cohuck@redhat.com \ --cc=cong.wang@bytedance.com \ --cc=duanxiongchun@bytedance.com \ --cc=jhansen@vmware.com \ --cc=jiang.wang@bytedance.com \ --cc=mst@redhat.com \ --cc=virtio-comment@lists.oasis-open.org \ --cc=virtualization@lists.linux-foundation.org \ --cc=xieyongji@bytedance.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.