Re: [PATCH 03/33] virtio_ring: packed: virtqueue_add_packed() support premapped

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 02, 2023 at 07:00:28PM +0800, Xuan Zhuo wrote:
> virtqueue_add_packed() only supports virtual addresses, dma is completed
> in virtqueue_add_packed().
> 
> In some scenarios (such as the AF_XDP scenario), the memory is allocated
> and DMA is completed in advance, so it is necessary for us to support
> passing the DMA address to virtqueue_add_packed().
> 
> Record this information in desc_state, we can skip unmap based on this
> when executing dma unmap.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/virtio/virtio_ring.c | 71 +++++++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index ec622403cbd5..25027a35fcf8 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -78,6 +78,7 @@ struct vring_desc_state_packed {
>  	struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
>  	u16 num;			/* Descriptor list length. */
>  	u16 last;			/* The last desc state in a list. */
> +	bool premapped;
>  };
>  
>  struct vring_desc_extra {


That's an extra cache line. 
> @@ -1200,7 +1201,8 @@ static inline u16 packed_last_used(u16 last_used_idx)
>  }
>  
>  static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> -				     struct vring_desc_extra *extra)
> +				     struct vring_desc_extra *extra,
> +				     bool premapped)
>  {
>  	u16 flags;
>  
> @@ -1215,6 +1217,9 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>  				 (flags & VRING_DESC_F_WRITE) ?
>  				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
>  	} else {
> +		if (premapped)
> +			return;
> +
>  		dma_unmap_page(vring_dma_dev(vq),
>  			       extra->addr, extra->len,
>  			       (flags & VRING_DESC_F_WRITE) ?
> @@ -1262,7 +1267,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  					 unsigned int out_sgs,
>  					 unsigned int in_sgs,
>  					 void *data,
> -					 gfp_t gfp)
> +					 gfp_t gfp,
> +					 bool premapped)
>  {
>  	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
> @@ -1288,10 +1294,15 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  
>  	for (n = 0; n < out_sgs + in_sgs; n++) {
>  		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> -			addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> -					DMA_TO_DEVICE : DMA_FROM_DEVICE);
> -			if (vring_mapping_error(vq, addr))
> -				goto unmap_release;
> +			if (premapped) {
> +				addr = sg_dma_address(sg);
> +
> +			} else {
> +				addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> +							DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +				if (vring_mapping_error(vq, addr))
> +					goto unmap_release;
> +			}
>  
>  			desc[i].flags = cpu_to_le16(n < out_sgs ?
>  						0 : VRING_DESC_F_WRITE);
> @@ -1350,6 +1361,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  	vq->packed.desc_state[id].data = data;
>  	vq->packed.desc_state[id].indir_desc = desc;
>  	vq->packed.desc_state[id].last = id;
> +	vq->packed.desc_state[id].premapped = premapped;
>  
>  	vq->num_added += 1;
>  
> @@ -1359,10 +1371,11 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  	return 0;
>  
>  unmap_release:
> -	err_idx = i;
> -
> -	for (i = 0; i < err_idx; i++)
> -		vring_unmap_desc_packed(vq, &desc[i]);
> +	if (!premapped) {
> +		err_idx = i;
> +		for (i = 0; i < err_idx; i++)
> +			vring_unmap_desc_packed(vq, &desc[i]);
> +	}
>  
>  	kfree(desc);
>  
> @@ -1377,6 +1390,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  				       unsigned int in_sgs,
>  				       void *data,
>  				       void *ctx,
> +				       bool premapped,
>  				       gfp_t gfp)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> @@ -1403,7 +1417,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  
>  	if (virtqueue_use_indirect(vq, total_sg)) {
>  		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> -						    in_sgs, data, gfp);
> +						    in_sgs, data, gfp, premapped);
>  		if (err != -ENOMEM) {
>  			END_USE(vq);
>  			return err;
> @@ -1435,10 +1449,17 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	c = 0;
>  	for (n = 0; 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, n < out_sgs ?
> -					DMA_TO_DEVICE : DMA_FROM_DEVICE);
> -			if (vring_mapping_error(vq, addr))
> -				goto unmap_release;
> +			dma_addr_t addr;
> +
> +			if (premapped) {
> +				addr = sg_dma_address(sg);
> +

drop this empty line pls.

> +			} else {
> +				addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> +							DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +				if (vring_mapping_error(vq, addr))
> +					goto unmap_release;
> +			}
>  
>  			flags = cpu_to_le16(vq->packed.avail_used_flags |
>  				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
> @@ -1485,6 +1506,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	vq->packed.desc_state[id].data = data;
>  	vq->packed.desc_state[id].indir_desc = ctx;
>  	vq->packed.desc_state[id].last = prev;
> +	vq->packed.desc_state[id].premapped = premapped;
>  
>  	/*
>  	 * A driver MUST NOT make the first descriptor in the list
> @@ -1501,22 +1523,26 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	return 0;
>  
>  unmap_release:
> +	vq->packed.avail_used_flags = avail_used_flags;
> +
> +	if (premapped)
> +		goto unmap_free;
> +

This goto branching inside error handling is too much like spaghetti code.
See Documentation/process/coding-style.rst for when goto is ok -
basically exit/error handling. This is not error handling.
Pls find a way to avoid.

>  	err_idx = i;
>  	i = head;
>  	curr = vq->free_head;
>  
> -	vq->packed.avail_used_flags = avail_used_flags;
> -
>  	for (n = 0; n < total_sg; n++) {
>  		if (i == err_idx)
>  			break;
> -		vring_unmap_extra_packed(vq, &vq->packed.desc_extra[curr]);
> +		vring_unmap_extra_packed(vq, &vq->packed.desc_extra[curr], false);
>  		curr = vq->packed.desc_extra[curr].next;
>  		i++;
>  		if (i >= vq->packed.vring.num)
>  			i = 0;
>  	}
>  
> +unmap_free:
>  	END_USE(vq);
>  	return -EIO;
>  }
> @@ -1576,8 +1602,10 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
>  	struct vring_desc_state_packed *state = NULL;
>  	struct vring_packed_desc *desc;
>  	unsigned int i, curr;
> +	bool premapped;
>  
>  	state = &vq->packed.desc_state[id];
> +	premapped = state->premapped;
>  
>  	/* Clear data ptr. */
>  	state->data = NULL;
> @@ -1590,7 +1618,8 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
>  		curr = id;
>  		for (i = 0; i < state->num; i++) {
>  			vring_unmap_extra_packed(vq,
> -						 &vq->packed.desc_extra[curr]);
> +						 &vq->packed.desc_extra[curr],
> +						 premapped);
>  			curr = vq->packed.desc_extra[curr].next;
>  		}
>  	}
> @@ -1603,7 +1632,7 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
>  		if (!desc)
>  			return;
>  
> -		if (vq->use_dma_api) {
> +		if (vq->use_dma_api && !premapped) {
>  			len = vq->packed.desc_extra[id].len;
>  			for (i = 0; i < len / sizeof(struct vring_packed_desc);
>  					i++)
> @@ -2122,7 +2151,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  
>  	return vq->packed_ring ? virtqueue_add_packed(_vq, sgs, total_sg,
> -					out_sgs, in_sgs, data, ctx, gfp) :
> +					out_sgs, in_sgs, data, ctx, premapped, gfp) :
>  				 virtqueue_add_split(_vq, sgs, total_sg,
>  					out_sgs, in_sgs, data, ctx, premapped, gfp);
>  }


Too much if !premapped all over the place. Pls refactor so we
get common code and then have premapped and non premapped
versions call that.
> -- 
> 2.32.0.3.g01195cf9f