Re: [dpdk-dev] [PATCH 3/7] mem: add function for setting DMA mask

[Alejandro Lucero <alejandro.lucero@netronome.com>]

On Thu, Nov 1, 2018 at 10:57 AM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 01-Nov-18 10:48 AM, Alejandro Lucero wrote:
> >
> >
> > On Thu, Nov 1, 2018 at 10:11 AM Burakov, Anatoly
> > <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> wrote:
> >
> >     On 31-Oct-18 5:29 PM, Alejandro Lucero wrote:
> >      > This patch adds the possibility of setting a dma mask to be used
> >      > once the memory initialization is done.
> >      >
> >      > This is currently needed when IOVA mode is set by PCI related
> >      > code and an x86 IOMMU hardware unit is present. Current code calls
> >      > rte_mem_check_dma_mask but it is wrong to do so at that point
> >      > because the memory has not been initialized yet.
> >      >
> >      > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
> >     <mailto:alejandro.lucero@netronome.com>>
> >      > ---
> >      >   lib/librte_eal/common/eal_common_memory.c  | 10 ++++++++++
> >      >   lib/librte_eal/common/include/rte_memory.h | 10 ++++++++++
> >      >   lib/librte_eal/rte_eal_version.map         |  1 +
> >      >   3 files changed, 21 insertions(+)
> >      >
> >      > diff --git a/lib/librte_eal/common/eal_common_memory.c
> >     b/lib/librte_eal/common/eal_common_memory.c
> >      > index e0f08f39a..24b72fcb0 100644
> >      > --- a/lib/librte_eal/common/eal_common_memory.c
> >      > +++ b/lib/librte_eal/common/eal_common_memory.c
> >      > @@ -480,6 +480,16 @@ rte_mem_check_dma_mask(uint8_t maskbits)
> >      >       return 0;
> >      >   }
> >      >
> >      > +/* set dma mask to use when memory initialization is done */
> >      > +void __rte_experimental
> >      > +rte_mem_set_dma_mask(uint8_t maskbits)
> >      > +{
> >      > +     struct rte_mem_config *mcfg =
> >     rte_eal_get_configuration()->mem_config;
> >      > +
> >      > +     mcfg->dma_maskbits = mcfg->dma_maskbits == 0 ? maskbits :
> >      > +                          RTE_MIN(mcfg->dma_maskbits, maskbits);
> >      > +}
> >      > +
> >      >   /* return the number of memory channels */
> >      >   unsigned rte_memory_get_nchannel(void)
> >      >   {
> >      > diff --git a/lib/librte_eal/common/include/rte_memory.h
> >     b/lib/librte_eal/common/include/rte_memory.h
> >      > index ad3f3cfb0..eff028db1 100644
> >      > --- a/lib/librte_eal/common/include/rte_memory.h
> >      > +++ b/lib/librte_eal/common/include/rte_memory.h
> >      > @@ -466,6 +466,16 @@ unsigned rte_memory_get_nrank(void);
> >      >   /* check memsegs iovas are within a range based on dma mask */
> >      >   int __rte_experimental rte_mem_check_dma_mask(uint8_t maskbits);
> >      >
> >      > +/**
> >      > + *  * @warning
> >      > + * @b EXPERIMENTAL: this API may change without prior notice
> >
> >     I think there's a copy-paste error here (extra star and indent before
> >     warning).
> >
> >
> > I will fix this.
> >
> > Thanks.
> >
> >
> >      > + *
> >      > + *  Set dma mask to use once memory initialization is done.
> >      > + *  Previous function rte_mem_check_dma_mask can not be used
> >      > + *  safely until memory has been initialized.
> >
> >     IMO this really requires a big bold warning saying that this function
> >     must only be called early during initialization, before memory is
> >     initialized, and not to be called in user code.
> >
> >
> > what about adding a new EAL variable for keeping memory initialization
> > status?
> > It would be something like "uninit" until is changed to "done" after the
> > memory initialization is completed.
> > That variable would help to avoid effective calls to set the dma mask
> > after initialization.
>
> I'm not opposed to it in principle, however, this is a slippery slope
> towards having each and every subsystem storing their init status :)
>
> That said, while it's not a complete solution because it won't prevent
> spurious calls to this function _after_ memory has initialized, there is
> a variable in internal_config that you can use to deny calls to this
> function after EAL init is complete (see internal_config->init_complete).
>
> You could also store a static variable in eal_common_memory.c, and
> change it at the end of rte_eal_memory_init() call, but that does not
> really complete the memory initialization, because we then go to
> rte_eal_malloc_heap_init(), which actually completes the memory init.
>
> I would greatly prefer using internal_config->init_complete - it is
> enough to forbid user code from calling it, while driver/DPDK developers
> presumably know what they're doing enough to read the warning and not
> try to call this after memory init :)
>

I agree that we should avoid new status variables as I proposed, and in
this case
I think using internal_config->init_complete will be enough.

I will add the check in the next version.

Thanks


>
> >
> >      > + */
> >      > +void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits);
> >      > +
> >      >   /**
> >      >    * Drivers based on uio will not load unless physical
> >      >    * addresses are obtainable. It is only possible to get
> >      > diff --git a/lib/librte_eal/rte_eal_version.map
> >     b/lib/librte_eal/rte_eal_version.map
> >      > index ef8126a97..ae24b5c73 100644
> >      > --- a/lib/librte_eal/rte_eal_version.map
> >      > +++ b/lib/librte_eal/rte_eal_version.map
> >      > @@ -296,6 +296,7 @@ EXPERIMENTAL {
> >      >       rte_devargs_remove;
> >      >       rte_devargs_type_count;
> >      >       rte_mem_check_dma_mask;
> >      > +     rte_mem_set_dma_mask;
> >
> >     Same, this needs to be alphabetic.
> >
> >      >       rte_eal_cleanup;
> >      >       rte_fbarray_attach;
> >      >       rte_fbarray_destroy;
> >      >
> >
> >
> >     --
> >     Thanks,
> >     Anatoly
> >
>
>
> --
> Thanks,
> Anatoly
>