Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:30:02PM +0200, Michael S. Tsirkin wrote:
> On Fri, Mar 16, 2018 at 07:36:47PM +0800, Jason Wang wrote:
> > > > @@ -1096,17 +1599,21 @@ struct virtqueue *vring_create_virtqueue(
> > > > > > > > >   if (!queue) {
> > > > > > > > >   /* Try to get a single page. You are my only 
> > > > > > > > > hope! */
> > > > > > > > > - queue = vring_alloc_queue(vdev, vring_size(num, 
> > > > > > > > > vring_align),
> > > > > > > > > + queue = vring_alloc_queue(vdev, 
> > > > > > > > > __vring_size(num, vring_align,
> > > > > > > > > +  
> > > > > > > > > packed),
> > > > > > > > > _addr, 
> > > > > > > > > GFP_KERNEL|__GFP_ZERO);
> > > > > > > > >   }
> > > > > > > > >   if (!queue)
> > > > > > > > >   return NULL;
> > > > > > > > > - queue_size_in_bytes = vring_size(num, vring_align);
> > > > > > > > > - vring_init(, num, queue, vring_align);
> > > > > > > > > + queue_size_in_bytes = __vring_size(num, vring_align, 
> > > > > > > > > packed);
> > > > > > > > > + if (packed)
> > > > > > > > > + vring_packed_init(_packed, num, 
> > > > > > > > > queue, vring_align);
> > > > > > > > > + else
> > > > > > > > > + vring_init(_split, num, queue, 
> > > > > > > > > vring_align);
> > > > > > > > Let's rename vring_init to vring_init_split() like other 
> > > > > > > > helpers?
> > > > > > > The vring_init() is a public API in 
> > > > > > > include/uapi/linux/virtio_ring.h.
> > > > > > > I don't think we can rename it.
> > > > > > I see, then this need more thoughts to unify the API.
> > > > > My thought is to keep the old API as is, and introduce
> > > > > new types and helpers for packed ring.
> > > > I admit it's not a fault of this patch. But we'd better think of this 
> > > > in the
> > > > future, consider we may have new kinds of ring.
> > > > 
> > > > > More details can be found in this patch:
> > > > > https://lkml.org/lkml/2018/2/23/243
> > > > > (PS. The type which has bit fields is just for reference,
> > > > >and will be changed in next version.)
> > > > > 
> > > > > Do you have any other suggestions?
> > > > No.
> > > Hmm.. Sorry, I didn't describe my question well.
> > > I mean do you have any suggestions about the API
> > > design for packed ring in uapi header? Currently
> > > I introduced below two new helpers:
> > > 
> > > static inline void vring_packed_init(struct vring_packed *vr, unsigned 
> > > int num,
> > >void *p, unsigned long align);
> > > static inline unsigned vring_packed_size(unsigned int num, unsigned long 
> > > align);
> > > 
> > > When new rings are introduced in the future, above
> > > helpers can't be reused. Maybe we should make the
> > > helpers be able to determine the ring type?
> > 
> > Let's wait for Michael's comment here. Generally, I fail to understand why
> > vring_init() become a part of uapi. Git grep shows the only use cases are
> > virtio_test/vringh_test.
> > 
> > Thanks
> 
> For init - I think it's a mistake that stems from lguest which sometimes
> made it less than obvious which code is where.  I don't see a reason to
> add to it.

Got it! I'll move vring_packed_init() out of uapi. Many thanks! :)

Best regards,
Tiwei Bie

> 
> -- 
> MST

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Tiwei Bie]

On Fri, Mar 16, 2018 at 07:36:47PM +0800, Jason Wang wrote:
> On 2018年03月16日 18:04, Tiwei Bie wrote:
> > On Fri, Mar 16, 2018 at 04:34:28PM +0800, Jason Wang wrote:
> > > On 2018年03月16日 15:40, Tiwei Bie wrote:
> > > > On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote:
> > > > > On 2018年03月16日 14:10, Tiwei Bie wrote:
> > > > > > On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote:
> > > > > > > On 2018年02月23日 19:18, Tiwei Bie wrote:
> > > > > > > > Signed-off-by: Tiwei Bie 
> > > > > > > > ---
> > > > > > > >  drivers/virtio/virtio_ring.c | 699 
> > > > > > > > +--
> > > > > > > >  include/linux/virtio_ring.h  |   8 +-
> > > > > > > >  2 files changed, 618 insertions(+), 89 deletions(-)
[...]
> > > @@ -1096,17 +1599,21 @@ struct virtqueue *vring_create_virtqueue(
> > > > > > > > if (!queue) {
> > > > > > > > /* Try to get a single page. You are my only 
> > > > > > > > hope! */
> > > > > > > > -   queue = vring_alloc_queue(vdev, vring_size(num, 
> > > > > > > > vring_align),
> > > > > > > > +   queue = vring_alloc_queue(vdev, 
> > > > > > > > __vring_size(num, vring_align,
> > > > > > > > +
> > > > > > > > packed),
> > > > > > > >   _addr, 
> > > > > > > > GFP_KERNEL|__GFP_ZERO);
> > > > > > > > }
> > > > > > > > if (!queue)
> > > > > > > > return NULL;
> > > > > > > > -   queue_size_in_bytes = vring_size(num, vring_align);
> > > > > > > > -   vring_init(, num, queue, vring_align);
> > > > > > > > +   queue_size_in_bytes = __vring_size(num, vring_align, 
> > > > > > > > packed);
> > > > > > > > +   if (packed)
> > > > > > > > +   vring_packed_init(_packed, num, 
> > > > > > > > queue, vring_align);
> > > > > > > > +   else
> > > > > > > > +   vring_init(_split, num, queue, 
> > > > > > > > vring_align);
> > > > > > > Let's rename vring_init to vring_init_split() like other helpers?
> > > > > > The vring_init() is a public API in 
> > > > > > include/uapi/linux/virtio_ring.h.
> > > > > > I don't think we can rename it.
> > > > > I see, then this need more thoughts to unify the API.
> > > > My thought is to keep the old API as is, and introduce
> > > > new types and helpers for packed ring.
> > > I admit it's not a fault of this patch. But we'd better think of this in 
> > > the
> > > future, consider we may have new kinds of ring.
> > > 
> > > > More details can be found in this patch:
> > > > https://lkml.org/lkml/2018/2/23/243
> > > > (PS. The type which has bit fields is just for reference,
> > > >and will be changed in next version.)
> > > > 
> > > > Do you have any other suggestions?
> > > No.
> > Hmm.. Sorry, I didn't describe my question well.
> > I mean do you have any suggestions about the API
> > design for packed ring in uapi header? Currently
> > I introduced below two new helpers:
> > 
> > static inline void vring_packed_init(struct vring_packed *vr, unsigned int 
> > num,
> >  void *p, unsigned long align);
> > static inline unsigned vring_packed_size(unsigned int num, unsigned long 
> > align);
> > 
> > When new rings are introduced in the future, above
> > helpers can't be reused. Maybe we should make the
> > helpers be able to determine the ring type?
> 
> Let's wait for Michael's comment here. Generally, I fail to understand why
> vring_init() become a part of uapi. Git grep shows the only use cases are
> virtio_test/vringh_test.

Thank you very much for the review on this patch!
I'll send out a new version ASAP to address these
comments. :)

Best regards,
Tiwei Bie

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Michael S. Tsirkin]

On Fri, Mar 16, 2018 at 07:36:47PM +0800, Jason Wang wrote:
> > > @@ -1096,17 +1599,21 @@ struct virtqueue *vring_create_virtqueue(
> > > > > > > > if (!queue) {
> > > > > > > > /* Try to get a single page. You are my only 
> > > > > > > > hope! */
> > > > > > > > -   queue = vring_alloc_queue(vdev, vring_size(num, 
> > > > > > > > vring_align),
> > > > > > > > +   queue = vring_alloc_queue(vdev, 
> > > > > > > > __vring_size(num, vring_align,
> > > > > > > > +
> > > > > > > > packed),
> > > > > > > >   _addr, 
> > > > > > > > GFP_KERNEL|__GFP_ZERO);
> > > > > > > > }
> > > > > > > > if (!queue)
> > > > > > > > return NULL;
> > > > > > > > -   queue_size_in_bytes = vring_size(num, vring_align);
> > > > > > > > -   vring_init(, num, queue, vring_align);
> > > > > > > > +   queue_size_in_bytes = __vring_size(num, vring_align, 
> > > > > > > > packed);
> > > > > > > > +   if (packed)
> > > > > > > > +   vring_packed_init(_packed, num, 
> > > > > > > > queue, vring_align);
> > > > > > > > +   else
> > > > > > > > +   vring_init(_split, num, queue, 
> > > > > > > > vring_align);
> > > > > > > Let's rename vring_init to vring_init_split() like other helpers?
> > > > > > The vring_init() is a public API in 
> > > > > > include/uapi/linux/virtio_ring.h.
> > > > > > I don't think we can rename it.
> > > > > I see, then this need more thoughts to unify the API.
> > > > My thought is to keep the old API as is, and introduce
> > > > new types and helpers for packed ring.
> > > I admit it's not a fault of this patch. But we'd better think of this in 
> > > the
> > > future, consider we may have new kinds of ring.
> > > 
> > > > More details can be found in this patch:
> > > > https://lkml.org/lkml/2018/2/23/243
> > > > (PS. The type which has bit fields is just for reference,
> > > >and will be changed in next version.)
> > > > 
> > > > Do you have any other suggestions?
> > > No.
> > Hmm.. Sorry, I didn't describe my question well.
> > I mean do you have any suggestions about the API
> > design for packed ring in uapi header? Currently
> > I introduced below two new helpers:
> > 
> > static inline void vring_packed_init(struct vring_packed *vr, unsigned int 
> > num,
> >  void *p, unsigned long align);
> > static inline unsigned vring_packed_size(unsigned int num, unsigned long 
> > align);
> > 
> > When new rings are introduced in the future, above
> > helpers can't be reused. Maybe we should make the
> > helpers be able to determine the ring type?
> 
> Let's wait for Michael's comment here. Generally, I fail to understand why
> vring_init() become a part of uapi. Git grep shows the only use cases are
> virtio_test/vringh_test.
> 
> Thanks

For init - I think it's a mistake that stems from lguest which sometimes
made it less than obvious which code is where.  I don't see a reason to
add to it.

-- 
MST

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Jason Wang]

On 2018年03月16日 18:04, Tiwei Bie wrote:

On Fri, Mar 16, 2018 at 04:34:28PM +0800, Jason Wang wrote:

On 2018年03月16日 15:40, Tiwei Bie wrote:

On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote:

On 2018年03月16日 14:10, Tiwei Bie wrote:

On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote:

On 2018年02月23日 19:18, Tiwei Bie wrote:

Signed-off-by: Tiwei Bie 
---
 drivers/virtio/virtio_ring.c | 699 
+--
 include/linux/virtio_ring.h  |   8 +-
 2 files changed, 618 insertions(+), 89 deletions(-)




[...]


+   }
+   }
+   for (; n < (out_sgs + in_sgs); n++) {
+   for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+   dma_addr_t addr = vring_map_one_sg(vq, sg, 
DMA_FROM_DEVICE);
+   if (vring_mapping_error(vq, addr))
+   goto unmap_release;
+
+   flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT |
+   VRING_DESC_F_WRITE |
+   VRING_DESC_F_AVAIL(vq->wrap_counter) |
+   VRING_DESC_F_USED(!vq->wrap_counter));
+   if (!indirect && i == head)
+   head_flags = flags;
+   else
+   desc[i].flags = flags;
+
+   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
+   desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
+   desc[i].id = cpu_to_virtio32(_vq->vdev, head);
+   prev = i;
+   i++;
+   if (!indirect && i >= vq->vring_packed.num) {
+   i = 0;
+   vq->wrap_counter ^= 1;
+   }
+   }
+   }
+   /* Last one doesn't continue. */
+   if (!indirect && (head + 1) % vq->vring_packed.num == i)
+   head_flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);

