[dpdk-dev] [PATCH v2] eal/linux: do not create user mem map repeatedly when it exists

[wangyunjian]

> -----Original Message-----
> From: Burakov, Anatoly [mailto:anatoly.burakov at intel.com]
> Sent: Friday, July 24, 2020 9:25 PM
> To: wangyunjian <wangyunjian at huawei.com>; dev at dpdk.org;
> david.marchand at redhat.com
> Cc: Lilijun (Jerry) <jerry.lilijun at huawei.com>; xudingke
> <xudingke at huawei.com>; stable at 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 <wangyunjian at 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: stable at dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunjian at 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.

@@ -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