Re: [RFC v2] virtio: support packed ring

Tiwei Bie
2018-04-14T11:22:11Z

On Fri, Apr 13, 2018 at 06:22:45PM +0300, Michael S. Tsirkin wrote:
> On Sun, Apr 01, 2018 at 10:12:16PM +0800, Tiwei Bie wrote:
> > +static inline bool more_used(const struct vring_virtqueue *vq)
> > +{
> > + return vq->packed ? more_used_packed(vq) : more_used_split(vq);
> > +}
> > +
> > +void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, unsigned int *len,
> > + void **ctx)
> > +{
> > + struct vring_virtqueue *vq = to_vvq(_vq);
> > + void *ret;
> > + unsigned int i;
> > + u16 last_used;
> > +
> > + START_USE(vq);
> > +
> > + if (unlikely(vq->broken)) {
> > + END_USE(vq);
> > + return NULL;
> > + }
> > +
> > + if (!more_used(vq)) {
> > + pr_debug("No more buffers in queue\n");
> > + END_USE(vq);
> > + return NULL;
> > + }
>
> So virtqueue_get_buf_ctx_split should only call more_used_split.

Yeah, you're right! Will fix this in the next version.

>
> to avoid such issues I think we should lay out the code like this:
>
> XXX_split
>
> XXX_packed
>
> XXX wrappers

I'll do it. Thanks for the suggestion!

>
> > +/* The standard layout
>
> I'd drop standard here.

Got it. I'll drop the word "standard".

>
> > for the packed ring is a continuous chunk of memory
> > + * which looks like this.
> > + *
> > + * struct vring_packed
> > + * {
>
> Can the opening bracket go on the prev line pls?

Sure.

>
> > + * // The actual descriptors (16 bytes each)
> > + * struct vring_packed_desc desc[num];
> > + *
> > + * // Padding to the next align boundary.
> > + * char pad[];
> > + *
> > + * // Driver Event Suppression
> > + * struct vring_packed_desc_event driver;
> > + *
> > + * // Device Event Suppression
> > + * struct vring_packed_desc_event device;
>
> Maybe that's how our driver does it but it's not based on spec
> so I don't think this belongs in the header.

I will move it to the place where vring_packed_init()
is defined.

>
> > + * };
> > + */
> > +
> > +static inline unsigned vring_packed_size(unsigned int num, unsigned long align)
> > +{
> > + return ((sizeof(struct vring_packed_desc) * num + align - 1)
> > + & ~(align - 1)) + sizeof(struct vring_packed_desc_event) * 2;
> > +}
> > +
>
> Cant say this API makes sense for me.

Hmm, do you have any suggestion? Also move it out of this header?

Thanks for the review! :)

Best regards,
Tiwei Bie

>
>
> > #endif /* _UAPI_LINUX_VIRTIO_RING_H */
> > --
> > 2.11.0