I can't get the why we need this here.

If only one desc is used, we will need to clear the
VRING_DESC_F_NEXT flag from the head_flags.

Yes, I meant why following desc[prev].flags won't work for this?

Because the update of desc[head].flags (in above case,
prev == head) has been delayed. The flags is saved in
head_flags.

Ok, but let's try to avoid modular here e.g tracking the number of sgs in a
counter.

And I see lots of duplication in the above two loops, I believe we can unify
them with a a single loop. the only difference is dma direction and write
flag.

The above implementation for packed ring is basically
an mirror of the existing implementation in split ring
as I want to keep the coding style consistent. Below
is the corresponding code in split ring:

static inline int virtqueue_add(struct virtqueue *_vq,
struct scatterlist *sgs[],
unsigned int total_sg,
unsigned int out_sgs,
unsigned int in_sgs,
void *data,
void *ctx,
gfp_t gfp)
{
..

for (n = 0; n < out_sgs; n++) {
for (sg = sgs[n]; sg; sg = sg_next(sg)) {
dma_addr_t addr = vring_map_one_sg(vq, sg, 
DMA_TO_DEVICE);
if (vring_mapping_error(vq, addr))
goto unmap_release;

desc[i].flags = cpu_to_virtio16(_vq->vdev, 
VRING_DESC_F_NEXT);
desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
prev = i;
i = virtio16_to_cpu(_vq->vdev, desc[i].next);
}
}
for (; n < (out_sgs + in_sgs); n++) {
for (sg = sgs[n]; sg; sg = sg_next(sg)) {
dma_addr_t addr = vring_map_one_sg(vq, sg, 
DMA_FROM_DEVICE);
if (vring_mapping_error(vq, addr))
goto unmap_release;

desc[i].flags = cpu_to_virtio16(_vq->vdev, 
VRING_DESC_F_NEXT | VRING_DESC_F_WRITE);
desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
prev = i;
i = virtio16_to_cpu(_vq->vdev, desc[i].next);
}
}

..
}


