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 11:30 AM Alejandro Lucero <
alejandro.lucero@netronome.com> wrote:

>
>
> 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'm wrong. And you already said it, that this will not avoid a bad usage
from PMDs.
I think I will follow your first advice about adding a warning about its
usage.



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