Linux Virtualization - Mar 2023 - [PATCH v2 0/3] virtio_ring: Clean up code for virtio ring and pci

This patch series performs a clean up of the code in virtio_ring and
virtio_pci, modifying it to conform with the Linux kernel coding style
guidance [1]. The modifications ensure the code easy to read and
understand. This small series does few short cleanups in the code.

Patch-1 Allow non power of 2 sizes for packed virtqueues.
Patch-2 Avoid using inline for small functions.
Patch-3 Use const to annotate read-only pointer params.

[1]
https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease

All of the patches have been verified based on the kernel code
commit 81ff855485a3 ("Merge tag 'i2c-for-6.3-rc2' of
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux")

Feng Liu (3):
  virtio_ring: Allow non power of 2 sizes for packed virtqueue
  virtio_ring: Avoid using inline for small functions
  virtio_ring: Use const to annotate read-only pointer params

 drivers/virtio/virtio_pci_modern.c |  5 ----
 drivers/virtio/virtio_ring.c       | 48 +++++++++++++++---------------
 include/linux/virtio.h             | 14 ++++-----
 3 files changed, 31 insertions(+), 36 deletions(-)

-- 
2.34.1

According to the Virtio Specification, the Queue Size parameter of a
virtqueue corresponds to the maximum number of descriptors in that
queue, and it does not have to be a power of 2 for packed virtqueues.
However, the virtio_pci_modern driver enforced a power of 2 check for
virtqueue sizes, which is unnecessary and restrictive for packed
virtuqueue.

Split virtqueue still needs to check the virtqueue size is power_of_2
which has been done in vring_alloc_queue_split of the virtio_ring layer.

To validate this change, we tested various virtqueue sizes for packed
rings, including 128, 256, 512, 100, 200, 500, and 1000, with
CONFIG_PAGE_POISONING enabled, and all tests passed successfully.

Signed-off-by: Feng Liu <feliu at nvidia.com>
Reviewed-by: Jiri Pirko <jiri at nvidia.com>

---
v0 -> v1
feedbacks from Jason Wang and Michael S. Tsirkin
- remove power_of_2 check of virtqueue size

v1 -> v2
feedbacks from Parav Pandit and Jiri Pirko
- keep power_of_2 check of split virtqueue in vring_alloc_queue_split of
  virtio_ring layer.
---
 drivers/virtio/virtio_pci_modern.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 9e496e288cfa..6e713904d8e8 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -310,11 +310,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device
*vp_dev,
 	if (!num || vp_modern_get_queue_enable(mdev, index))
 		return ERR_PTR(-ENOENT);
 
-	if (!is_power_of_2(num)) {
-		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u",
num);
-		return ERR_PTR(-EINVAL);
-	}
-
 	info->msix_vector = msix_vec;
 
 	/* create the vring */
-- 
2.34.1

Remove the inline keyword, according to kernel coding style [1], defining
inline functions is not necessary for samll functions.

It is verified with GCC 12.2.0, the generated code with/without inline
is the same. Additionally tested with kernel pktgen and iperf, and
verified the result, pps of the results are the same in the cases of
with/without inline.

[1]
https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease

Signed-off-by: Feng Liu <feliu at nvidia.com>
Reviewed-by: Jiri Pirko <jiri at nvidia.com>
---
 drivers/virtio/virtio_ring.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 41144b5246a8..a26fab91c59f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -233,8 +233,8 @@ static void vring_free(struct virtqueue *_vq);
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
-static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
-					  unsigned int total_sg)
+static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
+				   unsigned int total_sg)
 {
 	/*
 	 * If the host supports indirect descriptor tables, and we have multiple
@@ -349,7 +349,7 @@ static void vring_free_queue(struct virtio_device *vdev,
size_t size,
  * making all of the arch DMA ops work on the vring device itself
  * is a mess.
  */
-static inline struct device *vring_dma_dev(const struct vring_virtqueue *vq)
+static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
 {
 	return vq->dma_dev;
 }
@@ -784,7 +784,7 @@ static void detach_buf_split(struct vring_virtqueue *vq,
unsigned int head,
 	}
 }
 
-static inline bool more_used_split(const struct vring_virtqueue *vq)
+static bool more_used_split(const struct vring_virtqueue *vq)
 {
 	return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev,
 			vq->split.vring.used->idx);
@@ -1172,12 +1172,12 @@ static int virtqueue_resize_split(struct virtqueue *_vq,
u32 num)
 /*
  * Packed ring specific functions - *_packed().
  */
-static inline bool packed_used_wrap_counter(u16 last_used_idx)
+static bool packed_used_wrap_counter(u16 last_used_idx)
 {
 	return !!(last_used_idx & (1 << VRING_PACKED_EVENT_F_WRAP_CTR));
 }
 
-static inline u16 packed_last_used(u16 last_used_idx)
+static u16 packed_last_used(u16 last_used_idx)
 {
 	return last_used_idx & ~(-(1 << VRING_PACKED_EVENT_F_WRAP_CTR));
 }
@@ -1612,7 +1612,7 @@ static inline bool is_used_desc_packed(const struct
vring_virtqueue *vq,
 	return avail == used && used == used_wrap_counter;
 }
 
