DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Jiang, Cheng1" <cheng1.jiang@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
	"Xia, Chenbo" <chenbo.xia@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "Hu, Jiayu" <jiayu.hu@intel.com>,
	"Yang, YvonneX" <yvonnex.yang@intel.com>,
	"Wang, Yinan" <yinan.wang@intel.com>,
	"Liu, Yong" <yong.liu@intel.com>
Subject: Re: [dpdk-dev] [PATCH v7 2/4] vhost: add support for packed ring in async vhost
Date: Thu, 15 Apr 2021 05:42:58 +0000
Message-ID: <SJ0PR11MB5006B2C1BAF2358BDE23AB9EDC4D9@SJ0PR11MB5006.namprd11.prod.outlook.com> (raw)
In-Reply-To: <eace3d9f-c40f-4298-f23b-e04709fb2cea@redhat.com>

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, April 14, 2021 9:41 PM
> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Wang, Yinan <yinan.wang@intel.com>; Liu,
> Yong <yong.liu@intel.com>
> Subject: Re: [PATCH v7 2/4] vhost: add support for packed ring in async vhost
> 
> 
> 
> On 4/14/21 8:13 AM, Cheng Jiang wrote:
> > For now async vhost data path only supports split ring. This patch
> > enables packed ring in async vhost data path to make async vhost
> > compatible with virtio 1.1 spec.
> >
> > Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> > ---
> >  lib/librte_vhost/rte_vhost_async.h |   1 +
> >  lib/librte_vhost/vhost.c           |  49 ++--
> >  lib/librte_vhost/vhost.h           |  15 +-
> >  lib/librte_vhost/virtio_net.c      | 432 +++++++++++++++++++++++++++--
> >  4 files changed, 456 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/librte_vhost/rte_vhost_async.h
> > b/lib/librte_vhost/rte_vhost_async.h
> > index c855ff875..6faa31f5a 100644
> > --- a/lib/librte_vhost/rte_vhost_async.h
> > +++ b/lib/librte_vhost/rte_vhost_async.h
> > @@ -89,6 +89,7 @@ struct rte_vhost_async_channel_ops {  struct
> > async_inflight_info {
> >  	struct rte_mbuf *mbuf;
> >  	uint16_t descs; /* num of descs inflight */
> > +	uint16_t nr_buffers; /* num of buffers inflight for packed ring */
> >  };
> >
> >  /**
> > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index
> > a70fe01d8..f509186c6 100644
> > --- a/lib/librte_vhost/vhost.c
> > +++ b/lib/librte_vhost/vhost.c
> > @@ -338,19 +338,22 @@ cleanup_device(struct virtio_net *dev, int
> > destroy)  }
> >
> >  static void
> > -vhost_free_async_mem(struct vhost_virtqueue *vq)
> > +vhost_free_async_mem(struct virtio_net *dev, struct vhost_virtqueue
> > +*vq)
> >  {
> > -	if (vq->async_pkts_info)
> > -		rte_free(vq->async_pkts_info);
> > -	if (vq->async_descs_split)
> > +	rte_free(vq->async_pkts_info);
> > +
> > +	if (vq_is_packed(dev)) {
> > +		rte_free(vq->async_buffers_packed);
> > +		vq->async_buffers_packed = NULL;
> > +	} else {
> 
> Doing this is not necessary:
> 
> 	rte_free(vq->async_buffers_packed);
> 	vq->async_buffers_packed = NULL;
> 	rte_free(vq->async_descs_split);
> 	vq->async_descs_split = NULL;
> 
> Above will just work and will avoid adding dev parameter.

async_buffers_packed and async_descs_split are two members of a union.
If I rte_free(vq->async_buffers_packed), then there is no need to rte_free(vq->async_descs_split).
So I use dev to determine which one I need to free.
Sure, I can free then both with no condition(I can also only free any one of them), but I think it's a little strange, Right?

> 
> 
> >  		rte_free(vq->async_descs_split);
> > -	if (vq->it_pool)
> > -		rte_free(vq->it_pool);
> > -	if (vq->vec_pool)
> > -		rte_free(vq->vec_pool);
> > +		vq->async_descs_split = NULL;
> > +	}
> > +
> > +	rte_free(vq->it_pool);
> > +	rte_free(vq->vec_pool);
> >
> >  	vq->async_pkts_info = NULL;
> > -	vq->async_descs_split = NULL;
> >  	vq->it_pool = NULL;
> >  	vq->vec_pool = NULL;
> >  }
> > @@ -360,10 +363,10 @@ free_vq(struct virtio_net *dev, struct
> > vhost_virtqueue *vq)  {
> >  	if (vq_is_packed(dev))
> >  		rte_free(vq->shadow_used_packed);
> > -	else {
> > +	else
> >  		rte_free(vq->shadow_used_split);
> > -		vhost_free_async_mem(vq);
> > -	}
> > +
> > +	vhost_free_async_mem(dev, vq);
> >  	rte_free(vq->batch_copy_elems);
> >  	if (vq->iotlb_pool)
> >  		rte_mempool_free(vq->iotlb_pool);
> > @@ -1626,10 +1629,9 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  	if (unlikely(vq == NULL || !dev->async_copy))
> >  		return -1;
> >
> > -	/* packed queue is not supported */
> > -	if (unlikely(vq_is_packed(dev) || !f.async_inorder)) {
> > +	if (unlikely(!f.async_inorder)) {
> >  		VHOST_LOG_CONFIG(ERR,
> > -			"async copy is not supported on packed queue or
> non-inorder mode "
> > +			"async copy is not supported on non-inorder mode "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> >  		return -1;
> >  	}
> > @@ -1667,12 +1669,19 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  	vq->vec_pool = rte_malloc_socket(NULL,
> >  			VHOST_MAX_ASYNC_VEC * sizeof(struct iovec),
> >  			RTE_CACHE_LINE_SIZE, node);
> > -	vq->async_descs_split = rte_malloc_socket(NULL,
> > +	if (vq_is_packed(dev)) {
> > +		vq->async_buffers_packed = rte_malloc_socket(NULL,
> > +			vq->size * sizeof(struct vring_used_elem_packed),
> > +			RTE_CACHE_LINE_SIZE, node);
> > +	} else {
> > +		vq->async_descs_split = rte_malloc_socket(NULL,
> >  			vq->size * sizeof(struct vring_used_elem),
> >  			RTE_CACHE_LINE_SIZE, node);
> > -	if (!vq->async_descs_split || !vq->async_pkts_info ||
> > -		!vq->it_pool || !vq->vec_pool) {
> > -		vhost_free_async_mem(vq);
> > +	}
> > +
> > +	if (!vq->async_buffers_packed || !vq->async_descs_split ||
> > +		!vq->async_pkts_info || !vq->it_pool || !vq->vec_pool) {
> 
> Not really than of this error handling. Checking after every malloc if it suceed
> would be cleaner.

OK, make sense to me. But that means I need to add log(which can be more specific) after every check, do you thinks it's ok?
If you thinks it's ok, then I'll fix it in the next version.

> 
> > +		vhost_free_async_mem(dev, vq);
> >  		VHOST_LOG_CONFIG(ERR,
> >  				"async register failed: cannot allocate
> memory for vq data "
> >  				"(vid %d, qid: %d)\n", vid, queue_id); @@ -
> 1728,7 +1737,7 @@ int
> > rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
> >  		goto out;
> >  	}
> >
> > -	vhost_free_async_mem(vq);
> > +	vhost_free_async_mem(dev, vq);
> >
> >  	vq->async_ops.transfer_data = NULL;
> >  	vq->async_ops.check_completed_copies = NULL; diff --git
> > a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
> > f628714c2..673335217 100644
> > --- a/lib/librte_vhost/vhost.h
> > +++ b/lib/librte_vhost/vhost.h
> > @@ -201,9 +201,18 @@ struct vhost_virtqueue {
> >  	uint16_t	async_pkts_idx;
> >  	uint16_t	async_pkts_inflight_n;
> >  	uint16_t	async_last_pkts_n;
> > -	struct vring_used_elem  *async_descs_split;
> > -	uint16_t async_desc_idx;
> > -	uint16_t last_async_desc_idx;
> > +	union {
> > +		struct vring_used_elem  *async_descs_split;
> > +		struct vring_used_elem_packed *async_buffers_packed;
> > +	};
> > +	union {
> > +		uint16_t async_desc_idx;
> > +		uint16_t async_packed_buffer_idx;
> > +	};
> > +	union {
> > +		uint16_t last_async_desc_idx;
> > +		uint16_t last_async_buffer_idx;
> > +	};
> 
> Looks almost good to me now, thanks for doing the change.
> Only minor issue is the naming which is not consistent in the different fields.
> Sometimes it contains split or packed, sometimes not.

OK, I'll check these names, and fix them.

Thanks a lot.
Cheng

> 
> Maxime


  reply	other threads:[~2021-04-15  5:43 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-17  8:54 [dpdk-dev] [PATCH] " Cheng Jiang
2021-03-22  6:15 ` [dpdk-dev] [PATCH v2] " Cheng Jiang
2021-03-24  9:19   ` Liu, Yong
2021-03-29 12:29     ` Jiang, Cheng1
2021-03-31 14:06 ` [dpdk-dev] [PATCH v3] " Cheng Jiang
2021-04-07  6:26   ` Hu, Jiayu
2021-04-08 12:01     ` Jiang, Cheng1
2021-04-10 10:25 ` [dpdk-dev] [PATCH v4 0/4] " Cheng Jiang
2021-04-10 10:25   ` [dpdk-dev] [PATCH v4 1/4] vhost: abstract and reorganize async split ring code Cheng Jiang
2021-04-10 10:25   ` Cheng Jiang
2021-04-10 10:25   ` [dpdk-dev] [PATCH v4 2/4] vhost: add support for packed ring in async vhost Cheng Jiang
2021-04-10 10:25   ` [dpdk-dev] [PATCH v4 3/4] vhost: add batch datapath for async vhost packed ring Cheng Jiang
2021-04-10 10:25   ` [dpdk-dev] [PATCH v4 4/4] doc: add release note for vhost async " Cheng Jiang
2021-04-12 11:34 ` [dpdk-dev] [PATCH v5 0/4] add support for packed ring in async vhost Cheng Jiang
2021-04-12 11:34   ` [dpdk-dev] [PATCH v5 1/4] vhost: abstract and reorganize async split ring code Cheng Jiang
2021-04-13  2:44     ` Hu, Jiayu
2021-04-13  3:26       ` Jiang, Cheng1
2021-04-13  7:11     ` Maxime Coquelin
2021-04-13  9:06       ` Jiang, Cheng1
2021-04-12 11:34   ` [dpdk-dev] [PATCH v5 2/4] vhost: add support for packed ring in async vhost Cheng Jiang
2021-04-13  8:36     ` Maxime Coquelin
2021-04-13 11:48       ` Jiang, Cheng1
2021-04-13 13:08         ` Maxime Coquelin
2021-04-13 13:50           ` Jiang, Cheng1
2021-04-12 11:34   ` [dpdk-dev] [PATCH v5 3/4] vhost: add batch datapath for async vhost packed ring Cheng Jiang
2021-04-12 11:34   ` [dpdk-dev] [PATCH v5 4/4] doc: add release note for vhost async " Cheng Jiang
2021-04-13 14:55 ` [dpdk-dev] [PATCH v6 0/4] add support for packed ring in async vhost Cheng Jiang
2021-04-13 14:55   ` [dpdk-dev] [PATCH v6 1/4] vhost: abstract and reorganize async split ring code Cheng Jiang
2021-04-13 14:55   ` [dpdk-dev] [PATCH v6 2/4] vhost: add support for packed ring in async vhost Cheng Jiang
2021-04-13 14:55   ` [dpdk-dev] [PATCH v6 3/4] vhost: add batch datapath for async vhost packed ring Cheng Jiang
2021-04-13 14:55   ` [dpdk-dev] [PATCH v6 4/4] doc: add release note for vhost async " Cheng Jiang
2021-04-14  6:13 ` [dpdk-dev] [PATCH v7 0/4] add support for packed ring in async vhost Cheng Jiang
2021-04-14  6:13   ` [dpdk-dev] [PATCH v7 1/4] vhost: abstract and reorganize async split ring code Cheng Jiang
2021-04-14 12:24     ` Maxime Coquelin
2021-04-14  6:13   ` [dpdk-dev] [PATCH v7 2/4] vhost: add support for packed ring in async vhost Cheng Jiang
2021-04-14 13:40     ` Maxime Coquelin
2021-04-15  5:42       ` Jiang, Cheng1 [this message]
2021-04-15  2:02     ` Hu, Jiayu
2021-04-15  5:54       ` Jiang, Cheng1
2021-04-14  6:13   ` [dpdk-dev] [PATCH v7 3/4] vhost: add batch datapath for async vhost packed ring Cheng Jiang
2021-04-14  6:13   ` [dpdk-dev] [PATCH v7 4/4] doc: add release note for vhost async " Cheng Jiang
2021-04-19  8:51 ` [dpdk-dev] [PATCH v8 0/4] add support for packed ring in async vhost Cheng Jiang
2021-04-19  8:51   ` [dpdk-dev] [PATCH v8 1/4] vhost: abstract and reorganize async split ring code Cheng Jiang
2021-04-27  1:19     ` Hu, Jiayu
2021-04-19  8:51   ` [dpdk-dev] [PATCH v8 2/4] vhost: add support for packed ring in async vhost Cheng Jiang
2021-04-27  5:16     ` Hu, Jiayu
2021-04-27  6:07       ` Jiang, Cheng1
2021-04-19  8:51   ` [dpdk-dev] [PATCH v8 3/4] vhost: add batch datapath for async vhost packed ring Cheng Jiang
2021-04-19  8:51   ` [dpdk-dev] [PATCH v8 4/4] doc: add release note for vhost async " Cheng Jiang
2021-04-27  8:03 ` [dpdk-dev] [PATCH v9 0/4] add support for packed ring in async vhost Cheng Jiang
2021-04-27  8:03   ` [dpdk-dev] [PATCH v9 1/4] vhost: abstract and reorganize async split ring code Cheng Jiang
2021-04-27  8:03   ` [dpdk-dev] [PATCH v9 2/4] vhost: add support for packed ring in async vhost Cheng Jiang
2021-04-29  1:48     ` Hu, Jiayu
2021-04-29  9:50     ` Maxime Coquelin
2021-04-27  8:03   ` [dpdk-dev] [PATCH v9 3/4] vhost: add batch datapath for async vhost packed ring Cheng Jiang
2021-04-29  9:57     ` Maxime Coquelin
2021-04-27  8:03   ` [dpdk-dev] [PATCH v9 4/4] doc: add release note for vhost async " Cheng Jiang
2021-04-29  9:58     ` Maxime Coquelin
2021-05-04 18:38     ` Ferruh Yigit
2021-05-04  8:28   ` [dpdk-dev] [PATCH v9 0/4] add support for packed ring in async vhost Maxime Coquelin

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=SJ0PR11MB5006B2C1BAF2358BDE23AB9EDC4D9@SJ0PR11MB5006.namprd11.prod.outlook.com \
    --to=cheng1.jiang@intel.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=jiayu.hu@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=yinan.wang@intel.com \
    --cc=yong.liu@intel.com \
    --cc=yvonnex.yang@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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git