There's no need for such consistency especially consider it's a new kind 
of ring. Anyway, you can stick to it.


[...]


+   } else
+   vq->free_head = i;

ID is only valid in the last descriptor in the list, so head + 1 should be
ok too?

I don't really get your point. The vq->free_head stores
the 

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:34:28PM +0800, Jason Wang wrote:
> On 2018年03月16日 15:40, Tiwei Bie wrote:
> > On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote:
> > > On 2018年03月16日 14:10, Tiwei Bie wrote:
> > > > On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote:
> > > > > On 2018年02月23日 19:18, Tiwei Bie wrote:
> > > > > > Signed-off-by: Tiwei Bie 
> > > > > > ---
> > > > > > drivers/virtio/virtio_ring.c | 699 
> > > > > > +--
> > > > > > include/linux/virtio_ring.h  |   8 +-
> > > > > > 2 files changed, 618 insertions(+), 89 deletions(-)
> > [...]
> > > > > >   cpu_addr, size, direction);
> > > > > > }
> > > > > > -static void vring_unmap_one(const struct vring_virtqueue *vq,
> > > > > > -   struct vring_desc *desc)
> > > > > > +static void vring_unmap_one(const struct vring_virtqueue *vq, void 
> > > > > > *_desc)
> > > > > > {
> > > > > Let's split the helpers to packed/split version like other helpers?
> > > > > (Consider the caller has already known the type of vq).
> > > > Okay.
> > > > 
> > > [...]
> > > 
> > > > > > +   desc[i].flags = flags;
> > > > > > +
> > > > > > +   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
> > > > > > +   desc[i].len = cpu_to_virtio32(_vq->vdev, 
> > > > > > sg->length);
> > > > > > +   desc[i].id = cpu_to_virtio32(_vq->vdev, head);
> > > > > If it's a part of chain, we only need to do this for last buffer I 
> > > > > think.
> > > > I'm not sure I've got your point about the "last buffer".
> > > > But, yes, id just needs to be set for the last desc.
> > > Right, I think I meant "last descriptor" :)
> > > 
> > > > > > +   prev = i;
> > > > > > +   i++;
> > > > > It looks to me prev is always i - 1?
> > > > No. prev will be (vq->vring_packed.num - 1) when i becomes 0.
> > > Right, so prev = i ? i - 1 : vq->vring_packed.num - 1.
> > Yes, i wraps together with vq->wrap_counter in following code:
> > 
> > > > > > +   if (!indirect && i >= vq->vring_packed.num) {
> > > > > > +   i = 0;
> > > > > > +   vq->wrap_counter ^= 1;
> > > > > > +   }
> > 
> > > > > > +   }
> > > > > > +   }
> > > > > > +   for (; n < (out_sgs + in_sgs); n++) {
> > > > > > +   for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > > > > > +   dma_addr_t addr = vring_map_one_sg(vq, sg, 
> > > > > > DMA_FROM_DEVICE);
> > > > > > +   if (vring_mapping_error(vq, addr))
> > > > > > +   goto unmap_release;
> > > > > > +
> > > > > > +   flags = cpu_to_virtio16(_vq->vdev, 
> > > > > > VRING_DESC_F_NEXT |
> > > > > > +   VRING_DESC_F_WRITE |
> > > > > > +   
> > > > > > VRING_DESC_F_AVAIL(vq->wrap_counter) |
> > > > > > +   
> > > > > > VRING_DESC_F_USED(!vq->wrap_counter));
> > > > > > +   if (!indirect && i == head)
> > > > > > +   head_flags = flags;
> > > > > > +   else
> > > > > > +   desc[i].flags = flags;
> > > > > > +
> > > > > > +   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
> > > > > > +   desc[i].len = cpu_to_virtio32(_vq->vdev, 
> > > > > > sg->length);
> > > > > > +   desc[i].id = cpu_to_virtio32(_vq->vdev, head);
> > > > > > +   prev = i;
> > > > > > +   i++;
> > > > > > +   if (!indirect && i >= vq->vring_packed.num) {
> > > > > > +   i = 0;
> > > > > > +   vq->wrap_counter ^= 1;
> > > > > > +   }
> > > > > > +   }
> > > > > > +   }
> > > > > > +   /* Last one doesn't continue. */
> > > > > > +   if (!indirect && (head + 1) % vq->vring_packed.num == i)
> > > > > > +   head_flags &= cpu_to_virtio16(_vq->vdev, 
> > > > > > ~VRING_DESC_F_NEXT);
> > > > > I can't get the why we need this here.
> > > > If only one desc is used, we will need to clear the
> > > > VRING_DESC_F_NEXT flag from the head_flags.
> > > Yes, I meant why following desc[prev].flags won't work for this?
> > Because the update of desc[head].flags (in above case,
> > prev == head) has been delayed. The flags is saved in
> > head_flags.
> 
> Ok, but let's try to avoid modular here e.g tracking the number of sgs in a
> counter.
> 
> And I see lots of duplication in the above two loops, I believe we can unify
> them with a a single loop. the only difference is dma direction and write
> flag.

The above implementation for packed ring is basically
an mirror of the existing implementation in split ring
as I want to keep the coding style consistent. Below
is the corresponding code 

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Jason Wang]

On 2018年03月16日 15:40, Tiwei Bie wrote:

On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote:

On 2018年03月16日 14:10, Tiwei Bie wrote:

On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote:

On 2018年02月23日 19:18, Tiwei Bie wrote:

Signed-off-by: Tiwei Bie 
---
drivers/virtio/virtio_ring.c | 699 
+--
include/linux/virtio_ring.h  |   8 +-
2 files changed, 618 insertions(+), 89 deletions(-)

[...]

  cpu_addr, size, direction);
}
-static void vring_unmap_one(const struct vring_virtqueue *vq,
-   struct vring_desc *desc)
+static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc)
{

Let's split the helpers to packed/split version like other helpers?
(Consider the caller has already known the type of vq).

Okay.


[...]


+   desc[i].flags = flags;
+
+   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
+   desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
+   desc[i].id = cpu_to_virtio32(_vq->vdev, head);

If it's a part of chain, we only need to do this for last buffer I think.

I'm not sure I've got your point about the "last buffer".
But, yes, id just needs to be set for the last desc.

Right, I think I meant "last descriptor" :)


+   prev = i;
+   i++;

It looks to me prev is always i - 1?

No. prev will be (vq->vring_packed.num - 1) when i becomes 0.

Right, so prev = i ? i - 1 : vq->vring_packed.num - 1.

Yes, i wraps together with vq->wrap_counter in following code:


+   if (!indirect && i >= vq->vring_packed.num) {
+   i = 0;
+   vq->wrap_counter ^= 1;
+   }



+   }
+   }
+   for (; n < (out_sgs + in_sgs); n++) {
+   for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+   dma_addr_t addr = vring_map_one_sg(vq, sg, 
DMA_FROM_DEVICE);
+   if (vring_mapping_error(vq, addr))
+   goto unmap_release;
+
+   flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT |
+   VRING_DESC_F_WRITE |
+   VRING_DESC_F_AVAIL(vq->wrap_counter) |
+   VRING_DESC_F_USED(!vq->wrap_counter));
+   if (!indirect && i == head)
+   head_flags = flags;
+   else
+   desc[i].flags = flags;
+
+   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
+   desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
+   desc[i].id = cpu_to_virtio32(_vq->vdev, head);
+   prev = i;
+   i++;
+   if (!indirect && i >= vq->vring_packed.num) {
+   i = 0;
+   vq->wrap_counter ^= 1;
+   }
+   }
+   }
+   /* Last one doesn't continue. */
+   if (!indirect && (head + 1) % vq->vring_packed.num == i)
+   head_flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);

I can't get the why we need this here.

If only one desc is used, we will need to clear the
VRING_DESC_F_NEXT flag from the head_flags.

Yes, I meant why following desc[prev].flags won't work for this?

Because the update of desc[head].flags (in above case,
prev == head) has been delayed. The flags is saved in
head_flags.


Ok, but let's try to avoid modular here e.g tracking the number of sgs 
in a counter.


And I see lots of duplication in the above two loops, I believe we can 
unify them with a a single loop. the only difference is dma direction 
and write flag.





+   else
+   desc[prev].flags &= cpu_to_virtio16(_vq->vdev, 
~VRING_DESC_F_NEXT);
+
+   if (indirect) {
+   /* FIXME: to be implemented */
+
+   /* Now that the indirect table is filled in, map it. */
+   dma_addr_t addr = vring_map_single(
+   vq, desc, total_sg * sizeof(struct vring_packed_desc),
+   DMA_TO_DEVICE);
+   if (vring_mapping_error(vq, addr))
+   goto unmap_release;
+
+   head_flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_INDIRECT |
+VRING_DESC_F_AVAIL(wrap_counter) |
+VRING_DESC_F_USED(!wrap_counter));
+   vq->vring_packed.desc[head].addr = cpu_to_virtio64(_vq->vdev, 
addr);
+   vq->vring_packed.desc[head].len = cpu_to_virtio32(_vq->vdev,
+   total_sg * sizeof(struct vring_packed_desc));
+   

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Tiwei Bie]

On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote:
> On 2018年03月16日 14:10, Tiwei Bie wrote:
> > On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote:
> > > On 2018年02月23日 19:18, Tiwei Bie wrote:
> > > > Signed-off-by: Tiwei Bie 
> > > > ---
> > > >drivers/virtio/virtio_ring.c | 699 
> > > > +--
> > > >include/linux/virtio_ring.h  |   8 +-
> > > >2 files changed, 618 insertions(+), 89 deletions(-)
[...]
> > > >   cpu_addr, size, direction);
> > > >}
> > > > -static void vring_unmap_one(const struct vring_virtqueue *vq,
> > > > -   struct vring_desc *desc)
> > > > +static void vring_unmap_one(const struct vring_virtqueue *vq, void 
> > > > *_desc)
> > > >{
> > > Let's split the helpers to packed/split version like other helpers?
> > > (Consider the caller has already known the type of vq).
> > Okay.
> > 
> 
> [...]
> 
> > > > +   desc[i].flags = flags;
> > > > +
> > > > +   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
> > > > +   desc[i].len = cpu_to_virtio32(_vq->vdev, 
> > > > sg->length);
> > > > +   desc[i].id = cpu_to_virtio32(_vq->vdev, head);
> > > If it's a part of chain, we only need to do this for last buffer I think.
> > I'm not sure I've got your point about the "last buffer".
> > But, yes, id just needs to be set for the last desc.
> 
> Right, I think I meant "last descriptor" :)
> 
> > 
> > > > +   prev = i;
> > > > +   i++;
> > > It looks to me prev is always i - 1?
> > No. prev will be (vq->vring_packed.num - 1) when i becomes 0.
> 
> Right, so prev = i ? i - 1 : vq->vring_packed.num - 1.

Yes, i wraps together with vq->wrap_counter in following code:

> > > > +   if (!indirect && i >= vq->vring_packed.num) {
> > > > +   i = 0;
> > > > +   vq->wrap_counter ^= 1;
> > > > +   }


