Re: [dpdk-dev] [RFC] virtio-user: ctrl vq support for packed

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Jens,

On 12/21/18 4:29 PM, Jens Freimann wrote:
> This adds support to virtio-user for control virtqueues
> and reverts commit "5e4e7a752 net/virtio-user: fail if
> cq used with packed vq".
> 
> I add a struct virtio_user_queue to have a place for wrap
> counters and avail/used index (which is not needed for split
> ring because it has those in shared memory).
> 
> This is a RFC because it only supports in-order use of descriptors
> in the ring. I'm looking for ideas how to change this without the
> .next field in the descriptor as we have it with split virtqueues.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c            |  11 +-
>   .../net/virtio/virtio_user/virtio_user_dev.c  | 101 ++++++++++++++++--
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  14 ++-
>   drivers/net/virtio/virtio_user_ethdev.c       |  25 ++++-
>   4 files changed, 135 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 446c338fc..010ab6489 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -153,6 +153,7 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
>   	uint16_t flags;
>   	int sum = 0;
>   	int k;
> +	int ndescs = 0;
>   
>   	/*
>   	 * Format is enforced in qemu code:
> @@ -162,7 +163,6 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
>   	 */
>   	head = vq->vq_avail_idx;
>   	wrap_counter = vq->avail_wrap_counter;
> -	desc[head].flags = VRING_DESC_F_NEXT;
>   	desc[head].addr = cvq->virtio_net_hdr_mem;
>   	desc[head].len = sizeof(struct virtio_net_ctrl_hdr);
>   	vq->vq_free_cnt--;
> @@ -170,6 +170,7 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
>   		vq->vq_avail_idx -= vq->vq_nentries;
>   		vq->avail_wrap_counter ^= 1;
>   	}
> +	ndescs++;
>   
>   	for (k = 0; k < pkt_num; k++) {
>   		desc[vq->vq_avail_idx].addr = cvq->virtio_net_hdr_mem
> @@ -188,6 +189,7 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
>   			vq->vq_avail_idx -= vq->vq_nentries;
>   			vq->avail_wrap_counter ^= 1;
>   		}
> +		ndescs++;
>   	}
>   
>   
> @@ -198,6 +200,7 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
>   	flags |= VRING_DESC_F_AVAIL(vq->avail_wrap_counter) |
>   		 VRING_DESC_F_USED(!vq->avail_wrap_counter);
>   	desc[vq->vq_avail_idx].flags = flags;
> +
>   	flags = VRING_DESC_F_NEXT;
>   	flags |= VRING_DESC_F_AVAIL(wrap_counter) |
>   		 VRING_DESC_F_USED(!wrap_counter);
> @@ -209,6 +212,7 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
>   		vq->vq_avail_idx -= vq->vq_nentries;
>   		vq->avail_wrap_counter ^= 1;
>   	}
> +	ndescs++;
>   
>   	virtqueue_notify(vq);
>   
> @@ -220,8 +224,9 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
>   
>   	/* now get used descriptors */
>   	while (desc_is_used(&desc[vq->vq_used_cons_idx], vq)) {
> -		vq->vq_free_cnt++;
> -		if (++vq->vq_used_cons_idx >= vq->vq_nentries) {
> +		vq->vq_free_cnt += ndescs;
> +		vq->vq_used_cons_idx += ndescs;
> +		if (vq->vq_used_cons_idx >= vq->vq_nentries) {
>   			vq->vq_used_cons_idx -= vq->vq_nentries;
>   			vq->used_wrap_counter ^= 1;
>   		}

I'm not sure why this change is needed.
Why not getting used descriptor one by one as it was before?
If it does not work, doesn't that mean that the desc chain is not
marked as used properly by the device?

Thanks,
Maxime