Re: [dpdk-dev] [RFC 01/14] vhost: move async data in a dedicated structure

[Maxime Coquelin]

On 10/14/21 05:24, Hu, Jiayu wrote:

Hi Maxime,

-----Original Message-----
From: Maxime Coquelin <maxime.coque...@redhat.com>
Sent: Friday, October 8, 2021 6:00 AM
To: dev@dpdk.org; Xia, Chenbo <chenbo....@intel.com>; Hu, Jiayu
<jiayu...@intel.com>; Wang, YuanX <yuanx.w...@intel.com>; Ma,
WenwuX <wenwux...@intel.com>; Richardson, Bruce
<bruce.richard...@intel.com>; Mcnamara, John
<john.mcnam...@intel.com>
Cc: Maxime Coquelin <maxime.coque...@redhat.com>
Subject: [RFC 01/14] vhost: move async data in a dedicated structure

This patch moves async-related metadata from vhost_virtqueue to a
dedicated struct. It makes it clear which fields are async related, and also
saves some memory when async feature is not in use.

Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
---
  lib/vhost/vhost.c      | 129 ++++++++++++++++-------------------------
  lib/vhost/vhost.h      |  53 ++++++++---------
  lib/vhost/vhost_user.c |   4 +-
  lib/vhost/virtio_net.c | 114 +++++++++++++++++++-----------------
  4 files changed, 140 insertions(+), 160 deletions(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
9540522dac..58f72b633c 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -340,19 +340,15 @@ cleanup_device(struct virtio_net *dev, int destroy)
static void  vhost_free_async_mem(struct vhost_virtqueue *vq)  {
-       rte_free(vq->async_pkts_info);
+       rte_free(vq->async->pkts_info);

Apps may unregister async in vring_state_changed() explicitly when
vring is disabled. In this case, rte_vhost_async_channel_unregister()
will call vhost_free_async_mem() first, so that vq->async becomes NULL.
But after then device is destroyed, free_vq() calls vhost_free_async_mem()
again. "rte_free(vq->async->pkts_info)" will try to read a NULL pointer,
which will cause segment fault.

Good catch, I'll add a check of vq->async before dereferencing it.

-       rte_free(vq->async_buffers_packed);
-       vq->async_buffers_packed = NULL;
-       rte_free(vq->async_descs_split);
-       vq->async_descs_split = NULL;
+       rte_free(vq->async->buffers_packed);
+       vq->async->buffers_packed = NULL;
+       rte_free(vq->async->descs_split);
+       vq->async->descs_split = NULL;

-       rte_free(vq->it_pool);
-       rte_free(vq->vec_pool);
-
-       vq->async_pkts_info = NULL;
-       vq->it_pool = NULL;
-       vq->vec_pool = NULL;
+       rte_free(vq->async);
+       vq->async = NULL;
  }

  void
@@ -1629,77 +1625,63 @@ async_channel_register(int vid, uint16_t
queue_id,  {
        struct virtio_net *dev = get_device(vid);
        struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
+       struct vhost_async *async;
+       int node = vq->numa_node;

-       if (unlikely(vq->async_registered)) {
+       if (unlikely(vq->async)) {
                VHOST_LOG_CONFIG(ERR,
-                       "async register failed: channel already registered "
-                       "(vid %d, qid: %d)\n", vid, queue_id);
+                               "async register failed: already registered
(vid %d, qid: %d)\n",
+                               vid, queue_id);
                return -1;
        }

-       vq->async_pkts_info = rte_malloc_socket(NULL,
-                       vq->size * sizeof(struct async_inflight_info),
-                       RTE_CACHE_LINE_SIZE, vq->numa_node);
-       if (!vq->async_pkts_info) {
-               vhost_free_async_mem(vq);
-               VHOST_LOG_CONFIG(ERR,
-                       "async register failed: cannot allocate memory for
async_pkts_info "
-                       "(vid %d, qid: %d)\n", vid, queue_id);
+       async = rte_zmalloc_socket(NULL, sizeof(struct vhost_async), 0,
node);
+       if (!async) {
+               VHOST_LOG_CONFIG(ERR, "failed to allocate async metadata
(vid %d, qid: %d)\n",
+                               vid, queue_id);
                return -1;
        }

-       vq->it_pool = rte_malloc_socket(NULL,
-                       VHOST_MAX_ASYNC_IT * sizeof(struct
rte_vhost_iov_iter),
-                       RTE_CACHE_LINE_SIZE, vq->numa_node);
-       if (!vq->it_pool) {
-               vhost_free_async_mem(vq);
-               VHOST_LOG_CONFIG(ERR,
-                       "async register failed: cannot allocate memory for
it_pool "
-                       "(vid %d, qid: %d)\n", vid, queue_id);
-               return -1;
-       }
-
-       vq->vec_pool = rte_malloc_socket(NULL,
-                       VHOST_MAX_ASYNC_VEC * sizeof(struct iovec),
-                       RTE_CACHE_LINE_SIZE, vq->numa_node);
-       if (!vq->vec_pool) {
-               vhost_free_async_mem(vq);
-               VHOST_LOG_CONFIG(ERR,
-                       "async register failed: cannot allocate memory for
vec_pool "
-                       "(vid %d, qid: %d)\n", vid, queue_id);
-               return -1;
+       async->pkts_info = rte_malloc_socket(NULL, vq->size * sizeof(struct
async_inflight_info),
+                       RTE_CACHE_LINE_SIZE, node);
+       if (async->pkts_info) {

It should be "if (!async->pkts_info)".

Correct.

You can notice I didn't lie when I said it was only compile-tested in
the cover-letter! :)

Regards,
Maxime

Thanks,
Jiayu