> > > > +   }
> > > > +   }
> > > > +   for (; n < (out_sgs + in_sgs); n++) {
> > > > +   for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > > > +   dma_addr_t addr = vring_map_one_sg(vq, sg, 
> > > > DMA_FROM_DEVICE);
> > > > +   if (vring_mapping_error(vq, addr))
> > > > +   goto unmap_release;
> > > > +
> > > > +   flags = cpu_to_virtio16(_vq->vdev, 
> > > > VRING_DESC_F_NEXT |
> > > > +   VRING_DESC_F_WRITE |
> > > > +   
> > > > VRING_DESC_F_AVAIL(vq->wrap_counter) |
> > > > +   
> > > > VRING_DESC_F_USED(!vq->wrap_counter));
> > > > +   if (!indirect && i == head)
> > > > +   head_flags = flags;
> > > > +   else
> > > > +   desc[i].flags = flags;
> > > > +
> > > > +   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
> > > > +   desc[i].len = cpu_to_virtio32(_vq->vdev, 
> > > > sg->length);
> > > > +   desc[i].id = cpu_to_virtio32(_vq->vdev, head);
> > > > +   prev = i;
> > > > +   i++;
> > > > +   if (!indirect && i >= vq->vring_packed.num) {
> > > > +   i = 0;
> > > > +   vq->wrap_counter ^= 1;
> > > > +   }
> > > > +   }
> > > > +   }
> > > > +   /* Last one doesn't continue. */
> > > > +   if (!indirect && (head + 1) % vq->vring_packed.num == i)
> > > > +   head_flags &= cpu_to_virtio16(_vq->vdev, 
> > > > ~VRING_DESC_F_NEXT);
> > > I can't get the why we need this here.
> > If only one desc is used, we will need to clear the
> > VRING_DESC_F_NEXT flag from the head_flags.
> 
> Yes, I meant why following desc[prev].flags won't work for this?

Because the update of desc[head].flags (in above case,
prev == head) has been delayed. The flags is saved in
head_flags.

> 
> > 
> > > > +   else
> > > > +   desc[prev].flags &= cpu_to_virtio16(_vq->vdev, 
> > > > ~VRING_DESC_F_NEXT);
> > > > +
> > > > +   if (indirect) {
> > > > +   /* FIXME: to be implemented */
> > > > +
> > > > +   /* Now that the indirect table is filled in, map it. */
> > > > +   dma_addr_t addr = vring_map_single(
> > > > +   vq, desc, total_sg * sizeof(struct 
> > > > vring_packed_desc),
> > > > +   DMA_TO_DEVICE);
> > > > +   if (vring_mapping_error(vq, addr))
> > > > +   goto unmap_release;
> > > > +
> > > > +   head_flags = cpu_to_virtio16(_vq->vdev, 
> > > > VRING_DESC_F_INDIRECT |
> > > > + 

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Jason Wang]

On 2018年03月16日 14:10, Tiwei Bie wrote:

On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote:

On 2018年02月23日 19:18, Tiwei Bie wrote:

Signed-off-by: Tiwei Bie 
---
   drivers/virtio/virtio_ring.c | 699 
+--
   include/linux/virtio_ring.h  |   8 +-
   2 files changed, 618 insertions(+), 89 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index eb30f3e09a47..393778a2f809 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -58,14 +58,14 @@
   struct vring_desc_state {
void *data; /* Data for callback. */
-   struct vring_desc *indir_desc;  /* Indirect descriptor, if any. */
+   void *indir_desc;   /* Indirect descriptor, if any. */
+   int num;/* Descriptor list length. */
   };
   struct vring_virtqueue {
struct virtqueue vq;
-   /* Actual memory layout for this queue */
-   struct vring vring;
+   bool packed;
/* Can we use weak barriers? */
bool weak_barriers;
@@ -87,11 +87,28 @@ struct vring_virtqueue {
/* Last used index we've seen. */
u16 last_used_idx;
-   /* Last written value to avail->flags */
-   u16 avail_flags_shadow;
-
-   /* Last written value to avail->idx in guest byte order */
-   u16 avail_idx_shadow;
+   union {
+   /* Available for split ring */
+   struct {
+   /* Actual memory layout for this queue */
+   struct vring vring;
+
+   /* Last written value to avail->flags */
+   u16 avail_flags_shadow;
+
+   /* Last written value to avail->idx in
+* guest byte order */
+   u16 avail_idx_shadow;
+   };
+
+   /* Available for packed ring */
+   struct {
+   /* Actual memory layout for this queue */
+   struct vring_packed vring_packed;
+   u8 wrap_counter : 1;
+   bool chaining;
+   };
+   };
/* How to notify other side. FIXME: commonalize hcalls! */
bool (*notify)(struct virtqueue *vq);
@@ -201,26 +218,37 @@ static dma_addr_t vring_map_single(const struct 
vring_virtqueue *vq,
  cpu_addr, size, direction);
   }
-static void vring_unmap_one(const struct vring_virtqueue *vq,
-   struct vring_desc *desc)
+static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc)
   {

Let's split the helpers to packed/split version like other helpers?
(Consider the caller has already known the type of vq).

Okay.



[...]


+   desc[i].flags = flags;
+
+   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
+   desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
+   desc[i].id = cpu_to_virtio32(_vq->vdev, head);

If it's a part of chain, we only need to do this for last buffer I think.

I'm not sure I've got your point about the "last buffer".
But, yes, id just needs to be set for the last desc.


Right, I think I meant "last descriptor" :)




+   prev = i;
+   i++;

It looks to me prev is always i - 1?

No. prev will be (vq->vring_packed.num - 1) when i becomes 0.


Right, so prev = i ? i - 1 : vq->vring_packed.num - 1.




+   if (!indirect && i >= vq->vring_packed.num) {
+   i = 0;
+   vq->wrap_counter ^= 1;
+   }
+   }
+   }
+   for (; n < (out_sgs + in_sgs); n++) {
+   for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+   dma_addr_t addr = vring_map_one_sg(vq, sg, 
DMA_FROM_DEVICE);
+   if (vring_mapping_error(vq, addr))
+   goto unmap_release;
+
+   flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT |
+   VRING_DESC_F_WRITE |
+   VRING_DESC_F_AVAIL(vq->wrap_counter) |
+   VRING_DESC_F_USED(!vq->wrap_counter));
+   if (!indirect && i == head)
+   head_flags = flags;
+   else
+   desc[i].flags = flags;
+
+   desc[i].addr = cpu_to_virtio64(_vq->vdev, addr);
+   desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
+   desc[i].id = cpu_to_virtio32(_vq->vdev, head);
+   prev = i;
+   i++;
+   if (!indirect && i >= vq->vring_packed.num) {
+   i = 0;
+   

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Tiwei Bie]

