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? Later we can have a deprecation notice to remove old symbol on 22.11. Thanks, ferruh
