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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Tue, Jun 06, 2023 at 11:13:01AM +0800, zhenwei pi wrote:
> On 6/6/23 00:11, Stefan Hajnoczi wrote:
> > On Fri, Jun 02, 2023 at 10:26:48AM +0800, zhenwei pi wrote:
> > > On 5/31/23 23:20, Stefan Hajnoczi wrote:
> > > > On Thu, May 04, 2023 at 04:19:03PM +0800, zhenwei pi wrote:
> > > > > +                  | +------+
> > > > > +                  | |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.
> > > > 
> > > 
> > > I guest this is not enough. For example, a initiator want to read 3 desc:
> > > desc0[n bytes], desc1[m bytes], desc2[1 byte]. desc[2] is expected to read a
> > > u8 status.
> > > 
> > > the target fills desc0[n - x bytes], desc1[m - y bytes], desc2[1 byte], the
> > > 'length' is (n - x + m - y + 1), we should decode each descriptor and fill
> > > the driver buffer correctly.(otherwise, if x + y > 0, desc[2] is never
> > > filled)
> > 
> > No, the framing really doesn't matter - that's what the spec says, after
> > all. The framing could be [n, m, 1] like in your example or [1, 1, n-2,
> > m-1, 1, 1], both are valid. What matters is that the device knows at
> > which offset the 1-byte status field must be written.
> > 
> > It is the VIRTIO specification that determines how to find the offset,
> > not the framing of the virtqueue buffer elements. (Again, the spec
> > explicitly forbids depending on framing.)
> > 
> > In other words, the virtio-blk spec says that the status byte is the
> > last writeable byte and that's how the device knows the offset. The
> > framing doesn't matter.
> > 
> > > > 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.
> > > 
> > > Just keep the flags same to the descriptor from the command, give the
> > > initiator a hint 'this is a read descriptor'.
> > 
> > Sending virtqueue element information across the wire seems inefficient
> > to me. I think the protocol can be optimized for stream (TCP) and keyed
> > (RDMA) fabrics by omitting aspects that are not strictly needed.
> > 
> > Stefan
> 
> Got it, thanks! By the way, for both command and completion, the descriptors
> are not necessary? A command like:
> struct virtio_of_command_vq {
>         le16 opcode;
>         le16 command_id;
>         le32 out_length;
>         le32 in_length;
>         u8 rsvd[4];
> };
> 
> This seems enough ...

Yes.

Stefan

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