Re: Re: [virtio-comment] [PATCH v2 03/11] transport-fabircs: introduce Segment Descriptor Definition

[zhenwei pi <pizhenwei@bytedance.com>]

On 5/31/23 22:23, Stefan Hajnoczi wrote:
> On Thu, May 04, 2023 at 04:19:02PM +0800, zhenwei pi wrote:
> > Introduce segment descriptor to describe the Virtio device buffer
> > segments.
> >
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > ---
> >   transport-fabrics.tex | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 43 insertions(+)
> >
> > diff --git a/transport-fabrics.tex b/transport-fabrics.tex
> > index 26b0192..b88acfd 100644
> > --- a/transport-fabrics.tex
> > +++ b/transport-fabrics.tex
> > @@ -45,3 +45,46 @@ \subsection{Virtio Qualified Name}\label{sec:Virtio Transport Options / Virtio O
> >   \item The string is null terminated.
> >   \item There is no strict style limitation.
> >   \end{itemize}
> > +
> > +\subsection{Transmission Protocol}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol}
> > +This section defines transmission protocol for Virtio Over Fabrics. All the
>
> What does "transmission protocol" mean? I guess this is what is often
> called a network protocol or a wire protocol or just a protocol, but it
> wasn't clear to me maybe whether the "transmission protocol" is one
> protocol out of a set of protocols that make up Virtio Over Fabrics.
>
> This paragraph should describe which connections use this protocol. For
> example:
>
>    This protocol is used for both control and virtqueue connections.
>
> > +fields use little endian format.
> > +
> > +\subsubsection{Segment Descriptor Definition}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Segment Descriptor Definition}
> > +Virtio Over Fabrics uses the following structure to describe data segment:
>
> What is a data segment? I guess it's a message/command/request?
>
> There should be an explanation of how data segments are used. For
> example:
>
>    The initiator sends a data segment containing the command to the
>    target. The target sends a data segment containing the response to the
>    command back to the initiator.
>
> > +
> > +\begin{lstlisting}
> > +struct virtio_of_vring_desc {
>
> I think the name "vring" should be avoided. The vring is an in-memory
> layout for implementing virtqueues where shared memory is available.
> Calling it virtio_of_vq_desc makes it clear that Virtio Over Fabrics
> does not use vrings to implement virtqueues.
>
> > +        le64 addr;
> > +        le32 length;
> > +        /* This marks the unique ID within a command, no limitation among inflight commands */
>
> What is a command?
>
> > +        le16 id;
> > +        /* This marks a buffer as keyed transmission (otherwise stream transmission) */
> > +#define VIRTIO_OF_DESC_F_KEYED     1
> > +        /* This marks a buffer as device write-only (otherwise device read-only). */
> > +#define VIRTIO_OF_DESC_F_WRITE     2
> > +        le16 flags;
> > +        le32 key;
> > +};
> > +\end{lstlisting}
> > +
> > +The structure virtio_of_vring_desc is used for both keyed transmission
> > +(i.e. RDMA) and stream transmission(i.e. TCP). The fields is described as follows:
> > +
> > +\begin{tabular}{ |l|l|l| }
> > +\hline
> > +Field & keyed transmission & stream transmission \\
> > +\hline \hline
> > +addr & Start address of remote memory buffer & Start address within the stream buffer \\
>
> What is a stream buffer?
>
> > +\hline
> > +length & The length of remote memory buffer & The length of buffer within the stream \\
>
> I'm not sure what buffer means here. I guess it's not the same as a
> virtqueue buffer, it's probably a virtqueue descriptor (element)?
>
> Can you avoid using buffer here since it usually means something else in
> Virtio?

OK.

> > +\hline
> > +id & The ID of this descriptor & The ID of this descriptor \\
> > +\hline
> > +flags & both keyed transmission and stream transmission supported & stream transmission only \\
>
> I'm not sure what this means.
>
> > +\hline
> > +key & Key of the remote Memory Region & Ignore \\
>
> Should "Ignore" be "Reserved" so that stream transmission can use this
> field for something else in the future?

OK

> > +\hline
> > +\end{tabular}
> > +
> > +Depending on the opcode, a Command contains zero or more structure virtio_of_vring_desc.
>
> opcode hasn't been defined yet. I guess that's because the first
> virtio_of_vring_desc contains a Command and that has an opcode field?
> Please make sure the text is ordered so that terms are defined before
> they are used.
>
> Stefan

OK, I think reordering the text is needed in the next version.

--
zhenwei pi