Re: [PATCH 1/3] virtio: cache indirect desc for split

On Thu, Oct 28, 2021 at 1:07 AM Michael S. Tsirkin [off-list ref] wrote:

On Wed, Oct 27, 2021 at 02:19:11PM +0800, Xuan Zhuo wrote:

quoted

In the case of using indirect, indirect desc must be allocated and
released each time, which increases a lot of cpu overhead.

Here, a cache is added for indirect. If the number of indirect desc to be
applied for is less than VIRT_QUEUE_CACHE_DESC_NUM, the desc array with
the size of VIRT_QUEUE_CACHE_DESC_NUM is fixed and cached for reuse.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio.c      |  6 ++++
 drivers/virtio/virtio_ring.c | 63 ++++++++++++++++++++++++++++++------
 include/linux/virtio.h       | 10 ++++++
 3 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 0a5b54034d4b..04bcb74e5b9a 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c

@@ -431,6 +431,12 @@ bool is_virtio_device(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(is_virtio_device);

+void virtio_use_desc_cache(struct virtio_device *dev, bool val)
+{
+     dev->desc_cache = val;
+}
+EXPORT_SYMBOL_GPL(virtio_use_desc_cache);
+
 void unregister_virtio_device(struct virtio_device *dev)
 {
      int index = dev->index; /* save for after device release */

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index dd95dfd85e98..0b9a8544b0e8 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -117,6 +117,10 @@ struct vring_virtqueue {
      /* Hint for event idx: already triggered no need to disable. */
      bool event_triggered;

+     /* Is indirect cache used? */
+     bool use_desc_cache;
+     void *desc_cache_chain;
+
      union {
              /* Available for split ring */
              struct {

@@ -423,12 +427,47 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
      return extra[i].next;
 }

-static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,
+#define VIRT_QUEUE_CACHE_DESC_NUM 4
+
+static void desc_cache_chain_free_split(void *chain)
+{
+     struct vring_desc *desc;
+
+     while (chain) {
+             desc = chain;
+             chain = (void *)desc->addr;
+             kfree(desc);
+     }
+}
+
+static void desc_cache_put_split(struct vring_virtqueue *vq,
+                              struct vring_desc *desc, int n)
+{
+     if (vq->use_desc_cache && n <= VIRT_QUEUE_CACHE_DESC_NUM) {
+             desc->addr = (u64)vq->desc_cache_chain;
+             vq->desc_cache_chain = desc;
+     } else {
+             kfree(desc);
+     }
+}
+

So I have a question here. What happens if we just do:

if (n <= VIRT_QUEUE_CACHE_DESC_NUM) {
        return kmem_cache_alloc(VIRT_QUEUE_CACHE_DESC_NUM * sizeof desc, gfp)
} else {
        return kmalloc_arrat(n, sizeof desc, gfp)
}

A small change and won't we reap most performance benefits?

Yes, I think we need a benchmark to use private cache to see how much
it can help.

Thanks

--
MST