All of lore.kernel.org
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
@ 2021-08-31 13:10 Xuan Ding
  2021-08-31 13:46 ` Burakov, Anatoly
  2021-08-31 16:01 ` Ferruh Yigit
  0 siblings, 2 replies; 15+ messages in thread
From: Xuan Ding @ 2021-08-31 13:10 UTC (permalink / raw)
  To: dev, anatoly.burakov
  Cc: maxime.coquelin, chenbo.xia, ferruh.yigit, jiayu.hu,
	bruce.richardson, Xuan Ding

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.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-08-31 13:10 [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping Xuan Ding
@ 2021-08-31 13:46 ` Burakov, Anatoly
  2021-08-31 16:01 ` Ferruh Yigit
  1 sibling, 0 replies; 15+ messages in thread
From: Burakov, Anatoly @ 2021-08-31 13:46 UTC (permalink / raw)
  To: Xuan Ding, dev
  Cc: maxime.coquelin, chenbo.xia, ferruh.yigit, jiayu.hu, bruce.richardson

On 31-Aug-21 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.
> 

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-08-31 13:10 [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping Xuan Ding
  2021-08-31 13:46 ` Burakov, Anatoly
@ 2021-08-31 16:01 ` Ferruh Yigit
  2021-09-01  1:41   ` Ding, Xuan
  1 sibling, 1 reply; 15+ messages in thread
From: Ferruh Yigit @ 2021-08-31 16:01 UTC (permalink / raw)
  To: Xuan Ding, dev, anatoly.burakov
  Cc: maxime.coquelin, chenbo.xia, jiayu.hu, bruce.richardson

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-08-31 16:01 ` Ferruh Yigit
@ 2021-09-01  1:41   ` Ding, Xuan
  2021-09-01  9:56     ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: Ding, Xuan @ 2021-09-01  1:41 UTC (permalink / raw)
  To: Yigit, Ferruh, dev, Burakov, Anatoly
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce

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.

Thanks for you classification.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-01  1:41   ` Ding, Xuan
@ 2021-09-01  9:56     ` Ferruh Yigit
  2021-09-01 11:01       ` Burakov, Anatoly
  0 siblings, 1 reply; 15+ messages in thread
From: Ferruh Yigit @ 2021-09-01  9:56 UTC (permalink / raw)
  To: Ding, Xuan, dev, Burakov, Anatoly
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce

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.

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?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-01  9:56     ` Ferruh Yigit
@ 2021-09-01 11:01       ` Burakov, Anatoly
  2021-09-01 11:42         ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: Burakov, Anatoly @ 2021-09-01 11:01 UTC (permalink / raw)
  To: Ferruh Yigit, Ding, Xuan, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce

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?

> 
> 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.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-01 11:01       ` Burakov, Anatoly
@ 2021-09-01 11:42         ` Ferruh Yigit
  2021-09-01 13:25           ` Burakov, Anatoly
  0 siblings, 1 reply; 15+ messages in thread
From: Ferruh Yigit @ 2021-09-01 11:42 UTC (permalink / raw)
  To: Burakov, Anatoly, Ding, Xuan, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon, Ray Kinsella

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?

Thanks,
ferruh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-01 11:42         ` Ferruh Yigit
@ 2021-09-01 13:25           ` Burakov, Anatoly
  2021-09-02  9:50             ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: Burakov, Anatoly @ 2021-09-01 13:25 UTC (permalink / raw)
  To: Ferruh Yigit, Ding, Xuan, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon, Ray Kinsella

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-01 13:25           ` Burakov, Anatoly
@ 2021-09-02  9:50             ` Ferruh Yigit
  2021-09-02 16:13               ` Kinsella, Ray
  0 siblings, 1 reply; 15+ messages in thread
From: Ferruh Yigit @ 2021-09-02  9:50 UTC (permalink / raw)
  To: Burakov, Anatoly, Ding, Xuan, dev, Ray Kinsella
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-02  9:50             ` Ferruh Yigit
@ 2021-09-02 16:13               ` Kinsella, Ray
  2021-09-06  8:51                 ` Ding, Xuan
  0 siblings, 1 reply; 15+ messages in thread
From: Kinsella, Ray @ 2021-09-02 16:13 UTC (permalink / raw)
  To: Ferruh Yigit, Burakov, Anatoly, Ding, Xuan, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon



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
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-02 16:13               ` Kinsella, Ray
@ 2021-09-06  8:51                 ` Ding, Xuan
  2021-09-06 13:43                   ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: Ding, Xuan @ 2021-09-06  8:51 UTC (permalink / raw)
  To: Kinsella, Ray, Yigit, Ferruh, Burakov, Anatoly, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon

Hi,

> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Friday, September 3, 2021 12:13 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
> dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu,
> Jiayu <jiayu.hu@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [PATCH] doc: announce change in vfio dma mapping
> 
> 
> 
> 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 for your explanation.
@Yigit, Ferruh Does it mean that we can do API change in 21.11? If so, we will
follow the process and target API change in this release. :)

Regards,
Xuan

> >
> > Thanks,
> > ferruh
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-06  8:51                 ` Ding, Xuan
@ 2021-09-06 13:43                   ` Ferruh Yigit
  2021-09-07 15:21                     ` Burakov, Anatoly
  0 siblings, 1 reply; 15+ messages in thread
From: Ferruh Yigit @ 2021-09-06 13:43 UTC (permalink / raw)
  To: Ding, Xuan, Kinsella, Ray, Burakov, Anatoly, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon

On 9/6/2021 9:51 AM, Ding, Xuan wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Kinsella, Ray <mdr@ashroe.eu>
>> Sent: Friday, September 3, 2021 12:13 AM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Burakov, Anatoly
>> <anatoly.burakov@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
>> dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu,
>> Jiayu <jiayu.hu@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
>> Thomas Monjalon <thomas@monjalon.net>
>> Subject: Re: [PATCH] doc: announce change in vfio dma mapping
>>
>>
>>
>> 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 for your explanation.
> @Yigit, Ferruh Does it mean that we can do API change in 21.11? If so, we will
> follow the process and target API change in this release. :)
> 

With symbol versioning, yes you can make the change in this release.

You can send another deprecation notice to remove the old symbol for 22.11, that
can be sent anytime until 22.08 released.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-06 13:43                   ` Ferruh Yigit
@ 2021-09-07 15:21                     ` Burakov, Anatoly
  2021-09-07 16:08                       ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: Burakov, Anatoly @ 2021-09-07 15:21 UTC (permalink / raw)
  To: Ferruh Yigit, Ding, Xuan, Kinsella, Ray, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon

On 06-Sep-21 2:43 PM, Ferruh Yigit wrote:
> On 9/6/2021 9:51 AM, Ding, Xuan wrote:
>> Hi,
>>
>>> -----Original Message-----
>>> From: Kinsella, Ray <mdr@ashroe.eu>
>>> Sent: Friday, September 3, 2021 12:13 AM
>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Burakov, Anatoly
>>> <anatoly.burakov@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
>>> dev@dpdk.org
>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu,
>>> Jiayu <jiayu.hu@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
>>> Thomas Monjalon <thomas@monjalon.net>
>>> Subject: Re: [PATCH] doc: announce change in vfio dma mapping
>>>
>>>
>>>
>>> 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 for your explanation.
>> @Yigit, Ferruh Does it mean that we can do API change in 21.11? If so, we will
>> follow the process and target API change in this release. :)
>>
> 
> With symbol versioning, yes you can make the change in this release.
> 
> You can send another deprecation notice to remove the old symbol for 22.11, that
> can be sent anytime until 22.08 released.
> 

