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

["Kinsella, Ray" <mdr@ashroe.eu>]

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,
> ferruh
>