diff mbox series

[v5,03/11] net/virtio: add packed virtqueue helpers

Message ID 20180906181947.20646-4-jfreimann@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series implement packed virtqueues |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Jens Freimann Sept. 6, 2018, 6:19 p.m. UTC 
  Add helper functions to set/clear and check descriptor flags.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ring.h | 26 ++++++++++++++++++++++++++
 drivers/net/virtio/virtqueue.h   | 19 +++++++++++++++++++
 2 files changed, 45 insertions(+)

Comments

Maxime Coquelin Sept. 12, 2018, 8:25 a.m. UTC | #1 
On 09/06/2018 08:19 PM, Jens Freimann wrote:
> Add helper functions to set/clear and check descriptor flags.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_ring.h | 26 ++++++++++++++++++++++++++
>   drivers/net/virtio/virtqueue.h   | 19 +++++++++++++++++++
>   2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index e2c597434..f3b23f419 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -78,6 +78,8 @@ struct vring_packed_desc_event {
>   
>   struct vring {
>   	unsigned int num;
> +	unsigned int avail_wrap_counter;
> +	unsigned int used_wrap_counter;
>   	union {
>   		struct vring_desc_packed *desc_packed;
>   		struct vring_desc *desc;
> @@ -92,6 +94,30 @@ struct vring {
>   	};
>   };
>   
> +static inline void
> +_set_desc_avail(struct vring_desc_packed *desc, int wrap_counter)
> +{
> +	desc->flags |= VRING_DESC_F_AVAIL(wrap_counter) |
> +		       VRING_DESC_F_USED(!wrap_counter);

It implies the avail and used bits to be cleared beforehand.
Maybe it would be safer to clear them in the helper?

> +}
> +
> +static inline void
> +set_desc_avail(struct vring *vr, struct vring_desc_packed *desc)
> +{
> +	_set_desc_avail(desc, vr->avail_wrap_counter);
> +}
> +
> +static inline int
> +desc_is_used(struct vring_desc_packed *desc, struct vring *vr)
> +{
> +	uint16_t used, avail;
> +
> +	used = !!(desc->flags & VRING_DESC_F_USED(1));
> +	avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));
> +
> +	return used == avail && used == vr->used_wrap_counter;
> +}
> +
>   /* The standard layout for the ring is a continuous chunk of memory which
>    * looks like this.  We assume num is a power of 2.
>    *
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index d2a0b651a..53fce61b4 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -245,6 +245,25 @@ struct virtio_tx_region {
>   			   __attribute__((__aligned__(16)));
>   };
>   
> +static inline uint16_t
> +increment_pq_index(uint16_t idx, size_t ring_size)
> +{
> +	return ++idx >= ring_size ? 0 : idx;
> +}

Not sure this helper is really useful, as it ends up
doing two checks in a row.

> +
> +static inline uint16_t
> +update_pq_avail_index(struct virtqueue *vq)
> +{
> +	uint16_t idx;
> +
> +	idx = increment_pq_index(vq->vq_avail_idx, vq->vq_nentries);
> +	if (idx == 0)
> +		vq->vq_ring.avail_wrap_counter ^= 1;

> +	vq->vq_avail_idx = idx;
> +
> +	return vq->vq_avail_idx;
> +}

So it could be simplified to:

{
	if (++vq->vq_avail_idx >= vq->vq_entries) {
		vq->vq_avail_idx = 0;
		vq->vq_ring.avail_wrap_counter ^= 1;
	}

	return vq->vq_avail_idx;
}

> +
>   static inline void
>   vring_desc_init_packed(struct vring *vr, int n)
>   {
>
Jens Freimann Sept. 12, 2018, 9:04 a.m. UTC | #2 
On Wed, Sep 12, 2018 at 10:25:57AM +0200, Maxime Coquelin wrote:
>
>
>On 09/06/2018 08:19 PM, Jens Freimann wrote:
>>Add helper functions to set/clear and check descriptor flags.
>>
>>Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>---
>>  drivers/net/virtio/virtio_ring.h | 26 ++++++++++++++++++++++++++
>>  drivers/net/virtio/virtqueue.h   | 19 +++++++++++++++++++
>>  2 files changed, 45 insertions(+)
>>
>>diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
>>index e2c597434..f3b23f419 100644
>>--- a/drivers/net/virtio/virtio_ring.h
>>+++ b/drivers/net/virtio/virtio_ring.h
>>@@ -78,6 +78,8 @@ struct vring_packed_desc_event {
>>  struct vring {
>>  	unsigned int num;
>>+	unsigned int avail_wrap_counter;
>>+	unsigned int used_wrap_counter;
>>  	union {
>>  		struct vring_desc_packed *desc_packed;
>>  		struct vring_desc *desc;
>>@@ -92,6 +94,30 @@ struct vring {
>>  	};
>>  };
>>+static inline void
>>+_set_desc_avail(struct vring_desc_packed *desc, int wrap_counter)
>>+{
>>+	desc->flags |= VRING_DESC_F_AVAIL(wrap_counter) |
>>+		       VRING_DESC_F_USED(!wrap_counter);
>
>It implies the avail and used bits to be cleared beforehand.
>Maybe it would be safer to clear them in the helper?

Safer but also less explicit for someone who just reads the higher
level function. But I think it's better to go for the safer version
here.  

>
>>+}
>>+
>>+static inline void
>>+set_desc_avail(struct vring *vr, struct vring_desc_packed *desc)
>>+{
>>+	_set_desc_avail(desc, vr->avail_wrap_counter);
>>+}
>>+
>>+static inline int
>>+desc_is_used(struct vring_desc_packed *desc, struct vring *vr)
>>+{
>>+	uint16_t used, avail;
>>+
>>+	used = !!(desc->flags & VRING_DESC_F_USED(1));
>>+	avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));
>>+
>>+	return used == avail && used == vr->used_wrap_counter;
>>+}
>>+
>>  /* The standard layout for the ring is a continuous chunk of memory which
>>   * looks like this.  We assume num is a power of 2.
>>   *
>>diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>>index d2a0b651a..53fce61b4 100644
>>--- a/drivers/net/virtio/virtqueue.h
>>+++ b/drivers/net/virtio/virtqueue.h
>>@@ -245,6 +245,25 @@ struct virtio_tx_region {
>>  			   __attribute__((__aligned__(16)));
>>  };
>>+static inline uint16_t
>>+increment_pq_index(uint16_t idx, size_t ring_size)
>>+{
>>+	return ++idx >= ring_size ? 0 : idx;
>>+}
>
>Not sure this helper is really useful, as it ends up
>doing two checks in a row.
>
>>+
>>+static inline uint16_t
>>+update_pq_avail_index(struct virtqueue *vq)
>>+{
>>+	uint16_t idx;
>>+
>>+	idx = increment_pq_index(vq->vq_avail_idx, vq->vq_nentries);
>>+	if (idx == 0)
>>+		vq->vq_ring.avail_wrap_counter ^= 1;
>
>>+	vq->vq_avail_idx = idx;
>>+
>>+	return vq->vq_avail_idx;
>>+}
>
>So it could be simplified to:
>
>{
>	if (++vq->vq_avail_idx >= vq->vq_entries) {
>		vq->vq_avail_idx = 0;
>		vq->vq_ring.avail_wrap_counter ^= 1;
>	}
>
>	return vq->vq_avail_idx;

yes, I will either change to what you suggested or
get rid of the helper completely since it is only used in
very few places.

Thanks for the review!

regards,
Jens
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index e2c597434..f3b23f419 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -78,6 +78,8 @@  struct vring_packed_desc_event {
 
 struct vring {
 	unsigned int num;
+	unsigned int avail_wrap_counter;
+	unsigned int used_wrap_counter;
 	union {
 		struct vring_desc_packed *desc_packed;
 		struct vring_desc *desc;
@@ -92,6 +94,30 @@  struct vring {
 	};
 };
 
+static inline void
+_set_desc_avail(struct vring_desc_packed *desc, int wrap_counter)
+{
+	desc->flags |= VRING_DESC_F_AVAIL(wrap_counter) |
+		       VRING_DESC_F_USED(!wrap_counter);
+}
+
+static inline void
+set_desc_avail(struct vring *vr, struct vring_desc_packed *desc)
+{
+	_set_desc_avail(desc, vr->avail_wrap_counter);
+}
+
+static inline int
+desc_is_used(struct vring_desc_packed *desc, struct vring *vr)
+{
+	uint16_t used, avail;
+
+	used = !!(desc->flags & VRING_DESC_F_USED(1));
+	avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));
+
+	return used == avail && used == vr->used_wrap_counter;
+}
+
 /* The standard layout for the ring is a continuous chunk of memory which
  * looks like this.  We assume num is a power of 2.
  *
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index d2a0b651a..53fce61b4 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -245,6 +245,25 @@  struct virtio_tx_region {
 			   __attribute__((__aligned__(16)));
 };
 
+static inline uint16_t
+increment_pq_index(uint16_t idx, size_t ring_size)
+{
+	return ++idx >= ring_size ? 0 : idx;
+}
+
+static inline uint16_t
+update_pq_avail_index(struct virtqueue *vq)
+{
+	uint16_t idx;
+
+	idx = increment_pq_index(vq->vq_avail_idx, vq->vq_nentries);
+	if (idx == 0)
+		vq->vq_ring.avail_wrap_counter ^= 1;
+	vq->vq_avail_idx = idx;
+
+	return vq->vq_avail_idx;
+}
+
 static inline void
 vring_desc_init_packed(struct vring *vr, int n)
 {