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

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

On 26-Aug-21 11:09 AM, Bruce Richardson wrote:
> On Thu, Aug 26, 2021 at 10:46:07AM +0100, Burakov, Anatoly wrote:
>> On 26-Aug-21 10:29 AM, Ferruh Yigit wrote:
>>> On 8/25/2021 12:47 PM, Burakov, Anatoly wrote:
>>>> On 25-Aug-21 12:27 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..272ffa993e 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` and
>>>>> `rte_vfio_container_dma_unmap`
>>>>> +  will be amended to include page size. This change is targeted for DPDK 21.11.
>>>>>
>>>>
>>>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>>
>>>
>>> Techboard decision was to add a new API, instead of updating existing ones, to
>>> not break the apps using this API.
>>>
>>> @Xuan, @Anatoly, can you please confirm if this will solve your problem?
>>>
>>
>> I don't think adding a new API is a particularly good solution. The "new"
>> API will be almost exactly as the old one, but adding one parameter. I don't
>> expect code duplication to be an issue, but having two API's that do the
>> same thing seems like it's rife for potential confusion.
>>
> Well, if one API is marked as deprecated, then there will be no confusion
> for users, since using the wrong one will give a warning pointing to the
> right one.
> 
>> If we add a new API, we can then either remove the old API entirely in
>> 22.11 (effectively renaming it), or we remove the new API in 22.11 and
>> rename it back to the old function name. I don't think neither of these
>> is a good solution, as we risk introducing more users for the API that
>> will later change.
> The new API will not be renamed to the old one, since that would break apps
> using it without proper deprecation process. Removing the old one alone
> would be the approach to be used, but it would be correctly following the
> deprecation process and giving users at least 1 year, if no 2, of notice
> about the change.
> 
>>
>> I think the pain of updating current software for 21.11 (while keeping
>> compatibility with 20.11 ABI!) is going to happen regardless, and whether we
>> decide to add a "temporary" new API or permanently rename the old one. It's
>> (in my opinion) easier to just bite the bullet and update the function in
>> 21.11.
> I fail to see the issue with adding a new function. Whether we add a new
> function or add a parameter to the existing one, code will have to change
> either way. The advantage of the former scheme, adding the new function, is
> that it shows that we are serious about our ABI/API compatibility process,
> and are not lax about passing exceptions when other options are available.
> 
>>
>> However, if the tech board feels like adding a new API is a good solution,
>> then okay, but we need to flesh out roadmap a bit better. Do we rename the
>> old API, or do we add a temporary new API?
> 
> New API added, old API deprecated. In future old API goes away leaving new
> API as the only option.
> 
> /Bruce
> 

Okay, so it's settled then. I revoke my ack for this patch, and we need 
a new deprecation notice.

-- 
Thanks,
Anatoly