Re: [virtio-comment] [PATCH v2 04/11] transport-fabrics: introduce Stream Transmission

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6590 bytes --]

On Thu, May 04, 2023 at 04:19:03PM +0800, zhenwei pi wrote:
> Stream transmission is used for stream oriented communication(Ex TCP),
> also add virtio-blk read/write 8K example.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  transport-fabrics.tex | 229 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 229 insertions(+)
> 
> diff --git a/transport-fabrics.tex b/transport-fabrics.tex
> index b88acfd..c02cf26 100644
> --- a/transport-fabrics.tex
> +++ b/transport-fabrics.tex
> @@ -88,3 +88,232 @@ \subsubsection{Segment Descriptor Definition}\label{sec:Virtio Transport Options
>  \end{tabular}
>  
>  Depending on the opcode, a Command contains zero or more structure virtio_of_vring_desc.
> +
> +\subsubsection{Buffer Mapping Definition}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Buffer Mapping Definition}
> +Virtio Over Fabrics defines two types of buffer mapping rules.

What is a buffer? Is it a virtqueue buffer (consisting of one or more
descriptors/elements) or are you using the term for a different concept?

> +
> +\paragraph{Stream Transmission}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Stream Transmission}
> +Command, Segment Descriptors, and buffer are transmitted in a stream within a

Is a Segment Descriptor a virtio_of_vring_desc?

> +connection. The layout in stream:
> +
> +\begin{lstlisting}
> +CMDx contains 0 descriptor, CMDy contains (n - m + 1) descriptors and buffer:

"0 descriptors"

> +
> +     +-----+     +-----++-----+     +-----++-----+
> + ... | CMDx| ... | CMDy||DESCm| ... |DESCn|| BUF | ...
> +     +-----+     +-----++-----+     +-----++-----+
> +
> +COMPx contains 0 descriptor, COMPy contains (k - j + 1) descriptors and buffer:

I think this is the first time the concept of a completion (COMP) was
introduced. Please describe commands/completions before using them in
the text.

> +
> +     +-----+     +-----++-----+     +-----++-----+
> + ... |COMPx| ... |COMPy||DESCj| ... |DESCk|| BUF | ...
> +     +-----+     +-----++-----+     +-----++-----+
> +\end{lstlisting}
> +
> +An example of a virtio-blk write 8K request(total size: sizeof(Command) +
> +4 * sizeof(Descriptor) + 8208):
> +\begin{lstlisting}
> + COMMAND            +------+
> +                    |opcode|  ->  virtio_of_op_vring
> +                    +------+
> +                    |cmd id|  ->  10
> +                    +------+
> +                    |length|  ->  8208
> +                    +------+
> +                    |ndesc |  ->  4
> +                    +------+
> +                    |rsvd  |
> +                    +------+
> +
> + DESC0              +------+
> +              +-----|addr  |  -> 0
> +              |     +------+
> +              |     |length|  -> 16 (virtio blk write command)
> +              |     +------+
> +              |     |id    |  -> 0
> +              |     +------+
> +              |     |flags |  -> 0
> +              |     +------+
> +              |
> + DESC1        |     +------+
> +              | +---|addr  |  -> 16
> +              | |   +------+
> +              | |   |length|  -> 4096
> +              | |   +------+
> +              | |   |id    |  -> 1
> +              | |   +------+
> +              | |   |flags |  -> 0
> +              | |   +------+
> +              | |
> + DESC2        | |   +------+
> +              | | +-|addr  |  -> 4112
> +              | | | +------+
> +              | | | |length|  -> 4096
> +              | | | +------+
> +              | | | |id    |  -> 2
> +              | | | +------+
> +              | | | |flags |  -> 0
> +              | | | +------+
> +              | | |
> + DESC3        | | | +------+
> +              | | | |addr  |  -> 0

Is this field 0 in all stream connection VIRTIO_OF_DESC_F_WRITE
descriptors?

> +              | | | +------+
> +              | | | |length|  -> 1
> +              | | | +------+
> +              | | | |id    |  -> 3
> +              | | | +------+
> +              | | | |flags |  -> VIRTIO_OF_DESC_F_WRITE
> +              | | | +------+
> +              | | |
> + DATA         +-+-+>+------+  -> 0
> +                | | |......|
> +                +-+>+------+  -> 16
> +                  | |......|
> +                  +>+------+  -> 4112
> +                    |......|
> +                    +------+  -> 8208
> +\end{lstlisting}
> +
> +The Completion of this request(total size: sizeof(Completion) +
> +1 * sizeof(Descriptor) + 1):
> +\begin{lstlisting}
> + COMPLETION         +------+
> +                    |status|  ->  VIRTIO_OF_SUCCESS
> +                    +------+
> +                    |cmd id|  ->  10
> +                    +------+
> +                    |ndesc |  ->  1
> +                    +------+
> +                    |rsvd  |
> +                    +------+
> +                    |value |  -> 1 (value.u32)

What is this field and what does u32 mean?

> +                    +------+
> +
> + DESC0              +------+
> +                  +-|addr  |  -> 0
> +                  | +------+
> +                  | |length|  -> 1
> +                  | +------+
> +                  | |id    |  -> 3

This has to match with the original descriptor id sent with the Command?

> +                  | +------+
> +                  | |flags |  -> VIRTIO_OF_DESC_F_WRITE
> +                  | +------+
> +                  |
> + DATA             |>+------+  -> 0
> +                    |......|
> +                    +------+  -> 1
> +\end{lstlisting}

I think this is more flexible (and has more protocol overhead) than
necessary. When the device has used a virtqueue buffer, it indicates how
many bytes were used (this can be less than the totaly number of F_WRITE
bytes available). I don't think there is a need to communicate F_WRITE
descriptors, especially in the Completion. Just a Completion with a
'length' field instead of an 'ndesc' field followed by data is enough.

Since VIRTIO has flexible framing
(https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-390004),
there isn't really a need to communicate the F_WRITE descriptors at all,
just the maximum number of used bytes that the initiator expects.

Can you explain why you chose to transmit F_WRITE descriptors in both
the Command and the Completion? Maybe I missed a reason why it's
important.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]