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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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?

-- 
Thanks,
Anatoly