Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping

["Ding, Xuan" <xuan.ding@intel.com>]

Hi,

> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Friday, September 3, 2021 12:13 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
> dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu,
> Jiayu <jiayu.hu@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [PATCH] doc: announce change in vfio dma mapping
> 
> 
> 
> On 02/09/2021 10:50, Ferruh Yigit wrote:
> > On 9/1/2021 2:25 PM, Burakov, Anatoly wrote:
> >> On 01-Sep-21 12:42 PM, Ferruh Yigit wrote:
> >>> On 9/1/2021 12:01 PM, Burakov, Anatoly wrote:
> >>>> On 01-Sep-21 10:56 AM, Ferruh Yigit wrote:
> >>>>> On 9/1/2021 2:41 AM, Ding, Xuan wrote:
> >>>>>> Hi Ferruh,
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>>> Sent: Wednesday, September 1, 2021 12:01 AM
> >>>>>>> To: Ding, Xuan <xuan.ding@intel.com>; dev@dpdk.org; Burakov,
> Anatoly
> >>>>>>> <anatoly.burakov@intel.com>
> >>>>>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>; Hu,
> >>>>>>> Jiayu <jiayu.hu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> >>>>>>> Subject: Re: [PATCH] doc: announce change in vfio dma mapping
> >>>>>>>
> >>>>>>> On 8/31/2021 2:10 PM, Xuan Ding wrote:
> >>>>>>>> Currently, the VFIO subsystem will compact adjacent DMA regions for
> the
> >>>>>>>> purposes of saving space in the internal list of mappings. This has a
> >>>>>>>> side effect of compacting two separate mappings that just happen to
> be
> >>>>>>>> adjacent in memory. Since VFIO implementation on IA platforms also
> does
> >>>>>>>> not allow partial unmapping of memory mapped for DMA, the current
> >>>>>>> DPDK
> >>>>>>>> VFIO implementation will prevent unmapping of accidentally adjacent
> >>>>>>>> maps even though it could have been unmapped [1].
> >>>>>>>>
> >>>>>>>> The proper fix for this issue is to change the VFIO DMA mapping API to
> >>>>>>>> also include page size, and always map memory page-by-page.
> >>>>>>>>
> >>>>>>>> [1] https://mails.dpdk.org/archives/dev/2021-July/213493.html
> >>>>>>>>
> >>>>>>>> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> >>>>>>>> ---
> >>>>>>>>    doc/guides/rel_notes/deprecation.rst | 3 +++
> >>>>>>>>    1 file changed, 3 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
> >>>>>>> b/doc/guides/rel_notes/deprecation.rst
> >>>>>>>> index 76a4abfd6b..1234420caf 100644
> >>>>>>>> --- a/doc/guides/rel_notes/deprecation.rst
> >>>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>>>>>>> @@ -287,3 +287,6 @@ Deprecation Notices
> >>>>>>>>      reserved bytes to 2 (from 3), and use 1 byte to indicate warnings
> and
> >>>>>>> other
> >>>>>>>>      information from the crypto/security operation. This field will be
> >>>>>>>> used to
> >>>>>>>>      communicate events such as soft expiry with IPsec in lookaside
> mode.
> >>>>>>>> +
> >>>>>>>> +* vfio: the functions `rte_vfio_container_dma_map` will be amended
> to
> >>>>>>>> +  include page size. This change is targeted for DPDK 22.02.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Is this means adding a new parameter to API?
> >>>>>>> If so this is an ABI/API break and we can't do this change in the 22.02.
> >>>>>>
> >>>>>> Our original plan is add a new parameter in order not to use a new
> function
> >>>>>> name, so you mean, any changes to the API can only be done in the LTS
> version?
> >>>>>> If so, we can only add a new API and retire the old one in 22.11.
> >>>>>>
> >>>>>
> >>>>> We can add a new API anytime. Adding new parameter to an existing API
> can be
> >>>>> done on the ABI break release.
> >>>>
> >>>> So, 22.11 then?
> >>>>
> >>>
> >>> Yes.
> >>>
> >>>>>
> >>>>> You can add the new API in this release, and start using it.
> >>>>> And mark the old API as deprecated in this release. This lets existing
> binaries
> >>>>> to keep using it, but app needs to switch to new API for compilation.
> >>>>> Old API can be removed on 22.11, and you will need a deprecation notice
> before
> >>>>> 22.11 for it.
> >>>>>
> >>>>> Is above plan works for you?
> >>>>>
> >>>>
> >>>> We have slightly rethought our approach, and the functionality that Xuan
> >>>> requires does not rely on this API. They can, for all intents and purposes, be
> >>>> considered unrelated issues.
> >>>>
> >>>> I still think it's a good idea to update the API that way, so I would like to do
> >>>> that, and if we have to wait until 22.11 to fix it, I'm OK with that. Since
> >>>> there no longer is any urgency here, it's acceptable to wait for the next LTS
> to
> >>>> break it.
> >>>>
> >>>
> >>> Got it.
> >>>
> >>> As far as I understand, main motivation in techboard decision was to
> prevent the
> >>> ABI break as much as possible (main reason of decision wasn't deprecation
> notice
> >>> being late). But if the correct thing to do is to rename the API (and break the
> >>> ABI), I don't see the benefit to wait one more year, it is just delaying the
> >>> impact and adding overhead to us.
> >>> I am for being pragmatic and doing the change in this release if API rename
> is
> >>> better option, perhaps we can visit the issue again in techboard.
> >>>
> >>> Can you please describe why renaming API is better option, comparing to
> adding
> >>> new API with new parameter?
> >>
> >> I take it you meant "why renaming API *isn't* a better option".
> >>
> >> The problem we're solving is that the API in question does not know about
> page
> >> sizes and thus can't map segments page-by-page. I mean I /guess/ we could
> have
> >> two API's (one paged, one not paged), but then we get into all kinds of hairy
> >> things about the API leaking the details of underlying platform.
> >>
> >> Bottom line: i like current API function name. It's concise, it's descriptive.
> >> It's only missing a parameter, which i would like to add. A rename has been
> >> suggested (deprecate old API, add new API with a different name, and with
> added
> >> parameter), but honestly, I don't see why we have to do that because this is
> >> predicated upon the assumption that we *can't* break ABI at all, under any
> >> circumstances.
> >>
> >> Can you please explain to me what is wrong with keeping a versioned symbol?
> >> Like, keep the old function around to keep ABI compatibility, but break the
> API
> >> compatibility for those who target 22.02 or later? That's what symbol
> versioning
> >> is *for*, is it not?
> >>
> >
> > Nothing wrong with symbol versioning, indeed that is preferred way if it works
> > for you, I didn't get that symbol versioning is planned.
> >
> > @Ray,
> > Since symbol versioning is planned, ABI won't break, but API will change, can
> > this change be done in this release without deprecation notice?
> 
> Yes - I would think so.
> Since we are going to the effort of using symbol versioning nothing is being
> depreciated as such (yet).
> 
> > Later we can have a deprecation notice to remove old symbol on 22.11.

Thanks for your explanation.
@Yigit, Ferruh Does it mean that we can do API change in 21.11? If so, we will
follow the process and target API change in this release. :)

Regards,
Xuan

> >
> > Thanks,
> > ferruh
> >