Hi Ferruh and others,

We have decided to switch gears somewhat :)

The original intent was to ensure that two adjacent segments can be 
freely mapped and unmapped. The easiest solution was page-by-page 
mapping, so that's where the API change idea came from.

However, due to tech board decision to not grant the exception at the 
time, we have figured out a way to avoid the API change. The API change 
is still a good idea because mapping things page-by-page is a valid use 
case that is currently not covered by the API, so the next plan was to 
do the API change in a later release as a separate issue, not related to 
original Xuan's intent.

However, we've been discussing implementation details with Xuan, and we 
arrived at a realization that what Xuan wants to implement not only does 
not *require* page size, it *cannot* be implemented with page size API 
because there's no way to know page size for that memory at the time of 
the API call. So, turns out we need both paged and page-less versions :)

This means that there are no deprecation notices now, because we will be 
adding a new API after all, but we will *not* deprecate or remove the 
old API - that one will still stay valid.

Apologies for constantly shifting ground, but in the end i think we 
arrived at the best possible solution for this problem!

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-07 15:21                     ` Burakov, Anatoly
@ 2021-09-07 16:08                       ` Ferruh Yigit
  2021-09-08  8:59                         ` Kinsella, Ray
  0 siblings, 1 reply; 15+ messages in thread
From: Ferruh Yigit @ 2021-09-07 16:08 UTC (permalink / raw)
  To: Burakov, Anatoly, Ding, Xuan, Kinsella, Ray, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon

On 9/7/2021 4:21 PM, Burakov, Anatoly wrote:
> On 06-Sep-21 2:43 PM, Ferruh Yigit wrote:
>> On 9/6/2021 9:51 AM, Ding, Xuan wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Kinsella, Ray <mdr@ashroe.eu>
>>>> Sent: Friday, September 3, 2021 12:13 AM
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Burakov, Anatoly
>>>> <anatoly.burakov@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
>>>> dev@dpdk.org
>>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu,
>>>> Jiayu <jiayu.hu@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
>>>> Thomas Monjalon <thomas@monjalon.net>
>>>> Subject: Re: [PATCH] doc: announce change in vfio dma mapping
>>>>
>>>>
>>>>
>>>> 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 for your explanation.
>>> @Yigit, Ferruh Does it mean that we can do API change in 21.11? If so, we will
>>> follow the process and target API change in this release. :)
>>>
>>
>> With symbol versioning, yes you can make the change in this release.
>>
>> You can send another deprecation notice to remove the old symbol for 22.11, that
>> can be sent anytime until 22.08 released.
>>
> 
> Hi Ferruh and others,
> 
> We have decided to switch gears somewhat :)
> 
> The original intent was to ensure that two adjacent segments can be freely
> mapped and unmapped. The easiest solution was page-by-page mapping, so that's
> where the API change idea came from.
> 
> However, due to tech board decision to not grant the exception at the time, we
> have figured out a way to avoid the API change. The API change is still a good
> idea because mapping things page-by-page is a valid use case that is currently
> not covered by the API, so the next plan was to do the API change in a later
> release as a separate issue, not related to original Xuan's intent.
> 
> However, we've been discussing implementation details with Xuan, and we arrived
> at a realization that what Xuan wants to implement not only does not *require*
> page size, it *cannot* be implemented with page size API because there's no way
> to know page size for that memory at the time of the API call. So, turns out we
> need both paged and page-less versions :)
> 
> This means that there are no deprecation notices now, because we will be adding
> a new API after all, but we will *not* deprecate or remove the old API - that
> one will still stay valid.
> 
> Apologies for constantly shifting ground, but in the end i think we arrived at
> the best possible solution for this problem!
> 

So there won't be symbol versioning but only new API, which means no deprecation
notice is required, please update this patch's status accordingly.

Thanks for keep working on the issue to find a better solution.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping
  2021-09-07 16:08                       ` Ferruh Yigit
