diff mbox series

[v9,09/32] virtio_ring: split: extract the logic of vq init

Message ID 20220406034346.74409-10-xuanzhuo@linux.alibaba.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series virtio pci support VIRTIO_F_RING_RESET (refactor vring) | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Xuan Zhuo April 6, 2022, 3:43 a.m. UTC
Separate the logic of initializing vq, and subsequent patches will call
it separately.

The feature of this part is that it does not depend on the information
passed by the upper layer and can be called repeatedly.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 68 ++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 30 deletions(-)

Comments

Jason Wang April 12, 2022, 3:42 a.m. UTC | #1
在 2022/4/6 上午11:43, Xuan Zhuo 写道:
> Separate the logic of initializing vq, and subsequent patches will call
> it separately.
>
> The feature of this part is that it does not depend on the information
> passed by the upper layer and can be called repeatedly.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_ring.c | 68 ++++++++++++++++++++----------------
>   1 file changed, 38 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 083f2992ba0d..874f878087a3 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -916,6 +916,43 @@ static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)
>   	return NULL;
>   }
>   
> +static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
> +				       struct virtio_device *vdev,
> +				       bool own_ring)
> +{
> +	vq->packed_ring = false;
> +	vq->vq.num_free = vq->split.vring.num;
> +	vq->we_own_ring = own_ring;
> +	vq->broken = false;
> +	vq->last_used_idx = 0;
> +	vq->event_triggered = false;
> +	vq->num_added = 0;
> +	vq->use_dma_api = vring_use_dma_api(vdev);
> +#ifdef DEBUG
> +	vq->in_use = false;
> +	vq->last_add_time_valid = false;
> +#endif
> +
> +	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
> +
> +	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> +		vq->weak_barriers = false;
> +
> +	vq->split.avail_flags_shadow = 0;
> +	vq->split.avail_idx_shadow = 0;
> +
> +	/* No callback?  Tell other side not to bother us. */
> +	if (!vq->vq.callback) {
> +		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> +		if (!vq->event)
> +			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
> +					vq->split.avail_flags_shadow);
> +	}
> +
> +	/* Put everything in free lists. */
> +	vq->free_head = 0;


It's not clear what kind of initialization that we want to do here. E.g 
it mixes split specific setups with some general setups which is kind of 
duplication of vring_virtqueue_init_packed().

I wonder if it's better to only do split specific setups here and have a 
common helper to do the setup that is irrelevant to ring layout.

Thanks


> +}
> +
>   static void vring_virtqueue_attach_split(struct vring_virtqueue *vq,
>   					 struct vring vring,
>   					 struct vring_desc_state_split *desc_state,
> @@ -2249,42 +2286,15 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>   	if (!vq)
>   		return NULL;
>   
> -	vq->packed_ring = false;
>   	vq->vq.callback = callback;
>   	vq->vq.vdev = vdev;
>   	vq->vq.name = name;
> -	vq->vq.num_free = vring.num;
>   	vq->vq.index = index;
> -	vq->we_own_ring = false;
>   	vq->notify = notify;
>   	vq->weak_barriers = weak_barriers;
> -	vq->broken = false;
> -	vq->last_used_idx = 0;
> -	vq->event_triggered = false;
> -	vq->num_added = 0;
> -	vq->use_dma_api = vring_use_dma_api(vdev);
> -#ifdef DEBUG
> -	vq->in_use = false;
> -	vq->last_add_time_valid = false;
> -#endif
>   
>   	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
>   		!context;
> -	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
> -
> -	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> -		vq->weak_barriers = false;
> -
> -	vq->split.avail_flags_shadow = 0;
> -	vq->split.avail_idx_shadow = 0;
> -
> -	/* No callback?  Tell other side not to bother us. */
> -	if (!callback) {
> -		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> -		if (!vq->event)
> -			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
> -					vq->split.avail_flags_shadow);
> -	}
>   
>   	err = vring_alloc_state_extra_split(vring.num, &state, &extra);
>   	if (err) {
> @@ -2293,9 +2303,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>   	}
>   
>   	vring_virtqueue_attach_split(vq, vring, state, extra);
> -
> -	/* Put everything in free lists. */
> -	vq->free_head = 0;
> +	vring_virtqueue_init_split(vq, vdev, false);
>   
>   	spin_lock(&vdev->vqs_list_lock);
>   	list_add_tail(&vq->vq.list, &vdev->vqs);
Xuan Zhuo April 13, 2022, 7:04 a.m. UTC | #2
On Tue, 12 Apr 2022 11:42:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/4/6 上午11:43, Xuan Zhuo 写道:
> > Separate the logic of initializing vq, and subsequent patches will call
> > it separately.
> >
> > The feature of this part is that it does not depend on the information
> > passed by the upper layer and can be called repeatedly.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 68 ++++++++++++++++++++----------------
> >   1 file changed, 38 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 083f2992ba0d..874f878087a3 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -916,6 +916,43 @@ static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)
> >   	return NULL;
> >   }
> >
> > +static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
> > +				       struct virtio_device *vdev,
> > +				       bool own_ring)
> > +{
> > +	vq->packed_ring = false;
> > +	vq->vq.num_free = vq->split.vring.num;
> > +	vq->we_own_ring = own_ring;
> > +	vq->broken = false;
> > +	vq->last_used_idx = 0;
> > +	vq->event_triggered = false;
> > +	vq->num_added = 0;
> > +	vq->use_dma_api = vring_use_dma_api(vdev);
> > +#ifdef DEBUG
> > +	vq->in_use = false;
> > +	vq->last_add_time_valid = false;
> > +#endif
> > +
> > +	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
> > +
> > +	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> > +		vq->weak_barriers = false;
> > +
> > +	vq->split.avail_flags_shadow = 0;
> > +	vq->split.avail_idx_shadow = 0;
> > +
> > +	/* No callback?  Tell other side not to bother us. */
> > +	if (!vq->vq.callback) {
> > +		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> > +		if (!vq->event)
> > +			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
> > +					vq->split.avail_flags_shadow);
> > +	}
> > +
> > +	/* Put everything in free lists. */
> > +	vq->free_head = 0;
>
>
> It's not clear what kind of initialization that we want to do here. E.g
> it mixes split specific setups with some general setups which is kind of
> duplication of vring_virtqueue_init_packed().
>
> I wonder if it's better to only do split specific setups here and have a
> common helper to do the setup that is irrelevant to ring layout.

