RE: [PATCH v2] vfio: do not coalesce DMA mappings

["Gupta, Nipun" <Nipun.Gupta@amd.com>]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Wednesday, April 5, 2023 7:46 PM
> To: Gupta, Nipun <Nipun.Gupta@amd.com>; David Marchand
> <david.marchand@redhat.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; Yigit, Ferruh
> <Ferruh.Yigit@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>
> Subject: Re: [PATCH v2] vfio: do not coalesce DMA mappings
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> On 4/4/2023 5:32 PM, Nipun Gupta wrote:
> >
> >
> > On 4/4/2023 8:43 PM, Burakov, Anatoly wrote:
> >> Caution: This message originated from an External Source. Use proper
> >> caution when opening attachments, clicking links, or responding.
> >>
> >>
> >> On 2/7/2023 8:56 AM, Gupta, Nipun wrote:
> >>> [AMD Official Use Only - General]
> >>>
> >>> Hi David,
> >>>
> >>> I agree that change is not straightforward to review, but it should
> >>> not cause any functional issue as we are still creating all the
> >>> memory mappings, but one by one for each segment.
> >>> For hot plug case this causes issue as mentioned, that VFIO does not
> >>> allow unmap of the individual segments in case mapping was created of
> >>> a single coalesced segment.
> >>>
> >>> But yes, I am not sure why this code was added, which Anatoly may
> >>> have more understanding on.
> >>
> >> The motivation behind this code was that Linux allows limited amount of
> >> page mappings, so we were trying to save on those. However, since then
> >> there have been a few changes related to partial unmaps that may make it
> >> so that this code is not only no longer necessary, but is in fact
> >> actively harmful. I agree that this at least warrants a second look.
> >>
> >>>
> >>> Anatoly,
> >>>
> >>> Can you please provide your feedback on this change?
> >>
> >> The patch probably shouldn't include the mailmap changes :)
> >
> > Sure, will send a separate patch for it.
> >
> >>
> >> Could you please provide some steps to reproduce the hotplug issue
> >> you're having? It would be great to have a test case for this patchset
> >> to put it in context.
> >
> > I am working on CDX bus
> > (http://patchwork.dpdk.org/project/dpdk/patch/20230124140746.594066-2-
> nipun.gupta@amd.com/) and trying out some cases for plug/unplug.
> >
> > The test is as follows:
> >    # Run testpmd application
> >    ./dpdk-testpmd -c 0x3 -- -i --nb-cores=1
> >
> >    # Bind to VFIO
> >    echo "vfio-cdx" >  /sys/bus/cdx/devices/cdx-00\:00/driver_override
> >    echo "cdx-00:00" > /sys/bus/cdx/drivers_probe
> >
> >    # Plug a device
> >    testpmd> port attach cdx:cdx-00:00
> >
> >    #quit testpmd
> >    testpmd> quit
> >
> > This gave error at testpmd exit that memory cannot be freed. On
> > debugging I updated this code and seems it should be seen with any of
> > the device.
> >
> > I see similar test case (without quit) mentioned
> > https://doc.dpdk.org/dts/test_plans/hotplug_test_plan.html, but the
> > difference is that it is with igb_uio and issue is being observed with
> > VFIO.
> >
> > Please note the device/bus mentioned in the commands is not yet
> > upstreamed in DPDK, but patches would be sent out soon.
> >
> > Thanks,
> > Nipun
> >
> 
> Thanks, I can reproduce this issue with regular devices too (run testpmd
> with no devices, bind a NIC to VFIO, attach it, then quit). You're
> correct in that since the initial mapping was done with mapping large
> contiguous zones (such as when mempools are created before attach), any
> subsequent freeing of memory will cause these errors to happen.

Thanks for trying on other devices too, and good to know that this is also
reproduced with them :)

Regards,
Nipun

> 
> I don't think this can be fixed by anything other than not doing the
> contiguous mapping thing, so provisionally, I think this patch should be
> accepted. I'll play around with it some more and get back to you :)
> 
> >>
> >> --
> >> Thanks,
> >> Anatoly
> >>
> 
> --
> Thanks,
> Anatoly