DPDK patches and discussions
 help / color / mirror / Atom feed
From: Gavin Hu <Gavin.Hu@arm.com>
To: "Liu, Yong" <yong.liu@intel.com>,
	"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
	"Ye, Xiaolong" <xiaolong.ye@intel.com>,
	"Wang, Zhihong" <zhihong.wang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, nd <nd@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/2] vhost: cache gpa to hpa translation
Date: Thu, 2 Apr 2020 03:04:38 +0000	[thread overview]
Message-ID: <VI1PR08MB53764E9D3DE8BC67B22274448FC60@VI1PR08MB5376.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <86228AFD5BCD8E4EBFD2B90117B5E81E63524453@SHSMSX103.ccr.corp.intel.com>

Hi Marvin,

> -----Original Message-----
> From: Liu, Yong <yong.liu@intel.com>
> Sent: Wednesday, April 1, 2020 9:01 PM
> To: Gavin Hu <Gavin.Hu@arm.com>; maxime.coquelin@redhat.com; Ye,
> Xiaolong <xiaolong.ye@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/2] vhost: cache gpa to hpa translation
> 
> 
> 
> > -----Original Message-----
> > From: Gavin Hu <Gavin.Hu@arm.com>
> > Sent: Wednesday, April 1, 2020 6:07 PM
> > To: Liu, Yong <yong.liu@intel.com>; maxime.coquelin@redhat.com; Ye,
> > Xiaolong <xiaolong.ye@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>
> > Cc: dev@dpdk.org; nd <nd@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 2/2] vhost: cache gpa to hpa translation
> >
> > Hi Marvin,
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Marvin Liu
> > > Sent: Wednesday, April 1, 2020 10:50 PM
> > > To: maxime.coquelin@redhat.com; xiaolong.ye@intel.com;
> > > zhihong.wang@intel.com
> > > Cc: dev@dpdk.org; Marvin Liu <yong.liu@intel.com>
> > > Subject: [dpdk-dev] [PATCH v2 2/2] vhost: cache gpa to hpa translation
> > >
> > > If Tx zero copy enabled, gpa to hpa mapping table is updated one by
> > > one. This will harm performance when guest memory backend using 2M
> > > hugepages. Now add cached mapping table which will sorted by using
> > > sequence. Address translation will first check cached mapping table,
> > > then check unsorted mapping table if no match found.
> > >
> > > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> > >
> > > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> > > index 2087d1400..5cb0e83dd 100644
> > > --- a/lib/librte_vhost/vhost.h
> > > +++ b/lib/librte_vhost/vhost.h
> > > @@ -368,7 +368,9 @@ struct virtio_net {
> > >  	struct vhost_device_ops const *notify_ops;
> > >
> > >  	uint32_t		nr_guest_pages;
> > > +	uint32_t		nr_cached_guest_pages;
> > >  	uint32_t		max_guest_pages;
> > > +	struct guest_page       *cached_guest_pages;
> > >  	struct guest_page       *guest_pages;
> > >
> > >  	int			slave_req_fd;
> > > @@ -553,12 +555,25 @@ gpa_to_hpa(struct virtio_net *dev, uint64_t
> gpa,
> > > uint64_t size)
> > >  {
> > >  	uint32_t i;
> > >  	struct guest_page *page;
> > > +	uint32_t cached_pages = dev->nr_cached_guest_pages;
> > > +

Add a comment here, something like "Firstly look up in the cached pages"? 

> > > +	for (i = 0; i < cached_pages; i++) {

Should the searching order reversed  here to search the most recent entries? 

> > > +		page = &dev->cached_guest_pages[i];
> > > +		if (gpa >= page->guest_phys_addr &&
> > > +			gpa + size < page->guest_phys_addr + page->size) {
> > > +			return gpa - page->guest_phys_addr +
> > > +				page->host_phys_addr;
> > > +		}
> > > +	}
> > Sorry, I did not see any speedup with cached guest pages in comparison to
> > the old code below.
> > Is it not a simple copy?
> > Is it a better idea to use hash instead to speed up the translation?
> > /Gavin
> 
> Hi Gavin,
> Here just resort the overall mapping table according to using sequence.
> Most likely virtio driver will reuse recently cycled buffers, thus search will
> find match in beginning.
> That is simple fix for performance enhancement. If use hash for index, it will
> take much more cost in normal case.
> 
> Regards,
> Marvin

There are issues here, the cached table is growing over time, will it become less efficient when growing too big and even bigger than the original table and overflow happen? 
Is it a good idea to limit the cached entries to small therefore make the search quicker? 
/Gavin
> 
> 
> > >
Add a comment here, something like "Fall back to normal lookup if failed to get from the cached table"? 

> > >  	for (i = 0; i < dev->nr_guest_pages; i++) {
> > >  		page = &dev->guest_pages[i];
> > >
> > >  		if (gpa >= page->guest_phys_addr &&
> > >  		    gpa + size < page->guest_phys_addr + page->size) {
> > > +			rte_memcpy(&dev-
> > > >cached_guest_pages[cached_pages],
> > > +				   page, sizeof(struct guest_page));
> > > +			dev->nr_cached_guest_pages++;

Will overflow happen over time when there are many translations? 

> > >  			return gpa - page->guest_phys_addr +
> > >  			       page->host_phys_addr;
> > >  		}
> > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > > index 79fcb9d19..1bae1fddc 100644
> > > --- a/lib/librte_vhost/vhost_user.c
> > > +++ b/lib/librte_vhost/vhost_user.c
> > > @@ -192,7 +192,9 @@ vhost_backend_cleanup(struct virtio_net *dev)
> > >  	}
> > >
> > >  	rte_free(dev->guest_pages);
> > > +	rte_free(dev->cached_guest_pages);
> > >  	dev->guest_pages = NULL;
> > > +	dev->cached_guest_pages = NULL;
> > >
> > >  	if (dev->log_addr) {
> > >  		munmap((void *)(uintptr_t)dev->log_addr, dev->log_size);
> > > @@ -898,7 +900,7 @@ add_one_guest_page(struct virtio_net *dev,
> > > uint64_t guest_phys_addr,
> > >  		   uint64_t host_phys_addr, uint64_t size)
> > >  {
> > >  	struct guest_page *page, *last_page;
> > > -	struct guest_page *old_pages;
> > > +	struct guest_page *old_pages, *old_cached_pages;
> > >
> > >  	if (dev->nr_guest_pages == dev->max_guest_pages) {
> > >  		dev->max_guest_pages *= 2;
> > > @@ -906,9 +908,19 @@ add_one_guest_page(struct virtio_net *dev,
> > > uint64_t guest_phys_addr,
> > >  		dev->guest_pages = rte_realloc(dev->guest_pages,
> > >  					dev->max_guest_pages *
> > > sizeof(*page),
> > >  					RTE_CACHE_LINE_SIZE);
> > > -		if (dev->guest_pages == NULL) {
> > > +		old_cached_pages = dev->cached_guest_pages;
> > > +		dev->cached_guest_pages = rte_realloc(dev-
> > > >cached_guest_pages,
> > > +						dev->max_guest_pages *
> > > +						sizeof(*page),
> > > +						RTE_CACHE_LINE_SIZE);
> > > +		dev->nr_cached_guest_pages = 0;
> > > +		if (dev->guest_pages == NULL ||
> > > +				dev->cached_guest_pages == NULL) {
> > >  			VHOST_LOG_CONFIG(ERR, "cannot realloc
> > > guest_pages\n");
> > >  			rte_free(old_pages);
> > > +			rte_free(old_cached_pages);
> > > +			dev->guest_pages = NULL;
> > > +			dev->cached_guest_pages = NULL;
> > >  			return -1;
> > >  		}
> > >  	}
> > > @@ -1078,6 +1090,20 @@ vhost_user_set_mem_table(struct virtio_net
> > > **pdev, struct VhostUserMsg *msg,
> > >  		}
> > >  	}
> > >
> > > +	if (dev->cached_guest_pages == NULL) {
> > > +		dev->cached_guest_pages = rte_zmalloc(NULL,
> > > +						dev->max_guest_pages *
> > > +						sizeof(struct guest_page),
> > > +						RTE_CACHE_LINE_SIZE);
> > > +		if (dev->cached_guest_pages == NULL) {
> > > +			VHOST_LOG_CONFIG(ERR,
> > > +				"(%d) failed to allocate memory "
> > > +				"for dev->cached_guest_pages\n",
> > > +				dev->vid);
> > > +			return RTE_VHOST_MSG_RESULT_ERR;
> > > +		}
> > > +	}
> > > +
> > >  	dev->mem = rte_zmalloc("vhost-mem-table", sizeof(struct
> > > rte_vhost_memory) +
> > >  		sizeof(struct rte_vhost_mem_region) * memory->nregions,
> > > 0);
> > >  	if (dev->mem == NULL) {
> > > --
> > > 2.17.1


  reply	other threads:[~2020-04-02  3:04 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-16 15:33 [dpdk-dev] [PATCH] vhost: cache guest/vhost physical address mapping Marvin Liu
2020-03-16 13:48 ` Ye Xiaolong
2020-03-17  1:01   ` Liu, Yong
2020-04-01 14:50 ` [dpdk-dev] [PATCH v2 1/2] vhost: utilize dpdk dynamic memory allocator Marvin Liu
2020-04-01 10:08   ` Gavin Hu
2020-04-01 14:50   ` [dpdk-dev] [PATCH v2 2/2] vhost: cache gpa to hpa translation Marvin Liu
2020-04-01 10:07     ` Gavin Hu
2020-04-01 13:01       ` Liu, Yong
2020-04-02  3:04         ` Gavin Hu [this message]
2020-04-02  4:45           ` Liu, Yong
2020-04-03  8:22         ` Ma, LihongX
2020-04-02  2:57     ` Ye Xiaolong
2020-04-27  8:45     ` Maxime Coquelin
2020-04-28  0:44       ` Liu, Yong
2020-04-02  2:57   ` [dpdk-dev] [PATCH v2 1/2] vhost: utilize dpdk dynamic memory allocator Ye Xiaolong
2020-04-03  8:22   ` Ma, LihongX
2020-04-15 11:15   ` Maxime Coquelin
2020-04-28  9:13 ` [dpdk-dev] [PATCH v3 " Marvin Liu
2020-04-28  9:13   ` [dpdk-dev] [PATCH v3 2/2] vhost: binary search address mapping table Marvin Liu
2020-04-28 15:28     ` Maxime Coquelin
2020-04-28 15:38       ` Liu, Yong
2020-04-28 12:51   ` [dpdk-dev] [PATCH v3 1/2] vhost: utilize dpdk dynamic memory allocator Maxime Coquelin
2020-04-29  1:00 ` [dpdk-dev] [PATCH v4 1/2] net/virtio: add support Virtio link speed feature Marvin Liu
2020-04-29  1:00   ` [dpdk-dev] [PATCH v4 1/2] vhost: utilize dpdk dynamic memory allocator Marvin Liu
2020-04-29  1:00   ` [dpdk-dev] [PATCH v4 2/2] vhost: binary search address mapping table Marvin Liu
2020-04-29  1:01   ` [dpdk-dev] [PATCH v4 2/2] vhost: utilize dpdk dynamic memory allocator Marvin Liu
2020-04-29  1:06     ` Liu, Yong
2020-04-29 17:47       ` Maxime Coquelin
2020-04-29  1:04 ` [dpdk-dev] [PATCH v4 1/2] " Marvin Liu
2020-04-29  1:04   ` [dpdk-dev] [PATCH v4 2/2] vhost: binary search address mapping table Marvin Liu
2020-04-29 11:50     ` 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=VI1PR08MB53764E9D3DE8BC67B22274448FC60@VI1PR08MB5376.eurprd08.prod.outlook.com \
    --to=gavin.hu@arm.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=nd@arm.com \
    --cc=xiaolong.ye@intel.com \
    --cc=yong.liu@intel.com \
    --cc=zhihong.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).