Re: [dpdk-dev] [EXT] Re: [PATCH v13 2/2] kni: support IOVA mode

[David Marchand <david.marchand@redhat.com>]

On Fri, Nov 15, 2019 at 2:36 PM Vamsi Krishna Attunuru
<vattunuru@marvell.com> wrote:
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Friday, November 15, 2019 6:29 PM
> > To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>
> > Cc: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Jerin
> > Jacob Kollanukkaran <jerinj@marvell.com>; Kiran Kumar Kokkilagadda
> > <kirankumark@marvell.com>; Olivier Matz <olivier.matz@6wind.com>;
> > Burakov, Anatoly <anatoly.burakov@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; Stephen Hemminger
> > <stephen@networkplumber.org>
> > Subject: [EXT] Re: [dpdk-dev] [PATCH v13 2/2] kni: support IOVA mode
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > I can't see an interest in splitting this patch from the kmod update.
> > Ferruh, what do you think?
> >
> >
> > On Fri, Nov 15, 2019 at 12:19 PM <vattunuru@marvell.com> wrote:
> > >
> > > From: Vamsi Attunuru <vattunuru@marvell.com>
> > >
> > > Current KNI implementation only operates in IOVA_PA mode patch adds
> > > required functionality to enable KNI in IOVA_VA mode.
> > >
> > > KNI loopback mode tests will have performance impact in this mode due
> > > to IOVA to KVA address translations.
> > > However, In KNI real world use cases, the performace
> >
> > performance
> >
> > > impact will be based on Linux kernel stack and scheduler latencies.
> > > Performance varies based on the KNI use case.
> > > If bus iommu scheme is IOVA_DC and KNI module is loaded, DPDK chooses
> > > IOVA as PA as existing behaviour.
> > >
> > > During KNI creation, app's iova_mode details are passed to the KNI
> > > kernel module, accordingly kernel module translates PA/IOVA addresses
> > > to KVA and vice-versa.
> > >
> > > Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
> > > Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> > > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > ---
> > >  doc/guides/prog_guide/kernel_nic_interface.rst | 15 +++++++++++++++
> > >  doc/guides/rel_notes/release_19_11.rst         | 15 ++++++++++++++-
> > >  lib/librte_eal/linux/eal/eal.c                 | 23 ++++++++++++++++-------
> > >  lib/librte_kni/rte_kni.c                       |  6 ++++++
> > >  4 files changed, 51 insertions(+), 8 deletions(-)
> > >
> >
> > [snip]
> >
> > > diff --git a/lib/librte_eal/linux/eal/eal.c
> > > b/lib/librte_eal/linux/eal/eal.c index 9e2d50c..53ca84b 100644
> > > --- a/lib/librte_eal/linux/eal/eal.c
> > > +++ b/lib/librte_eal/linux/eal/eal.c
> > > @@ -1086,14 +1086,23 @@ rte_eal_init(int argc, char **argv)
> > >                         }
> > >                 }
> > >  #ifdef RTE_LIBRTE_KNI
> > > -               /* Workaround for KNI which requires physical address to work */
> > > -               if (iova_mode == RTE_IOVA_VA &&
> > > -                               rte_eal_check_module("rte_kni") == 1) {
> > > -                       if (phys_addrs) {
> > > +               if (rte_eal_check_module("rte_kni") == 1) { #if
> > > +KERNEL_VERSION(4, 6, 0) > LINUX_VERSION_CODE
> > > +                       if (iova_mode == RTE_IOVA_VA) {
> > >                                 iova_mode = RTE_IOVA_PA;
> > > -                               RTE_LOG(WARNING, EAL, "Forcing IOVA as 'PA' because KNI
> > module is loaded\n");
> > > -                       } else {
> > > -                               RTE_LOG(DEBUG, EAL, "KNI can not work since physical
> > addresses are unavailable\n");
> > > +                               RTE_LOG(WARNING, EAL, "Forcing IOVA as 'PA' because "
> > > +                                               "Kernel version supports only 'PA' mode for KNI
> > module\n");
> > > +                       }
> > > +#endif
> > > +                       if (rte_bus_get_iommu_class() == RTE_IOVA_DC)
> > > +                               iova_mode = RTE_IOVA_PA;
> >
> > If physical addresses are unavailable, this code forces PA anyway.
> >
> >
> > > +
> > > +                       if (iova_mode == RTE_IOVA_PA) {
> > > +                               if (phys_addrs && is_iommu_enabled())
> > > +                                       RTE_LOG(WARNING, EAL, "Forced
> > > + IOVA as 'PA' because KNI module is loaded\n");
> > > +
> > > +                               if (!phys_addrs)
> > > +                                       RTE_LOG(DEBUG, EAL, "KNI can
> > > + not work since physical addresses are unavailable\n");
> > >                         }
> >
> > Checking physical addresses availability after, and having a log is not enough.
> >
> >
> > So far, KNI could not work with IOVA as VA.
> > Your patchset adds support for IOVA as VA if kernel is >= 4.6.
> > Repeating my proposal (as far as eal.c is concerned) of just changing:
>
> To keep the existing behavior intact when bus iommu returns IOVA_DC, had to handle those case also here.

No.
If you want to change something, do it in a separate patch with full
explanation.

-- 
David Marchand