From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 373C9A0C4C; Thu, 14 Oct 2021 10:54:36 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EF44B40042; Thu, 14 Oct 2021 10:54:35 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 4508140041 for ; Thu, 14 Oct 2021 10:54:34 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1634201673; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=72XYuHV8OEDct3mi7GjOjNx2AUZU6X5uZYfYa7uAh38=; b=OxF8cIueapof96Ejaigji94Dlic7YW6bvj+xBS/jaMue4SkVxYZ4aKu8LDE6wFPUu30rub rzxhIheAa8IxXz/7r7qDY9Ef1W7gh9nsErUXc0zJOnjQQbrgxbSeDYWvXxkc+yoMKO01yY ZbgvCb0k4A8WWeozS2XFJ7s4Lrnns00= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-464-2yB6dSS6O6aoAcf97cHdEA-1; Thu, 14 Oct 2021 04:54:30 -0400 X-MC-Unique: 2yB6dSS6O6aoAcf97cHdEA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CD8C51006AA4; Thu, 14 Oct 2021 08:54:28 +0000 (UTC) Received: from [10.39.208.20] (unknown [10.39.208.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3776419D9F; Thu, 14 Oct 2021 08:54:27 +0000 (UTC) Message-ID: <331ece96-7420-448f-aec3-40ebd3d6a0c9@redhat.com> Date: Thu, 14 Oct 2021 10:54:25 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 To: "Hu, Jiayu" , "dev@dpdk.org" , "Xia, Chenbo" , "Wang, YuanX" , "Ma, WenwuX" , "Richardson, Bruce" , "Mcnamara, John" References: <20211007220013.355530-1-maxime.coquelin@redhat.com> <20211007220013.355530-2-maxime.coquelin@redhat.com> <8cec24ee308a496eb48734c0cd535639@intel.com> From: Maxime Coquelin In-Reply-To: <8cec24ee308a496eb48734c0cd535639@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC 01/14] vhost: move async data in a dedicated structure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/14/21 05:24, Hu, Jiayu wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Friday, October 8, 2021 6:00 AM >> To: dev@dpdk.org; Xia, Chenbo ; Hu, Jiayu >> ; Wang, YuanX ; Ma, >> WenwuX ; Richardson, Bruce >> ; Mcnamara, John >> >> Cc: Maxime Coquelin >> 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 >> --- >> 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 >