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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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 :)

> 
>      > + */
>      > +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