[dpdk-dev] [PATCH 1/1] 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: Monday, July 20, 2020 7:46 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 1/1] eal/linux: do not create user mem map
> repeatedly when it exists
> 
> On 20-Jul-20 3:00 AM, wangyunjian wrote:
> >> -----Original Message-----
> >> From: Burakov, Anatoly [mailto:anatoly.burakov at intel.com]
> >> Sent: Friday, July 17, 2020 10:24 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 1/1] eal/linux: do not create user mem
> >> map repeatedly when it exists
> >>
> >> On 17-Jul-20 3:19 PM, Burakov, Anatoly wrote:
> >>> On 16-Jul-20 2:38 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.
> >>>>
> >>>> Fixes: 0cbce3a167f1 ("vfio: skip DMA map failure if already
> >>>> mapped")
> >>>> Cc: stable at dpdk.org
> >>>>
> >>>> Signed-off-by: Yunjian Wang <wangyunjian at huawei.com>
> >>>> ---
> >>>>    lib/librte_eal/linux/eal_vfio.c | 7 +++++++
> >>>>    1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/lib/librte_eal/linux/eal_vfio.c
> >>>> b/lib/librte_eal/linux/eal_vfio.c index abb12a354..d8a8c39ab 100644
> >>>> --- a/lib/librte_eal/linux/eal_vfio.c
> >>>> +++ b/lib/librte_eal/linux/eal_vfio.c
> >>>> @@ -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;
> >>>> +
> >>>
> >>> I'm not sure i understand this patch. If we get errno, the call has
> >>> failed, which means we're doing "goto out" from a few lines above.
> >>> Am i missing something here?
> >>>
> >>>>        /* create new user mem map entry */
> >>>>        new_map =
> >> &user_mem_maps->maps[user_mem_maps->n_maps++];
> >>>>        new_map->addr = vaddr;
> >>>>
> >>>
> >>>
> >>
> >> Oh, i see, the actual functions will set errno and return 0.
> >>
> >> I don't think it's an actual issue as compacting will presumably
> >> remove the extra user mem map anyway. What exactly is being fixed
> >> here? Does compacting user mem maps not remove the extra entry?
> >
> > I read the codes about compacting user mem maps. Currently, the
> > function only merges adjacent user mem maps and does not remove the
> same entry.
> >
> > How about removing the same entry in the fuction?
> 
> I would've expected "the same" to be within the definition of "adjacent". Can
> you confirm that this actually doesn't happen? If so, then yes, probably
> compacting should do that, instead of relying on an artifact of implementation.

OK, I will do that, will send the v2 later.

Thanks
Yunjian

> 
> >
> > Thanks
> > Yunjian
> >
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly