diff mbox series

doc: announce change in dma mapping/unmapping

Message ID 20210825112700.83810-1-xuan.ding@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series doc: announce change in dma mapping/unmapping |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-spell-check-testing success Testing PASS
ci/github-robot: build fail github build: failed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-aarch64-unit-testing fail Testing issues

Commit Message

Ding, Xuan Aug. 25, 2021, 11:27 a.m. UTC 
  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(+)

Comments

Anatoly Burakov Aug. 25, 2021, 11:47 a.m. UTC | #1 
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>
Ferruh Yigit Aug. 26, 2021, 9:29 a.m. UTC | #2 
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?
Anatoly Burakov Aug. 26, 2021, 9:46 a.m. UTC | #3 
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.

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.

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.

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?
Bruce Richardson Aug. 26, 2021, 10:09 a.m. UTC | #4 
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
Anatoly Burakov Aug. 26, 2021, 10:14 a.m. UTC | #5 
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.
Ding, Xuan Aug. 31, 2021, 1:42 p.m. UTC | #6 
Hi,

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Thursday, August 26, 2021 6:15 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Ding, Xuan
> <xuan.ding@intel.com>; dev@dpdk.org; maxime.coquelin@redhat.com; Xia,
> Chenbo <chenbo.xia@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>;
> techboard@dpdk.org; David Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH] doc: announce change in dma mapping/unmapping
> 
> 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.

A new depreciation notice was sent [1], targeting for API change in DPDK-22.02.
For the unmapping issue mentioned before, we developed a compromised solution
to optimize the partial unmap logic in DPDK-21.11, and it is compatible with current
API.

[1] https://mails.dpdk.org/archives/dev/2021-August/217802.html

Thanks for your suggestion and support!

Regards,
Xuan
> 
> --
> Thanks,
> Anatoly
diff mbox series

Patch

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.