Re: [dpdk-dev] [PATCH v3 13/14] eal: unify internal config initialization

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

On 04-Jul-19 11:54 AM, David Marchand wrote:
> 
> 
> On Thu, Jul 4, 2019 at 12:50 PM Burakov, Anatoly 
> <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> wrote:
> 
>     On 04-Jul-19 8:56 AM, David Marchand wrote:
>      > On Thu, Jul 4, 2019 at 9:50 AM David Marchand
>     <david.marchand@redhat.com <mailto:david.marchand@redhat.com>>
>      > wrote:
>      >
>      >>
>      >>
>      >> On Thu, Jun 27, 2019 at 1:39 PM Anatoly Burakov
>     <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>>
>      >> wrote:
>      >>
>      >>> Currently, each EAL will update internal/shared config in their
>      >>> own way at init, resulting in needless duplication of code and
>      >>> OS-dependent behavior. Move the functions to a common file and
>      >>> add missing FreeBSD steps.
>      >>>
>      >>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com
>     <mailto:anatoly.burakov@intel.com>>
>      >>> ---
>      >>>   lib/librte_eal/common/eal_common_mcfg.c | 18 ++++++++++++++++++
>      >>>   lib/librte_eal/common/eal_memcfg.h      |  8 ++++++++
>      >>>   lib/librte_eal/freebsd/eal/eal.c        |  2 ++
>      >>>   lib/librte_eal/linux/eal/eal.c          | 22
>     ++--------------------
>      >>>   4 files changed, 30 insertions(+), 20 deletions(-)
>      >>>
>      >>> diff --git a/lib/librte_eal/common/eal_common_mcfg.c
>      >>> b/lib/librte_eal/common/eal_common_mcfg.c
>      >>> index dc6665d6a..fe8d2b726 100644
>      >>> --- a/lib/librte_eal/common/eal_common_mcfg.c
>      >>> +++ b/lib/librte_eal/common/eal_common_mcfg.c
>      >>> @@ -31,6 +31,24 @@ eal_mcfg_wait_complete(void)
>      >>>                  rte_pause();
>      >>>   }
>      >>>
>      >>> +void
>      >>> +eal_mcfg_update_internal(void)
>      >>> +{
>      >>> +       struct rte_mem_config *mcfg =
>      >>> rte_eal_get_configuration()->mem_config;
>      >>> +
>      >>> +       internal_config.legacy_mem = mcfg->legacy_mem;
>      >>> +       internal_config.single_file_segments =
>     mcfg->single_file_segments;
>      >>> +}
>      >>> +
>      >>> +void
>      >>> +eal_mcfg_update_from_internal(void)
>      >>> +{
>      >>> +       struct rte_mem_config *mcfg =
>      >>> rte_eal_get_configuration()->mem_config;
>      >>> +
>      >>> +       mcfg->legacy_mem = internal_config.legacy_mem;
>      >>> +       mcfg->single_file_segments =
>     internal_config.single_file_segments;
>      >>> +}
>      >>> +
>      >>>   void
>      >>>   rte_mcfg_mem_read_lock(void)
>      >>>   {
>      >>> diff --git a/lib/librte_eal/common/eal_memcfg.h
>      >>> b/lib/librte_eal/common/eal_memcfg.h
>      >>> index a2434417e..d02ac1621 100644
>      >>> --- a/lib/librte_eal/common/eal_memcfg.h
>      >>> +++ b/lib/librte_eal/common/eal_memcfg.h
>      >>> @@ -68,6 +68,14 @@ struct rte_mem_config {
>      >>>          uint8_t dma_maskbits;
>      >>>   };
>      >>>
>      >>> +/* update internal config from shared mem config */
>      >>> +void
>      >>> +eal_mcfg_update_internal(void);
>      >>> +
>      >>> +/* update shared mem config from internal config */
>      >>> +void
>      >>> +eal_mcfg_update_from_internal(void);
>      >>> +
>      >>>   /* wait until primary process initialization is complete */
>      >>>   void
>      >>>   eal_mcfg_wait_complete(void);
>      >>> diff --git a/lib/librte_eal/freebsd/eal/eal.c
>      >>> b/lib/librte_eal/freebsd/eal/eal.c
>      >>> index 13e230fc8..6bfe203fd 100644
>      >>> --- a/lib/librte_eal/freebsd/eal/eal.c
>      >>> +++ b/lib/librte_eal/freebsd/eal/eal.c
>      >>> @@ -379,6 +379,7 @@ rte_config_init(void)
>      >>>          case RTE_PROC_PRIMARY:
>      >>>                  if (rte_eal_config_create() < 0)
>      >>>                          return -1;
>      >>> +               eal_mcfg_update_internal();
>      >>>                  break;
>      >>>          case RTE_PROC_SECONDARY:
>      >>>                  if (rte_eal_config_attach() < 0)
>      >>> @@ -386,6 +387,7 @@ rte_config_init(void)
>      >>>                  eal_mcfg_wait_complete();
>      >>>                  if (rte_eal_config_reattach() < 0)
>      >>>                          return -1;
>      >>> +               eal_mcfg_update_from_internal();
>      >>>                  break;
>      >>>          case RTE_PROC_AUTO:
>      >>>          case RTE_PROC_INVALID:
>      >>>
>      >>
>      >>
>      >> Hum, you swapped eal_mcfg_update_internal and
>      >> eal_mcfg_update_from_internal.
>      >> The names are a bit ambiguous, and I wonder if we really need those
>      >> separate helpers.
>      >>
>      >
>      > Replying to myself... at least those helpers try to do something
>      > explicitly: synchronising the local copy of the configuration
>     with the
>      > shared mem config.
>      > Keeping them separate documents this step.
>      >
>      > Anyway, your choice, but you must fix freebsd :-)
>      >
> 
>     Sorry, i don't follow - what needs to be fixed in FreeBSD?
> 
> 
> eal_mcfg_update_from_internal() must be called in primary process.
> eal_mcfg_update_internal() must be called in secondary processes.
> 

Oh, right, good catch, thanks!

> 
> -- 
> David Marchand


-- 
Thanks,
Anatoly