@ 2021-09-08  8:59                         ` Kinsella, Ray
  0 siblings, 0 replies; 15+ messages in thread
From: Kinsella, Ray @ 2021-09-08  8:59 UTC (permalink / raw)
  To: Ferruh Yigit, Burakov, Anatoly, Ding, Xuan, dev
  Cc: maxime.coquelin, Xia, Chenbo, Hu, Jiayu, Richardson, Bruce,
	Thomas Monjalon

> 
> So there won't be symbol versioning but only new API, which means no deprecation
> notice is required, please update this patch's status accordingly.
> 
> Thanks for keep working on the issue to find a better solution.
> 

+1, good work

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-09-08  9:00 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 13:10 [dpdk-dev] [PATCH] doc: announce change in vfio dma mapping Xuan Ding
2021-08-31 13:46 ` Burakov, Anatoly
2021-08-31 16:01 ` Ferruh Yigit
2021-09-01  1:41   ` Ding, Xuan
2021-09-01  9:56     ` Ferruh Yigit
2021-09-01 11:01       ` Burakov, Anatoly
2021-09-01 11:42         ` Ferruh Yigit
2021-09-01 13:25           ` Burakov, Anatoly
2021-09-02  9:50             ` Ferruh Yigit
2021-09-02 16:13               ` Kinsella, Ray
2021-09-06  8:51                 ` Ding, Xuan
2021-09-06 13:43                   ` Ferruh Yigit
2021-09-07 15:21                     ` Burakov, Anatoly
2021-09-07 16:08                       ` Ferruh Yigit
2021-09-08  8:59                         ` Kinsella, Ray

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.