On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote:
> On 2018年02月23日 19:18, Tiwei Bie wrote:
> > Signed-off-by: Tiwei Bie 
> > ---
> >   drivers/virtio/virtio_ring.c | 699 
> > +--
> >   include/linux/virtio_ring.h  |   8 +-
> >   2 files changed, 618 insertions(+), 89 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index eb30f3e09a47..393778a2f809 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -58,14 +58,14 @@
> >   struct vring_desc_state {
> > void *data; /* Data for callback. */
> > -   struct vring_desc *indir_desc;  /* Indirect descriptor, if any. */
> > +   void *indir_desc;   /* Indirect descriptor, if any. */
> > +   int num;/* Descriptor list length. */
> >   };
> >   struct vring_virtqueue {
> > struct virtqueue vq;
> > -   /* Actual memory layout for this queue */
> > -   struct vring vring;
> > +   bool packed;
> > /* Can we use weak barriers? */
> > bool weak_barriers;
> > @@ -87,11 +87,28 @@ struct vring_virtqueue {
> > /* Last used index we've seen. */
> > u16 last_used_idx;
> > -   /* Last written value to avail->flags */
> > -   u16 avail_flags_shadow;
> > -
> > -   /* Last written value to avail->idx in guest byte order */
> > -   u16 avail_idx_shadow;
> > +   union {
> > +   /* Available for split ring */
> > +   struct {
> > +   /* Actual memory layout for this queue */
> > +   struct vring vring;
> > +
> > +   /* Last written value to avail->flags */
> > +   u16 avail_flags_shadow;
> > +
> > +   /* Last written value to avail->idx in
> > +* guest byte order */
> > +   u16 avail_idx_shadow;
> > +   };
> > +
> > +   /* Available for packed ring */
> > +   struct {
> > +   /* Actual memory layout for this queue */
> > +   struct vring_packed vring_packed;
> > +   u8 wrap_counter : 1;
> > +   bool chaining;
> > +   };
> > +   };
> > /* How to notify other side. FIXME: commonalize hcalls! */
> > bool (*notify)(struct virtqueue *vq);
> > @@ -201,26 +218,37 @@ static dma_addr_t vring_map_single(const struct 
> > vring_virtqueue *vq,
> >   cpu_addr, size, direction);
> >   }
> > -static void vring_unmap_one(const struct vring_virtqueue *vq,
> > -   struct vring_desc *desc)
> > +static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc)
> >   {
> 
> Let's split the helpers to packed/split version like other helpers?
> (Consider the caller has already known the type of vq).

Okay.

> 
> > +   u64 addr;
> > +   u32 len;
> > u16 flags;
> > if (!vring_use_dma_api(vq->vq.vdev))
> > return;
> > -   flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
> > +   if (vq->packed) {
> > +   struct vring_packed_desc *desc = _desc;
> > +
> > +   addr = virtio64_to_cpu(vq->vq.vdev, desc->addr);
> > +   len = virtio32_to_cpu(vq->vq.vdev, desc->len);
> > +   flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
> > +   } else {
> > +   struct vring_desc *desc = _desc;
> > +
> > +   addr = virtio64_to_cpu(vq->vq.vdev, desc->addr);
> > +   len = virtio32_to_cpu(vq->vq.vdev, desc->len);
> > +   flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
> > +   }
> > if (flags & VRING_DESC_F_INDIRECT) {
> > dma_unmap_single(vring_dma_dev(vq),
> > -virtio64_to_cpu(vq->vq.vdev, desc->addr),
> > -virtio32_to_cpu(vq->vq.vdev, desc->len),
> > +addr, len,
> >  (flags & VRING_DESC_F_WRITE) ?
> >  DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > } else {
> > dma_unmap_page(vring_dma_dev(vq),
> > -  virtio64_to_cpu(vq->vq.vdev, desc->addr),
> > -  virtio32_to_cpu(vq->vq.vdev, desc->len),
> > +  addr, len,
> >(flags & VRING_DESC_F_WRITE) ?
> >DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > }
> > @@ -235,8 +263,9 @@ static int vring_mapping_error(const struct 
> > vring_virtqueue *vq,
> > return dma_mapping_error(vring_dma_dev(vq), addr);
> >   }
> > -static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
> > -unsigned int total_sg, gfp_t gfp)
> > +static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,
> > +  unsigned int total_sg,
> > +  gfp_t gfp)
> >   {
> > struct vring_desc *desc;
> > unsigned 

Re: [PATCH RFC 2/2] virtio_ring: support packed ring

[Jason Wang]

On 2018年02月23日 19:18, Tiwei Bie wrote:

Signed-off-by: Tiwei Bie 
---
  drivers/virtio/virtio_ring.c | 699 +--
  include/linux/virtio_ring.h  |   8 +-
  2 files changed, 618 insertions(+), 89 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index eb30f3e09a47..393778a2f809 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -58,14 +58,14 @@
  
  struct vring_desc_state {

void *data; /* Data for callback. */
-   struct vring_desc *indir_desc;  /* Indirect descriptor, if any. */
+   void *indir_desc;   /* Indirect descriptor, if any. */
+   int num;/* Descriptor list length. */
  };
  
  struct vring_virtqueue {

struct virtqueue vq;
  
-	/* Actual memory layout for this queue */

-   struct vring vring;
+   bool packed;
  
  	/* Can we use weak barriers? */

bool weak_barriers;
@@ -87,11 +87,28 @@ struct vring_virtqueue {
/* Last used index we've seen. */
u16 last_used_idx;
  
-	/* Last written value to avail->flags */

-   u16 avail_flags_shadow;
-
-   /* Last written value to avail->idx in guest byte order */
-   u16 avail_idx_shadow;
+   union {
+   /* Available for split ring */
+   struct {
+   /* Actual memory layout for this queue */
+   struct vring vring;
+
+   /* Last written value to avail->flags */
+   u16 avail_flags_shadow;
+
+   /* Last written value to avail->idx in
+* guest byte order */
+   u16 avail_idx_shadow;
+   };
+
+   /* Available for packed ring */
+   struct {
+   /* Actual memory layout for this queue */
+   struct vring_packed vring_packed;
+   u8 wrap_counter : 1;
+   bool chaining;
+   };
+   };
  
  	/* How to notify other side. FIXME: commonalize hcalls! */

bool (*notify)(struct virtqueue *vq);
@@ -201,26 +218,37 @@ static dma_addr_t vring_map_single(const struct 
vring_virtqueue *vq,
  cpu_addr, size, direction);
  }
  
-static void vring_unmap_one(const struct vring_virtqueue *vq,

-   struct vring_desc *desc)
+static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc)
  {


Let's split the helpers to packed/split version like other helpers? 
(Consider the caller has already known the type of vq).



+   u64 addr;
+   u32 len;
u16 flags;
  
  	if (!vring_use_dma_api(vq->vq.vdev))

return;
  
-	flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);

+   if (vq->packed) {
+   struct vring_packed_desc *desc = _desc;
+
+   addr = virtio64_to_cpu(vq->vq.vdev, desc->addr);
+   len = virtio32_to_cpu(vq->vq.vdev, desc->len);
+   flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
+   } else {
+   struct vring_desc *desc = _desc;
+
+   addr = virtio64_to_cpu(vq->vq.vdev, desc->addr);
+   len = virtio32_to_cpu(vq->vq.vdev, desc->len);
+   flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
+   }
  
  	if (flags & VRING_DESC_F_INDIRECT) {

dma_unmap_single(vring_dma_dev(vq),
-virtio64_to_cpu(vq->vq.vdev, desc->addr),
-virtio32_to_cpu(vq->vq.vdev, desc->len),
+addr, len,
 (flags & VRING_DESC_F_WRITE) ?
 DMA_FROM_DEVICE : DMA_TO_DEVICE);
} else {
dma_unmap_page(vring_dma_dev(vq),
-  virtio64_to_cpu(vq->vq.vdev, desc->addr),
-  virtio32_to_cpu(vq->vq.vdev, desc->len),
+  addr, len,
   (flags & VRING_DESC_F_WRITE) ?
   DMA_FROM_DEVICE : DMA_TO_DEVICE);
}
@@ -235,8 +263,9 @@ static int vring_mapping_error(const struct vring_virtqueue 
*vq,
return dma_mapping_error(vring_dma_dev(vq), addr);
  }
  
-static struct vring_desc *alloc_indirect(struct virtqueue *_vq,

-unsigned int total_sg, gfp_t gfp)
+static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,
+  unsigned int total_sg,
+  gfp_t gfp)
  {
struct vring_desc *desc;
unsigned int i;
@@ -257,14 +286,32 @@ static struct vring_desc *alloc_indirect(struct virtqueue 
*_vq,
return desc;
  }
  
-static inline int virtqueue_add(struct virtqueue *_vq,

-