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.
>
> - 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)".
Thanks,
Jiayu
