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

["Ding, Xuan" <xuan.ding@intel.com>]

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