[virtio-comment] Re: Re: Re: Re: [PATCH v2 06/11] transport-fabrics: introduce command set

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Jun 07, 2023 at 10:58:45AM +0800, zhenwei pi wrote:
> On 6/6/23 21:34, Stefan Hajnoczi wrote:
> > On Tue, Jun 06, 2023 at 09:31:27AM +0800, zhenwei pi wrote:
> > > On 6/6/23 00:30, Stefan Hajnoczi wrote:
> > > > On Fri, Jun 02, 2023 at 01:15:00PM +0800, zhenwei pi wrote:
> > > > > 
> > > > > 
> > > > > On 6/1/23 01:10, Stefan Hajnoczi wrote:
> > > > > > On Thu, May 04, 2023 at 04:19:05PM +0800, zhenwei pi wrote:
> > > > > > > Introduce command structures for Virtio-oF.
> > > > > > > 
> > > > > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > > > > > > ---
> > > > > > >     transport-fabrics.tex | 209 ++++++++++++++++++++++++++++++++++++++++++
> > > > > > >     1 file changed, 209 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/transport-fabrics.tex b/transport-fabrics.tex
> > > > > > > index 7711321..37f57c6 100644
> > > > > > > --- a/transport-fabrics.tex
> > > > > > > +++ b/transport-fabrics.tex
> > > > > > > @@ -495,3 +495,212 @@ \subsubsection{Buffer Mapping Definition}\label{sec:Virtio Transport Options / V
> > > > > > >                         |value |  -> 8193 (value.u32)
> > > > > > >                         +------+
> > > > > > >     \end{lstlisting}
> > > > > > > +
> > > > > > > +\subsubsection{Commands Definition}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition}
> > > > > > > +This section defines command structures for Virtio Over Fabrics.
> > > > > > > +
> > > > > > > +A common structure virtio_of_value is fixed to 8 bytes and MUST be used as one
> > > > > > > +of the following format:
> > > > > > > +
> > > > > > > +\begin{itemize}
> > > > > > > +\item u8
> > > > > > > +\item le16
> > > > > > > +\item le32
> > > > > > > +\item le64
> > > > > > > +\end{itemize}
> > > > > > 
> > > > > > The way it's written does not document where the u8, u16, u32 bytes are
> > > > > > located and that the unused bytes are 0. I think I understand what you
> > > > > > mean though:
> > > > > > 
> > > > > >      le64 value = cpu_to_le64((u64)v); /* v is u8, u16, u32, or u64 */
> > > > > > 
> > > > > > Please clarify.
> > > > > > 
> > > > > 
> > > > > I want to describe an union structure of 8 bytes:
> > > > > union virtio_of_value {
> > > > >       u8;
> > > > >       u16;
> > > > >       u32;
> > > > >       u64;
> > > > > };
> > > > > 
> > > > > Depending on the opcode, use the right one.
> > > > 
> > > > I was trying to point out that the memory layout of C unions is not
> > > > portable. Your example does not define the exact in-memory layout of
> > > > union virtio_of_value. Here is the first web search result I found about
> > > > this topic:
> > > > 
> > > >     "Q: And a related question: if you dump unions in binary form to a file,
> > > >     and then reload them from the file on a different platform, or with a
> > > >     program compiled by a different compiler, are you guaranteed to get
> > > >     back what you stored? (I think not, but I'm not sure)
> > > > 
> > > >     A: You're right; you're not."
> > > > 
> > > >     https://bytes.com/topic/c/answers/220372-unions-storage-abis
> > > > 
> > > > In the cpu_to_le64() code example that I gave, the exact in-memory
> > > > layout is well-defined. There is no ambiguity.
> > > > 
> > > 
> > > OK, thanks.
> > > 
> > > > > > > +\hline
> > > > > > > +0xff00 - 0xfffd & Reserved \\
> > > > > > > +\hline
> > > > > > > +\end{tabular}
> > > > > > > +
> > > > > > > +\paragraph{Connect Command}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition / Connect Command}
> > > > > > > +The Connect Command is used to establish Virtio Over Fabrics queue. The control
> > > > > > > +queue MUST be established firstly, then the Connect command establishes an
> > > > > > > +association between the initiator and the target.
> > > > > > 
> > > > > > Is a "Virtio Over Fabrics queue" different from a virtqueue?
> > > > > > 
> > > > > > If I understand correctly, the control queue must be established by the
> > > > > > initiator first and then the Connect command is sent to begin
> > > > > > communication between the initiator and the target?
> > > > > > 
> > > > > 
> > > > > The queue mapping is missing in the '[PATCH v2 01/11] transport-fabrics:
> > > > > introduce Virtio Over Fabrics overview', like:
> > > > > A "Virtio Over Fabrics queue" is a reliable connection between initiator and
> > > > > target. There are 2 types of Virtio Over Fabrics queue:
> > > > > +\begin{itemize}
> > > > > +\item A single Control queue is required to execute control operations.
> > > > > +\item 0 or more Virtio Over Fabrics queues map the virtqueues.
> > > > > +\end{itemize}
> > > > 
> > > > That helps, thanks!
> > > > 
> > > > > 
> > > > > > > +
> > > > > > > +The Target ID of 0xffff is reserved, then:
> > > > > > 
> > > > > > Please move this after the fields have been shown and the purpose of the
> > > > > > Target ID field has been explained.
> > > > > > 
> > > > > > > +\begin{itemize}
> > > > > > > +\item The Target ID of 0xffff MUST be specified as the Target ID in a Connect
> > > > > > > +Command for the control queue.
> > > > > > > +\item The target SHOULD allocate any available Target ID to the initiator,
> > > > > > > +and return the allocated Target ID in the Completion.
> > > > > > > +\item The returned Target ID MUST be specified as the Target ID, and the Queue ID
> > > > > > > +MUST be specified in a Connect Command for the virtqueue.
> > > > > > > +\end{itemize}
> > > > > > 
> > > > > > What is the purpose of the Target ID? Is it to allow a server to provide
> > > > > > access to multiple targets over the same connection?
> > > > > > 
> > > > > 
> > > > > A target listens on a port, and provides access to 0 or more targets. An
> > > > > initiator connect the specific target by TVQN of connect command.
> > > > > An initiator could connect a single target, multiple initiators could
> > > > > connect the same target(typically, shared disk/fs).
> > > > 
> > > > Why is the target ID separate from the TVQN? If the Target ID is a
> > > > separate parameter then users will have to learn additional
> > > > syntax/command-line options to specify the TVQN + Target ID and that
> > > > syntax may vary between software.
> > > > 
> > > > > 
> > > > > > > +
> > > > > > > +The Connect Command has following structure:
> > > > > > > +
> > > > > > > +\begin{lstlisting}
> > > > > > > +struct virtio_of_command_connect {
> > > > > > > +        le16 opcode;
> > > > > > > +        le16 command_id;
> > > > > > > +        le16 target_id;
> > > > > > > +        le16 queue_id;
> > > > > > > +        le16 ndesc;
> > > > > > 
> > > > > > Where is this field documented?
> > > > > > 
> > > > > 
> > > > > OK. Will add.
> > > > > 
> > > > > > Why does the initiator send ndesc to the target? Normally a VIRTIO Transport reports the device's max descriptors and then the driver can tell the device to reduce the number of descriptors, if desired.
> > > > > > 
> > > > > 
> > > > > A target supports at lease 1 descriptor. The 'ndesc' of struct
> > > > > virtio_of_command_connect indicates the full PDU contains: struct
> > > > > virtio_of_command_connect + 1 * virtio_of_vq_desc + data.
> > > > > 
> > > > > > > +#define VIRTIO_OF_CONNECTION_TCP     1
> > > > > > > +#define VIRTIO_OF_CONNECTION_RDMA    2
> > > > > > 
> > > > > > What does RDMA mean? I thought RDMA is a general concept that several
> > > > > > fabrics implement (with different details like how addressing works).
> > > > > > 
> > > > > 
> > > > > I guest your concern is the difference of IB/RoCE/iWarp ...
> > > > > We are trying to define the payload protocol here, so I think we can ignore
> > > > > the difference of the HCA.
> > > > 
> > > > I see, maybe this could be called STREAM vs KEYED instead of TCP vs RDMA?
> > > > 
> > > 
> > > I'd like to define two PDU mapping rules(in '[PATCH v2 04/11]
> > > transport-fabrics: introduce Stream Transmission' and '[PATCH v2 05/11]
> > > transport-fabrics: introduce Keyed Transmission'): STREAM and KEYED. A
> > > transport protocols need to use one.
> > > 
> > > Then we can define protocols:
> > > #define VIRTIO_OF_CONNECTION_TCP     1		-> use STREAM
> > > #define VIRTIO_OF_CONNECTION_RDMA    2		-> use KEYED
> > > #define VIRTIO_OF_CONNECTION_TLS     3(in the future)	-> use STREAM
> > > #define VIRTIO_OF_CONNECTION_XXX
> > 
> > It's not clear to me whether TCP actually means TCP/IP or if it actually
> > means STREAM. For example, if I run Virtio Over Fabrics over AF_VSOCK,
> > would it use VIRTIO_OF_CONNECTION_TCP although there is no TCP/IP? If
> > so, then I think the name TCP is misleading and STREAM would be clearer.
> > 
> 
> What about dropping 'oftype' field from this command? When the command is
> allowed to issue, the reliable connection is already established, at this
> point, we have enough information about the connection type.
> 
> Instead, we define the multiple transports in the following section, like:
> \subsection{Transport Binding}\label{sec:Virtio Transport Options / Virtio
> Over Fabrics / Transport Binding}
> \subsubsection{TCP/IP}\label{sec:Virtio Transport Options / Virtio Over
> Fabrics / Transport Binding / TCP_IP}
> TCP/IP supports both IPv4 and IPv6, it uses \ref{sec:Virtio Transport
> Options / Virtio Over Fabrics / Transmission Protocol / Commands Definition
> / Stream Transmission}
> ~\nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission
> Protocol / Commands Definition / Stream Transmission} ...
> 
> \subsubsection{TLS-TCP/IP}\label{sec:Virtio Transport Options / Virtio Over
> Fabrics / Transport Binding / TLS-TCP_IP}
> TLS-TCP/IP supports both IPv4 and IPv6 ...
> 
> \subsubsection{RDMA}\label{sec:Virtio Transport Options / Virtio Over
> Fabrics / Transport Binding / RDMA}
> RDMA MUST use \ref{sec:Virtio Transport Options / Virtio Over Fabrics /
> Transmission Protocol / Commands Definition / Keyed Transmission}
> ~\nameref{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission
> Protocol / Commands Definition / Keyed Transmission} ...
> 
> [\subsubsection{TCP/VSOCK}\label{sec:Virtio Transport Options / Virtio Over
> Fabrics / ransport Binding / TCP_VSOCK} ...]

