From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id DEC0AA0487
	for <public@inbox.dpdk.org>; Thu,  4 Jul 2019 09:50:55 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id A267F1B942;
	Thu,  4 Jul 2019 09:50:55 +0200 (CEST)
Received: from mail-vs1-f67.google.com (mail-vs1-f67.google.com
 [209.85.217.67]) by dpdk.org (Postfix) with ESMTP id 626461041
 for <dev@dpdk.org>; Thu,  4 Jul 2019 09:50:54 +0200 (CEST)
Received: by mail-vs1-f67.google.com with SMTP id k9so1430512vso.5
 for <dev@dpdk.org>; Thu, 04 Jul 2019 00:50:54 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20161025;
 h=x-gm-message-state:mime-version:references:in-reply-to:from:date
 :message-id:subject:to:cc;
 bh=nTdfD2R7N7Omcr2E7qzU367ZK1jRenQCXyo5MoZq7yE=;
 b=YC2MKa8jxzHMN4okYXjPZSg5y5KD7M58hiMWr30Iecvzh0ZbbkOb16pQoO/UhbUku5
 lTm/iyppPJC9NoCX2knytRUx/cZb9JoEbn4DeZFjbPzu9wVPusTT8614HdRPypN9dhCe
 p4djum44OyUN9+v91PMl81X+5iimwy7l4QeJFKFtA8lI7pWLKMfVU0ETBkq5kIHHE1T6
 38LLGvl486/cM9zh4ToJBsbvp0V3TZuUQ8WU+g49KpyaRhA+jeQCmTMCI7Cc+RuQ5I9N
 YctP27E+Wr41abq49H9kHB7HVjSMWn7XH4ie7wNo9EL4J3E3doQbJBY3x4TMq7eRBLCo
 vp7A==
X-Gm-Message-State: APjAAAXszyrGC7gfElS+AHTxFoA15axMgtz6PC8ID5mNVPCwsm73jxRz
 PYqXf8wSE5YXYRIU8HJ63jnzm6LNhA9KGmo2YnZ1zw==
X-Google-Smtp-Source: APXvYqzKLGUX9tx5Epgaa80JH+BlZzvrhO9qNwZMP0hK4BMcp44dgxGkhzfPb1Z47V/MagoBCMLw10ddzHvjEoATCOU=
X-Received: by 2002:a67:e9ca:: with SMTP id q10mr21427584vso.105.1562226653816; 
 Thu, 04 Jul 2019 00:50:53 -0700 (PDT)
MIME-Version: 1.0
References: <cover.1561478388.git.anatoly.burakov@intel.com>
 <cover.1561635481.git.anatoly.burakov@intel.com>
 <b49510c5323cceaea7405292637f8bd3d24e8ad7.1561635481.git.anatoly.burakov@intel.com>
In-Reply-To: <b49510c5323cceaea7405292637f8bd3d24e8ad7.1561635481.git.anatoly.burakov@intel.com>
From: David Marchand <david.marchand@redhat.com>
Date: Thu, 4 Jul 2019 09:50:43 +0200
Message-ID: <CAJFAV8wdXhfzRAHCdTGKm9DyY9y4WhNYoK=vvxzSNk1vSmuujA@mail.gmail.com>
To: Anatoly Burakov <anatoly.burakov@intel.com>
Cc: dev <dev@dpdk.org>, Bruce Richardson <bruce.richardson@intel.com>, 
 Thomas Monjalon <thomas@monjalon.net>,
 Stephen Hemminger <stephen@networkplumber.org>
Content-Type: text/plain; charset="UTF-8"
X-Content-Filtered-By: Mailman/MimeDel 2.1.15
Subject: Re: [dpdk-dev] [PATCH v3 13/14] eal: unify internal config
	initialization
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On Thu, Jun 27, 2019 at 1:39 PM Anatoly Burakov <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>
> ---
>  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.

rte_eal_config_create and rte_eal_config_attach are already awfully close
in linux and freebsd implementation.
Can't we have them as common code and put those helpers bits direct in them
?




> diff --git a/lib/librte_eal/linux/eal/eal.c
> b/lib/librte_eal/linux/eal/eal.c
> index 4fd18b15f..fa205fd29 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -472,24 +472,6 @@ eal_proc_type_detect(void)
>         return ptype;
>  }
>
> -/* copies data from internal config to shared config */
> -static void
> -eal_update_mem_config(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;
> -}
> -
> -/* copies data from shared config to internal config */
> -static void
> -eal_update_internal_config(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;
> -}
> -
>  /* Sets up rte_config structure with the pointer to shared memory
> config.*/
>  static int
>  rte_config_init(void)
> @@ -500,7 +482,7 @@ rte_config_init(void)
>         case RTE_PROC_PRIMARY:
>                 if (rte_eal_config_create() < 0)
>                         return -1;
> -               eal_update_mem_config();
> +               eal_mcfg_update_from_internal();
>                 break;
>         case RTE_PROC_SECONDARY:
>                 if (rte_eal_config_attach() < 0)
> @@ -508,7 +490,7 @@ rte_config_init(void)
>                 eal_mcfg_wait_complete();
>                 if (rte_eal_config_reattach() < 0)
>                         return -1;
> -               eal_update_internal_config();
> +               eal_mcfg_update_internal();
>                 break;
>         case RTE_PROC_AUTO:
>         case RTE_PROC_INVALID:
> --
> 2.17.1
>


-- 
David Marchand