Yes, you are right, I didn't notice this situation before.

Thanks.

>
> Thanks
>
>
> > +}
> > +
> >   static void vring_virtqueue_attach_split(struct vring_virtqueue *vq,
> >   					 struct vring vring,
> >   					 struct vring_desc_state_split *desc_state,
> > @@ -2249,42 +2286,15 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >   	if (!vq)
> >   		return NULL;
> >
> > -	vq->packed_ring = false;
> >   	vq->vq.callback = callback;
> >   	vq->vq.vdev = vdev;
> >   	vq->vq.name = name;
> > -	vq->vq.num_free = vring.num;
> >   	vq->vq.index = index;
> > -	vq->we_own_ring = false;
> >   	vq->notify = notify;
> >   	vq->weak_barriers = weak_barriers;
> > -	vq->broken = false;
> > -	vq->last_used_idx = 0;
> > -	vq->event_triggered = false;
> > -	vq->num_added = 0;
> > -	vq->use_dma_api = vring_use_dma_api(vdev);
> > -#ifdef DEBUG
> > -	vq->in_use = false;
> > -	vq->last_add_time_valid = false;
> > -#endif
> >
> >   	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
> >   		!context;
> > -	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
> > -
> > -	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> > -		vq->weak_barriers = false;
> > -
> > -	vq->split.avail_flags_shadow = 0;
> > -	vq->split.avail_idx_shadow = 0;
> > -
> > -	/* No callback?  Tell other side not to bother us. */
> > -	if (!callback) {
> > -		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> > -		if (!vq->event)
> > -			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
> > -					vq->split.avail_flags_shadow);
> > -	}
> >
> >   	err = vring_alloc_state_extra_split(vring.num, &state, &extra);
> >   	if (err) {
> > @@ -2293,9 +2303,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >   	}
> >
> >   	vring_virtqueue_attach_split(vq, vring, state, extra);
> > -
> > -	/* Put everything in free lists. */
> > -	vq->free_head = 0;
> > +	vring_virtqueue_init_split(vq, vdev, false);
> >
> >   	spin_lock(&vdev->vqs_list_lock);
> >   	list_add_tail(&vq->vq.list, &vdev->vqs);
>
diff mbox series

Patch

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 083f2992ba0d..874f878087a3 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -916,6 +916,43 @@  static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)
 	return NULL;
 }
 
+static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
+				       struct virtio_device *vdev,
+				       bool own_ring)
+{
+	vq->packed_ring = false;
+	vq->vq.num_free = vq->split.vring.num;
+	vq->we_own_ring = own_ring;
+	vq->broken = false;
+	vq->last_used_idx = 0;
+	vq->event_triggered = false;
+	vq->num_added = 0;
+	vq->use_dma_api = vring_use_dma_api(vdev);
+#ifdef DEBUG
+	vq->in_use = false;
+	vq->last_add_time_valid = false;
+#endif
+
+	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
+
+	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
+		vq->weak_barriers = false;
+
+	vq->split.avail_flags_shadow = 0;
+	vq->split.avail_idx_shadow = 0;
+
+	/* No callback?  Tell other side not to bother us. */
+	if (!vq->vq.callback) {
+		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+		if (!vq->event)
+			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
+					vq->split.avail_flags_shadow);
+	}
+
+	/* Put everything in free lists. */
+	vq->free_head = 0;
+}
+
 static void vring_virtqueue_attach_split(struct vring_virtqueue *vq,
 					 struct vring vring,
 					 struct vring_desc_state_split *desc_state,
@@ -2249,42 +2286,15 @@  struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	if (!vq)
 		return NULL;
 
-	vq->packed_ring = false;
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
-	vq->vq.num_free = vring.num;
 	vq->vq.index = index;
-	vq->we_own_ring = false;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
-	vq->broken = false;
-	vq->last_used_idx = 0;
-	vq->event_triggered = false;
-	vq->num_added = 0;
-	vq->use_dma_api = vring_use_dma_api(vdev);
-#ifdef DEBUG
-	vq->in_use = false;
-	vq->last_add_time_valid = false;
-#endif
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
 		!context;
-	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
-
-	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
-		vq->weak_barriers = false;
-
-	vq->split.avail_flags_shadow = 0;
-	vq->split.avail_idx_shadow = 0;
-
-	/* No callback?  Tell other side not to bother us. */
-	if (!callback) {
-		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
-		if (!vq->event)
-			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
-					vq->split.avail_flags_shadow);
-	}
 
 	err = vring_alloc_state_extra_split(vring.num, &state, &extra);
 	if (err) {
@@ -2293,9 +2303,7 @@  struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	}
 
 	vring_virtqueue_attach_split(vq, vring, state, extra);
-
-	/* Put everything in free lists. */
-	vq->free_head = 0;
+	vring_virtqueue_init_split(vq, vdev, false);
 
 	spin_lock(&vdev->vqs_list_lock);
 	list_add_tail(&vq->vq.list, &vdev->vqs);