Re: [dpdk-dev] [PATCH] eal/freebsd: add support for base virtaddr option

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

On 12-Aug-19 11:19 AM, David Marchand wrote:
> 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 :-)

This was agreed upon when this file was first introduced. We don't have 
a "common" section and there's no real way to create it without 
triggering a bunch of errors in doxygen, so it was decided that putting 
this in a Linux GSG is the best way to do 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 :-)

Yes, will do.

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

<snip>

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

Good catch, will fix.

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

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


-- 
Thanks,
Anatoly