-static inline bool more_used_packed(const struct vring_virtqueue *vq)
+static bool more_used_packed(const struct vring_virtqueue *vq)
 {
 	u16 last_used;
 	u16 last_used_idx;
-- 
2.34.1

Add const to make the read-only pointer parameters clear, similar to
many existing functions.

Use `container_of_const` to implement `to_vvq`, which ensures the
const-ness of read-only parameters and avoids accidental modification
of their members.

Signed-off-by: Feng Liu <feliu at nvidia.com>
Reviewed-by: Jiri Pirko <jiri at nvidia.com>

---
v0 -> v1
feedbacks from Michael S. Tsirkin
- use `container_of_const` to implement `to_vvq`
---
 drivers/virtio/virtio_ring.c | 36 ++++++++++++++++++------------------
 include/linux/virtio.h       | 14 +++++++-------
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a26fab91c59f..4c3bb0ddeb9b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
  * Helpers.
  */
 
-#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
+#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
 
-static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
+static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
 				   unsigned int total_sg)
 {
 	/*
@@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue
*vq,
  * unconditionally on data path.
  */
 
-static bool vring_use_dma_api(struct virtio_device *vdev)
+static bool vring_use_dma_api(const struct virtio_device *vdev)
 {
 	if (!virtio_has_dma_quirk(vdev))
 		return true;
@@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
 	return false;
 }
 
-size_t virtio_max_dma_size(struct virtio_device *vdev)
+size_t virtio_max_dma_size(const struct virtio_device *vdev)
 {
 	size_t max_segment_size = SIZE_MAX;
 
@@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32
num)
  */
 
 static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
-					   struct vring_desc *desc)
+					   const struct vring_desc *desc)
 {
 	u16 flags;
 
@@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
 }
 
 static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
-				     struct vring_desc_extra *extra)
+				     const struct vring_desc_extra *extra)
 {
 	u16 flags;
 
@@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct
vring_virtqueue *vq,
 }
 
 static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
-				   struct vring_packed_desc *desc)
+				    const struct vring_packed_desc *desc)
 {
 	u16 flags;
 
@@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
  * Returns the size of the vring.  This is mainly used for boasting to
  * userspace.  Unlike other operations, this need not be serialized.
  */
-unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
+unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
 {
 
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
 }
@@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
 
-bool virtqueue_is_broken(struct virtqueue *_vq)
+bool virtqueue_is_broken(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	return READ_ONCE(vq->broken);
 }
@@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
 
-dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	BUG_ON(!vq->we_own_ring);
 
@@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
 
-dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	BUG_ON(!vq->we_own_ring);
 
@@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
 
-dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	BUG_ON(!vq->we_own_ring);
 
@@ -2910,7 +2910,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
 EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
 
 /* Only available for split ring */
-const struct vring *virtqueue_get_vring(struct virtqueue *vq)
+const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
 {
 	return &to_vvq(vq)->split.vring;
 }
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 2b472514c49b..c4225653f949 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
 
 void *virtqueue_detach_unused_buf(struct virtqueue *vq);
 
-unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
+unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
 
-bool virtqueue_is_broken(struct virtqueue *vq);
+bool virtqueue_is_broken(const struct virtqueue *vq);
 
-const struct vring *virtqueue_get_vring(struct virtqueue *vq);
-dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
-dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
-dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
+const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
+dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
+dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
+dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
 
 int virtqueue_resize(struct virtqueue *vq, u32 num,
 		     void (*recycle)(struct virtqueue *vq, void *buf));
@@ -147,7 +147,7 @@ int virtio_device_restore(struct virtio_device *dev);
 #endif
 void virtio_reset_device(struct virtio_device *dev);
 
-size_t virtio_max_dma_size(struct virtio_device *vdev);
+size_t virtio_max_dma_size(const struct virtio_device *vdev);
 
 #define virtio_device_for_each_vq(vdev, vq) \
 	list_for_each_entry(vq, &vdev->vqs, list)
-- 
2.34.1

Series:

Reviewed-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>

Thanks.

On Wed, 15 Mar 2023 20:54:55 +0200, Feng Liu via Virtualization
<virtualization at lists.linux-foundation.org>
wrote:
> This patch series performs a clean up of the code in virtio_ring and
> virtio_pci, modifying it to conform with the Linux kernel coding style
> guidance [1]. The modifications ensure the code easy to read and
> understand. This small series does few short cleanups in the code.
>
> Patch-1 Allow non power of 2 sizes for packed virtqueues.
> Patch-2 Avoid using inline for small functions.
> Patch-3 Use const to annotate read-only pointer params.
>
> [1]
>
https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease
>
> All of the patches have been verified based on the kernel code
> commit 81ff855485a3 ("Merge tag 'i2c-for-6.3-rc2' of
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux")
>
> Feng Liu (3):
>   virtio_ring: Allow non power of 2 sizes for packed virtqueue
>   virtio_ring: Avoid using inline for small functions
>   virtio_ring: Use const to annotate read-only pointer params
>
>  drivers/virtio/virtio_pci_modern.c |  5 ----
>  drivers/virtio/virtio_ring.c       | 48 +++++++++++++++---------------
>  include/linux/virtio.h             | 14 ++++-----
>  3 files changed, 31 insertions(+), 36 deletions(-)
>
> --
> 2.34.1
>
> _______________________________________________
> Virtualization mailing list
> Virtualization at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization