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 93818A0471
	for <public@inbox.dpdk.org>; Mon, 12 Aug 2019 12:19:38 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id AB93549E0;
	Mon, 12 Aug 2019 12:19:37 +0200 (CEST)
Received: from mail-vk1-f194.google.com (mail-vk1-f194.google.com
 [209.85.221.194]) by dpdk.org (Postfix) with ESMTP id CC50D378B
 for <dev@dpdk.org>; Mon, 12 Aug 2019 12:19:35 +0200 (CEST)
Received: by mail-vk1-f194.google.com with SMTP id w186so12238487vkd.11
 for <dev@dpdk.org>; Mon, 12 Aug 2019 03:19:35 -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=dga2iZRDEIxswgAbVOsHeQWFdwbFkopAJ3ePAi3zR2Q=;
 b=Edd8L0QBEe0tphfAIRXtlwpbGMDX72jDnYSCuX2SsQ3M5FID0ahNwwOkS7x4fzsKSg
 Vssbsqb7olkpJDpzOr0kVNF6yYn5PCl4nI9JZknHbXXpepNsSv78uH5j4M3aYugLlBE7
 OeQSPuEJsILmdfVq+iXrgjWviFiKiTgfFag+IokxZaPamfGF19Z2i3JKGZqeWymOmBKD
 TyE4dFtkW8/BJRygLjX86ZVBhQXdYFd+5zxdWIiDtkjXHwxI5Y+FKhjqn6S4WbSqCcte
 0YzMlB6er5GsWLjwXOAlHxTB4wB2ifspx0aqnH6NaVQpu487tk5ce8A7d3v1In/3/gzM
 vCpQ==
X-Gm-Message-State: APjAAAVuYg8ipGedJcaeiXYLrsx+ywZ8UDxadzyet4cKJZKQmCGdYLpX
 h93KTXnMlZK5jeBlp1rtD9KL/Gw6PLDwbfWgwmDcdw==
X-Google-Smtp-Source: APXvYqyQCDMH+FpCb9ODenl9hHaRE0TMrrSA9ST1P46JKmQW0IzOBlfbIeBG0f/CFM6beSyjXa7xKC35TRWyP3ypJmE=
X-Received: by 2002:a1f:5185:: with SMTP id f127mr2229740vkb.52.1565605175068; 
 Mon, 12 Aug 2019 03:19:35 -0700 (PDT)
MIME-Version: 1.0
References: <26384fad6f9afcba9d3311d1bf73e76f4577225d.1563276298.git.anatoly.burakov@intel.com>
In-Reply-To: <26384fad6f9afcba9d3311d1bf73e76f4577225d.1563276298.git.anatoly.burakov@intel.com>
From: David Marchand <david.marchand@redhat.com>
Date: Mon, 12 Aug 2019 12:19:22 +0200
Message-ID: <CAJFAV8xQ__KNVSmf9e3C5T=T3FNjExR=7+F-O+vV+hv_OPUYsw@mail.gmail.com>
To: Anatoly Burakov <anatoly.burakov@intel.com>
Cc: dev <dev@dpdk.org>, John McNamara <john.mcnamara@intel.com>, 
 Marko Kovacevic <marko.kovacevic@intel.com>,
 Bruce Richardson <bruce.richardson@intel.com>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [dpdk-dev] [PATCH] eal/freebsd: add support for base virtaddr
	option
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 Tue, Jul 16, 2019 at 1:25 PM Anatoly Burakov
<anatoly.burakov@intel.com> wrote:
>
> According to our docs, only Linuxapp supports base-virtaddr option.
> That is, strictly speaking, not true because most of the things
> that are attempting to respect base-virtaddr are in common files,
> so FreeBSD already *mostly* supports this option in practice.
>
> This commit fixes the remaining bits to explicitly support
> base-virtaddr option, and moves the arg parsing from EAL to common
> options parsing code. Documentation is also updated to reflect
> that all platforms now support base-virtaddr.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  doc/guides/linux_gsg/eal_args.include.rst     |  6 +++
>  doc/guides/linux_gsg/linux_eal_parameters.rst |  6 ---
>  doc/guides/rel_notes/release_19_08.rst        |  5 +++
>  lib/librte_eal/common/eal_common_options.c    | 38 ++++++++++++++++++
>  lib/librte_eal/freebsd/eal/eal.c              | 13 ++++++-
>  lib/librte_eal/linux/eal/eal.c                | 39 -------------------
>  6 files changed, 60 insertions(+), 47 deletions(-)
>
> diff --git a/doc/guides/linux_gsg/eal_args.include.rst b/doc/guides/linux_gsg/eal_args.include.rst
> index cf421a56e..ed8b0e35b 100644
> --- a/doc/guides/linux_gsg/eal_args.include.rst
> +++ b/doc/guides/linux_gsg/eal_args.include.rst
> @@ -86,6 +86,12 @@ Multiprocessing-related options
>
>      Set the type of the current process.
>
> +*   ``--base-virtaddr <address>``
> +
> +    Attempt to use a different starting address for all memory maps of the
> +    primary DPDK process. This can be helpful if secondary processes cannot
> +    start due to conflicts in address map.
> +

