DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Hu, Jiayu" <jiayu.hu@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"Xia, Chenbo" <chenbo.xia@intel.com>,
	"Wang, YuanX" <yuanx.wang@intel.com>,
	"Ma, WenwuX" <wenwux.ma@intel.com>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	"Mcnamara, John" <john.mcnamara@intel.com>
Subject: Re: [dpdk-dev] [RFC 01/14] vhost: move async data in a dedicated structure
Date: Thu, 14 Oct 2021 03:24:18 +0000	[thread overview]
Message-ID: <8cec24ee308a496eb48734c0cd535639@intel.com> (raw)
In-Reply-To: <20211007220013.355530-2-maxime.coquelin@redhat.com>

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, October 8, 2021 6:00 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Wang, YuanX <yuanx.wang@intel.com>; Ma,
> WenwuX <wenwux.ma@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@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.coquelin@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

  reply	other threads:[~2021-10-14  3:24 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-07 21:59 [dpdk-dev] [RFC 00/14] vhost: clean-up and simplify async implementation Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 01/14] vhost: move async data in a dedicated structure Maxime Coquelin
2021-10-14  3:24   ` Hu, Jiayu [this message]
2021-10-14  8:54     ` Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 02/14] vhost: hide inflight async structure Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 03/14] vhost: simplify async IO vectors Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 04/14] vhost: simplify async IO vectors iterators Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 05/14] vhost: remove async batch threshold Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 06/14] vhost: introduce specific iovec structure Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 07/14] vhost: remove useless fields in async iterator struct Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 08/14] vhost: improve IO vector logic Maxime Coquelin
2021-10-12  6:05   ` Hu, Jiayu
2021-10-12  8:34     ` Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 09/14] vhost: remove notion of async descriptor Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 10/14] vhost: simplify async enqueue completion Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 11/14] vhost: simplify getting the first in-flight index Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 12/14] vhost: prepare async for mbuf to desc refactoring Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 13/14] vhost: prepare sync " Maxime Coquelin
2021-10-07 22:00 ` [dpdk-dev] [RFC 14/14] vhost: merge sync and async mbuf to desc filling Maxime Coquelin
2021-10-08 12:36 ` [dpdk-dev] [RFC 00/14] vhost: clean-up and simplify async implementation David Marchand
2021-10-12  6:24 ` Hu, Jiayu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8cec24ee308a496eb48734c0cd535639@intel.com \
    --to=jiayu.hu@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=john.mcnamara@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=wenwux.ma@intel.com \
    --cc=yuanx.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).