> -----Original Message----- > From: Burakov, Anatoly [mailto:anatoly.bura...@intel.com] > Sent: Monday, July 27, 2020 5:24 PM > To: wangyunjian <wangyunj...@huawei.com>; dev@dpdk.org; > david.march...@redhat.com > Cc: Lilijun (Jerry) <jerry.lili...@huawei.com>; xudingke > <xudin...@huawei.com>; sta...@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] eal/linux: do not create user mem map > repeatedly when it exists > > On 25-Jul-20 10:59 AM, wangyunjian wrote: > >> -----Original Message----- > >> From: Burakov, Anatoly [mailto:anatoly.bura...@intel.com] > >> Sent: Friday, July 24, 2020 9:25 PM > >> To: wangyunjian <wangyunj...@huawei.com>; dev@dpdk.org; > >> david.march...@redhat.com > >> Cc: Lilijun (Jerry) <jerry.lili...@huawei.com>; xudingke > >> <xudin...@huawei.com>; sta...@dpdk.org > >> Subject: Re: [dpdk-dev] [PATCH v2] eal/linux: do not create user mem > >> map repeatedly when it exists > >> > >> On 23-Jul-20 3:48 PM, wangyunjian wrote: > >>> From: Yunjian Wang <wangyunj...@huawei.com> > >>> > >>> Currently, we will create new user mem map entry for the same memory > >>> segment, but in fact it has already been added to the user mem maps. > >>> It's not necessary to create it twice. > >>> > >>> To resolve the issue, add support to remove the same entry in the > >>> function compact_user_maps(). > >>> > >>> Fixes: 0cbce3a167f1 ("vfio: skip DMA map failure if already mapped") > >>> Cc: sta...@dpdk.org > >>> > >>> Signed-off-by: Yunjian Wang <wangyunj...@huawei.com> > >>> --- > >>> v2: > >>> * Remove the same entry in the function compact_user_maps() > >>> --- > >>> lib/librte_eal/linux/eal_vfio.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/lib/librte_eal/linux/eal_vfio.c > >>> b/lib/librte_eal/linux/eal_vfio.c index abb12a354..df99307b7 100644 > >>> --- a/lib/librte_eal/linux/eal_vfio.c > >>> +++ b/lib/librte_eal/linux/eal_vfio.c > >>> @@ -167,6 +167,10 @@ adjust_map(struct user_mem_map *src, struct > >> user_mem_map *end, > >>> static int > >>> merge_map(struct user_mem_map *left, struct user_mem_map > *right) > >>> { > >>> + /* merge the same maps into one */ > >>> + if (memcmp(left, right, sizeof(struct user_mem_map)) == 0) > >>> + goto out; > >>> + > >> > >> merge_map looks for adjacent maps only, but does not handle maps that > >> are wholly contained within one another ("the same map" also matches > >> this definition). wouldn't it be better to check for that instead of > >> *just* handling identical maps? > > > > What about using the initial implementation? > > We don't create new user mem map entry for the same memory segment. > > I don't like this implementation because it relies on particulars of how VFIO > mapping work without explicitly specifying them. I.e. it's prone to breaking > when changing code. That's not even mentioning that we have no guarantees > on kernel behavior in that particular case being identical on all supported > platforms. > > I would honestly prefer an explicit compaction over implicit one.
What about this implementation? diff --git a/lib/librte_eal/linux/eal_vfio.c b/lib/librte_eal/linux/eal_vfio.c index e07979936..8dcb04cd9 100644 --- a/lib/librte_eal/linux/eal_vfio.c +++ b/lib/librte_eal/linux/eal_vfio.c @@ -179,6 +179,19 @@ merge_map(struct user_mem_map *left, struct user_mem_map *right) return 1; } +/* try merging two same maps into one, return 1 if succeeded */ +static int +merge_same_map(struct user_mem_map *left, struct user_mem_map *right) +{ + if (memcmp(left, right, sizeof(struct user_mem_map)) != 0) { + return 0; + } + + memset(right, 0, sizeof(*right)); + + return 1; +} + static struct user_mem_map * find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr, uint64_t iova, uint64_t len) @@ -232,7 +245,7 @@ compact_user_maps(struct user_mem_maps *user_mem_maps) if (is_null_map(l) || is_null_map(r)) continue; - if (merge_map(l, r)) + if (merge_map(l, r) || merge_same_map(l, r)) n_merged++; } Thanks, Yunjian > > > > > @@ -1828,6 +1828,13 @@ container_dma_map(struct vfio_config > *vfio_cfg, uint64_t vaddr, uint64_t iova, > > ret = -1; > > goto out; > > } > > + > > + /* we don't need create new user mem map entry > > + * for the same memory segment. > > + */ > > + if (errno == EBUSY || errno == EEXIST) > > + goto out; > > + > > /* create new user mem map entry */ > > new_map = > &user_mem_maps->maps[user_mem_maps->n_maps++]; > > new_map->addr = vaddr; > > > > Thanks, > > Yunjian > >> > >>> if (left->addr + left->len != right->addr) > >>> return 0; > >>> if (left->iova + left->len != right->iova) @@ -174,6 +178,7 @@ > >>> merge_map(struct user_mem_map *left, struct > >> user_mem_map *right) > >>> > >>> left->len += right->len; > >>> > >>> +out: > >>> memset(right, 0, sizeof(*right)); > >>> > >>> return 1; > >>> > >> > >> > >> -- > >> Thanks, > >> Anatoly > > > -- > Thanks, > Anatoly