doc/guides/freebsd_gsg/freebsd_eal_parameters.rst:.. include::
../linux_gsg/eal_args.include.rst

Ok, a bit misleading to put in linux_gsg/, so writing this here if
someone else looks at this :-)


>  Memory-related options
>  ~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/doc/guides/linux_gsg/linux_eal_parameters.rst b/doc/guides/linux_gsg/linux_eal_parameters.rst
> index c63f0f49a..b2cc60e44 100644
> --- a/doc/guides/linux_gsg/linux_eal_parameters.rst
> +++ b/doc/guides/linux_gsg/linux_eal_parameters.rst
> @@ -49,12 +49,6 @@ Multiprocessing-related options
>      allows running multiple independent DPDK primary/secondary processes under
>      different prefixes.
>
> -*   ``--base-virtaddr <address>``
> -
> -    Attempt to use a different starting address for all memory maps of the
> -    primary DPDK process. This can be helpful if secondary processes cannot
> -    start due to conflicts in address map.
> -
>  Memory-related options
>  ~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/doc/guides/rel_notes/release_19_08.rst b/doc/guides/rel_notes/release_19_08.rst
> index 4a1fd8dd8..1b58d9282 100644
> --- a/doc/guides/rel_notes/release_19_08.rst
> +++ b/doc/guides/rel_notes/release_19_08.rst
> @@ -56,6 +56,11 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
>
> +* **FreeBSD now supports `--base-virtaddr` EAL option.**
> +
> +  FreeBSD version now also supports setting base virtual address for mapping
> +  pages and resources into its address space.
> +
>  * **Added MCS lock.**
>
>    MCS lock provides scalability by spinning on a CPU/thread local variable

Well, obviously, this needs some rebase on 19.11-rc0 :-)

> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 512d5088e..156e48e19 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -20,6 +20,7 @@
>  #include <rte_eal.h>
>  #include <rte_log.h>
>  #include <rte_lcore.h>
> +#include <rte_memory.h>
>  #include <rte_tailq.h>
>  #include <rte_version.h>
>  #include <rte_devargs.h>
> @@ -1095,6 +1096,36 @@ eal_parse_iova_mode(const char *name)
>         return 0;
>  }
>
> +static int
> +eal_parse_base_virtaddr(const char *arg)
> +{
> +       char *end;
> +       uint64_t addr;
> +
> +       errno = 0;
> +       addr = strtoull(arg, &end, 16);
> +
> +       /* check for errors */
> +       if ((errno != 0) || (arg[0] == '\0') || end == NULL || (*end != '\0'))
> +               return -1;
> +
> +       /* make sure we don't exceed 32-bit boundary on 32-bit target */
> +#ifndef RTE_ARCH_64
> +       if (addr >= UINTPTR_MAX)
> +               return -1;
> +#endif
> +
> +       /* align the addr on 16M boundary, 16MB is the minimum huge page
> +        * size on IBM Power architecture. If the addr is aligned to 16MB,
> +        * it can align to 2MB for x86. So this alignment can also be used
> +        * on x86 and other architectures.
> +        */
> +       internal_config.base_virtaddr =
> +               RTE_PTR_ALIGN_CEIL((uintptr_t)addr, (size_t)RTE_PGSIZE_16M);
> +
> +       return 0;
> +}
> +
>  /* caller is responsible for freeing the returned string */
>  static char *
>  available_cores(void)
> @@ -1408,6 +1439,13 @@ eal_parse_common_option(int opt, const char *optarg,
>                         return -1;
>                 }
>                 break;
> +       case OPT_BASE_VIRTADDR_NUM:
> +               if (eal_parse_base_virtaddr(optarg) < 0) {
> +                       RTE_LOG(ERR, EAL, "invalid parameter for --"
> +                                       OPT_BASE_VIRTADDR "\n");
> +                       return -1;
> +               }
> +               break;
>
>         /* don't know what to do, leave this to caller */
>         default:
> diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
> index d53f0fe69..7ebe11db6 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -227,6 +227,14 @@ rte_eal_config_create(void)
>         if (internal_config.no_shconf)
>                 return 0;
>
> +       /* map the config before base address so that we don't waste a page */
> +       if (internal_config.base_virtaddr != 0)
> +               rte_mem_cfg_addr = (void *)
> +                       RTE_ALIGN_FLOOR(internal_config.base_virtaddr -
> +                       sizeof(struct rte_mem_config), sysconf(_SC_PAGE_SIZE));
> +       else
> +               rte_mem_cfg_addr = NULL;
> +
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
>                 if (mem_cfg_fd < 0) {
> @@ -254,8 +262,9 @@ rte_eal_config_create(void)
>                 return -1;
>         }
>
> -       rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config),
> -                               PROT_READ | PROT_WRITE, MAP_SHARED, mem_cfg_fd, 0);
> +       rte_mem_cfg_addr = mmap(rte_mem_cfg_addr,
> +                       sizeof(*rte_config.mem_config), PROT_READ | PROT_WRITE,
> +                       MAP_SHARED, mem_cfg_fd, 0);
>
>         if (rte_mem_cfg_addr == MAP_FAILED){
>                 RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");

Nit: when compared to Linux implementation, the reattach step does not
recommend using the --base-virtaddr in case the remmapping failed.


> diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
> index 2e5499f9b..79f5d70c3 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -609,35 +609,6 @@ eal_parse_socket_arg(char *strval, volatile uint64_t *socket_arg)
>         return 0;
>  }
>
> -static int
> -eal_parse_base_virtaddr(const char *arg)
> -{
> -       char *end;
> -       uint64_t addr;
> -
> -       errno = 0;
> -       addr = strtoull(arg, &end, 16);
> -
> -       /* check for errors */
> -       if ((errno != 0) || (arg[0] == '\0') || end == NULL || (*end != '\0'))
> -               return -1;
> -
> -       /* make sure we don't exceed 32-bit boundary on 32-bit target */
> -#ifndef RTE_ARCH_64
> -       if (addr >= UINTPTR_MAX)
> -               return -1;
> -#endif
> -
> -       /* align the addr on 16M boundary, 16MB is the minimum huge page
> -        * size on IBM Power architecture. If the addr is aligned to 16MB,
> -        * it can align to 2MB for x86. So this alignment can also be used
> -        * on x86 */
> -       internal_config.base_virtaddr =
> -               RTE_PTR_ALIGN_CEIL((uintptr_t)addr, (size_t)RTE_PGSIZE_16M);
> -
> -       return 0;
> -}
> -
>  static int
>  eal_parse_vfio_intr(const char *mode)
>  {
> @@ -796,16 +767,6 @@ eal_parse_args(int argc, char **argv)
>                         internal_config.force_socket_limits = 1;
>                         break;
>
> -               case OPT_BASE_VIRTADDR_NUM:
> -                       if (eal_parse_base_virtaddr(optarg) < 0) {
> -                               RTE_LOG(ERR, EAL, "invalid parameter for --"
> -                                               OPT_BASE_VIRTADDR "\n");
> -                               eal_usage(prgname);
> -                               ret = -1;
> -                               goto out;
> -                       }
> -                       break;
> -
>                 case OPT_VFIO_INTR_NUM:
>                         if (eal_parse_vfio_intr(optarg) < 0) {
>                                 RTE_LOG(ERR, EAL, "invalid parameters for --"
> --
> 2.17.1


Reviewed-by: David Marchand <david.marchand@redhat.com>

-- 
David Marchand