Sounds good. Thanks!

> 
> > > 
> > > > > 
> > > > > > > +        u8 oftype;
> > > > > > > +        u8 padding[5];
> > > > > > > +};
> > > > > > > +\end{lstlisting}
> > > > > > > +
> > > > > > > +The Connect commands MUST contains one Segment Descriptor and one structure
> > > > > > > +virtio_of_command_connect to specify Initiator VQN and Target VNQ,
> > > > > > > +virtio_of_command_connect has following structure:
> > > > > > 
> > > > > > I'm confsued. virtio_of_command_connect was defined above. The struct
> > > > > > defined below is virtio_of_connect. Does this paragraph need to be
> > > > > > updated (virtio_of_command_connect -> virtio_of_connect)?
> > > > > > 
> > > > > > Why is virtio_of_connect a separate struct and not part of
> > > > > > virtio_of_command_connect?
> > > > > > 
> > > > > 
> > > > > Because I'd like to define all the commands with a fixed length.
> > > > 
> > > > I don't understand. virtio_of_connect and virtio_of_command_connect are
> > > > both fixed-length. Why can't they be unified into 1 fixed-length struct?
> > > > 
> > > 
> > > For stream protocol, it always work fine.
> > > For keyed protocol, for example RDMA, the target side needs to use
> > > ibv_post_recv to receive a large size(sizeof virtio_of_command_connect +
> > > sizeof virtio_of_connect). If the target uses ibv_post_recv to receive
> > > sizeof(CMD) + sizeof(DESC) * 1, the initiator fails in RDMA SEND.
> > 
> > I read that "A RC connection is very similar to a TCP connection" in the
> > NVIDIA documentation
> > (https://docs.nvidia.com/networking/display/RDMAAwareProgrammingv17/Transport+Modes)
> > and expected SOCK_STREAM semantics for RDMA SEND.
> > 
> > Are you saying ibv_post_send() fails when the receiver's work request
> > sg_list size is smaller (fewer bytes) than the sender's?
> > 
> 
> Yes, it will fail.
> The receiver get a CQE with status 'IBV_WC_LOC_LEN_ERR', see
> https://www.rdmamojo.com/2013/02/15/ibv_poll_cq/

Parav: Can you confirm that this is expected?

This makes it hard to inline payloads as I was suggesting before :(.

Stefan

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