Hi, > -----Original Message----- > From: Kinsella, Ray <m...@ashroe.eu> > Sent: Friday, September 3, 2021 12:13 AM > To: Yigit, Ferruh <ferruh.yi...@intel.com>; Burakov, Anatoly > <anatoly.bura...@intel.com>; Ding, Xuan <xuan.d...@intel.com>; > dev@dpdk.org > Cc: maxime.coque...@redhat.com; Xia, Chenbo <chenbo....@intel.com>; Hu, > Jiayu <jiayu...@intel.com>; Richardson, Bruce <bruce.richard...@intel.com>; > Thomas Monjalon <tho...@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.yi...@intel.com> > >>>>>>> Sent: Wednesday, September 1, 2021 12:01 AM > >>>>>>> To: Ding, Xuan <xuan.d...@intel.com>; dev@dpdk.org; Burakov, > Anatoly > >>>>>>> <anatoly.bura...@intel.com> > >>>>>>> Cc: maxime.coque...@redhat.com; Xia, Chenbo > <chenbo....@intel.com>; Hu, > >>>>>>> Jiayu <jiayu...@intel.com>; Richardson, Bruce > <bruce.richard...@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.d...@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 > >
