From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E05D6A0559; Tue, 17 Mar 2020 02:01:22 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 20BB71C06A; Tue, 17 Mar 2020 02:01:22 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 9E1111C06A for ; Tue, 17 Mar 2020 02:01:19 +0100 (CET) IronPort-SDR: czkdWEvLEf7AoCC7bcyCz7z07Il45Z3GSBxi33PCRR2I55ledGcKLoO7CjqPyu9jlMbMJJPfUj ifBX5B2lZLEw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Mar 2020 18:01:18 -0700 IronPort-SDR: ZeFYftt6Y1uihEslzez7qkhvZonQFCzWnjrvtwP3sKkmXLmjZp0IkpFG9NmQJlzDvDw2I0VJbR iaEsnzG9Fa/w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,562,1574150400"; d="scan'208";a="390895453" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga004.jf.intel.com with ESMTP; 16 Mar 2020 18:01:17 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 16 Mar 2020 18:01:17 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 16 Mar 2020 18:01:17 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.137]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.96]) with mapi id 14.03.0439.000; Tue, 17 Mar 2020 09:01:14 +0800 From: "Liu, Yong" To: "Ye, Xiaolong" CC: "maxime.coquelin@redhat.com" , "Wang, Zhihong" , "dev@dpdk.org" Thread-Topic: [PATCH] vhost: cache guest/vhost physical address mapping Thread-Index: AQHV+2iRFyB1hGfm0UKbtbHAKu7UW6hKtiyAgAE98GA= Date: Tue, 17 Mar 2020 01:01:14 +0000 Message-ID: <86228AFD5BCD8E4EBFD2B90117B5E81E63509D42@SHSMSX103.ccr.corp.intel.com> References: <20200316153353.112897-1-yong.liu@intel.com> <20200316134819.GE64357@intel.com> In-Reply-To: <20200316134819.GE64357@intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] vhost: cache guest/vhost physical address mapping X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Thanks, xiaolong.=20 > -----Original Message----- > From: Ye, Xiaolong > Sent: Monday, March 16, 2020 9:48 PM > To: Liu, Yong > Cc: maxime.coquelin@redhat.com; Wang, Zhihong > ; dev@dpdk.org > Subject: Re: [PATCH] vhost: cache guest/vhost physical address mapping >=20 > Hi, Marvin >=20 > On 03/16, Marvin Liu wrote: > >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, > >now performance is back. > > > >Signed-off-by: Marvin Liu > > > >diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > >index 2087d1400..de2c09e7e 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; >=20 > What about naming it nr_cached_guest_pages to make it more self- > explanatory > as nr_cached is too generic? Agreed, naming is too generic. Will be changed in next version. >=20 > > uint32_t max_guest_pages; > >+ struct guest_page *cached_guest_pages; > > struct guest_page *guest_pages; > > > > int slave_req_fd; > >@@ -554,11 +556,23 @@ gpa_to_hpa(struct virtio_net *dev, uint64_t gpa, > uint64_t size) > > uint32_t i; > > struct guest_page *page; > > > >+ for (i =3D 0; i < dev->nr_cached; i++) { > >+ page =3D &dev->cached_guest_pages[i]; > >+ if (gpa >=3D page->guest_phys_addr && > >+ gpa + size < page->guest_phys_addr + page->size) { > >+ return gpa - page->guest_phys_addr + > >+ page->host_phys_addr; > >+ } > >+ } > >+ > > for (i =3D 0; i < dev->nr_guest_pages; i++) { > > page =3D &dev->guest_pages[i]; > > > > if (gpa >=3D page->guest_phys_addr && > > gpa + size < page->guest_phys_addr + page->size) { > >+ rte_memcpy(&dev->cached_guest_pages[dev- > >nr_cached], > >+ page, sizeof(struct guest_page)); > >+ dev->nr_cached++; > > 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 bd1be0104..573e99066 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) > > } > > > > free(dev->guest_pages); > >+ free(dev->cached_guest_pages); > > dev->guest_pages =3D NULL; > >+ dev->cached_guest_pages =3D NULL; > > > > if (dev->log_addr) { > > munmap((void *)(uintptr_t)dev->log_addr, dev->log_size); > >@@ -905,7 +907,10 @@ add_one_guest_page(struct virtio_net *dev, > uint64_t guest_phys_addr, > > old_pages =3D dev->guest_pages; > > dev->guest_pages =3D realloc(dev->guest_pages, > > dev->max_guest_pages * > sizeof(*page)); > >- if (!dev->guest_pages) { > >+ dev->cached_guest_pages =3D realloc(dev- > >cached_guest_pages, > >+ dev->max_guest_pages * > sizeof(*page)); > >+ dev->nr_cached =3D 0; > >+ if (!dev->guest_pages || !dev->cached_guest_pages) { >=20 > Better to compare pointer to NULL according to DPDK's coding style. >=20 OK, will change it. > > VHOST_LOG_CONFIG(ERR, "cannot realloc > guest_pages\n"); > > free(old_pages); > > return -1; > >@@ -1075,6 +1080,18 @@ vhost_user_set_mem_table(struct virtio_net > **pdev, struct VhostUserMsg *msg, > > } > > } > > >=20 > Do we need initialize dev->nr_cached to 0 explicitly here? >=20 Structure vhost_virtqueue has been cleared in function init_vring_queue, it= is not needed to do initialization in other place. > >+ if (!dev->cached_guest_pages) { > >+ dev->cached_guest_pages =3D malloc(dev->max_guest_pages * > >+ sizeof(struct guest_page)); >=20 > I'm wondering why use malloc/realloc/free for cached_guest_pages instead > of DPDK > memory allocator APIs, I can see current code uses malloc/realloc/free fo= r > guest_pages, > Is there any history reason I don't know? >=20 I don't think there's specific reason to use glibc malloc/realloc functions= .=20 History may be lost since code was added few years ago. I will changed thes= e functions to dpdk API in next version. > Thanks, > Xiaolong >=20 > >+ if (dev->cached_guest_pages =3D=3D 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 =3D rte_zmalloc("vhost-mem-table", sizeof(struct > rte_vhost_memory) + > > sizeof(struct rte_vhost_mem_region) * memory->nregions, > 0); > > if (dev->mem =3D=3D NULL) { > >-- > >2.17.1 > >