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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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

Thanks,
ferruh