On Sat, Oct 17, 2015 at 10:48:29AM +0300, Nissim Nisimov wrote:
> * this patch removes unnecessary call to rte_eal_memory_init() introduced in
> the first version.
>
> Problem:
> In DPDK Primary/Secondary module we assume mapping same regions of virtual
> memory addresses for Primary process and Secondary.
> An issue may occur when the Primary and secondary processes are not symmetric
> in such way that the code is not the same (for example, Primary process is a
> traffic distributer and secondary is a worker). The result may be that
> specific virtual address region in the first process won't be available in
> the second process.
>
> Changes done at eal init:
> map all related rte configuration and uio sections close to the end of huge
> pages memory (that mean rte_eal_memory_init() should be called before
> rte_config_init() in primary process)
> According to our observations there will be more probability to success when
> allocating rte_config and uio memzones after huge pages sections (actually
> uio is already allocated after the huge pages area)
>
> Signed-off-by: Nissim Nisimov <nissimn at radware.com>
Hi Nissim,
thanks for the V2. Some review comments below. Also, when applying there were
some whitespace errors reported from git:
git apply dpdk-dev-v2-eal-Map-rte-cfg-and-uio-at-the-end-of-hugepage-mem.patch
dpdk-dev-v2-eal-Map-rte-cfg-and-uio-at-the-end-of-hugepage-mem.patch:64:
trailing whitespace.
dpdk-dev-v2-eal-Map-rte-cfg-and-uio-at-the-end-of-hugepage-mem.patch:111: space
before tab in indent.
pci_map_addr = pci_find_max_end_va();
dpdk-dev-v2-eal-Map-rte-cfg-and-uio-at-the-end-of-hugepage-mem.patch:113: space
before tab in indent.
pci_map_addr =
(void*)RTE_PTR_ALIGN(((char*)rte_eal_get_configuration()->mem_config->mem_cfg_addr
+ sizeof(struct rte_mem_config)),sysconf(_SC_PAGE_SIZE));
warning: 3 lines add whitespace errors.
> ---
> lib/librte_eal/linuxapp/eal/eal.c | 28 +++++++++++++++++++++-------
> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 10 +++++++---
> 2 files changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> b/lib/librte_eal/linuxapp/eal/eal.c
> index 33e1067..f85eb63 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -87,6 +87,7 @@
>
> #define SOCKET_MEM_STRLEN (RTE_MAX_NUMA_NODES * 10)
>
> +void *pci_find_max_end_va(void);
Probably want a blank line after the prototype, since it's unrelated to the line
afterward. Maybe mark it extern too.
[The alternative to this function prototype here is obviously to include
eal_pci_init.h to get the function prototype - not sure which solution is
better]
> /* Allow the application to print its usage message too if set */
> static rte_usage_hook_t rte_application_usage_hook = NULL;
>
> @@ -189,12 +190,15 @@ rte_eal_config_create(void)
> return;
>
> /* map the config before hugepage address so that we don't waste a page
> */
> - if (internal_config.base_virtaddr != 0)
> + 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;
> + }
> + else{
> + rte_mem_cfg_addr = pci_find_max_end_va();
Would it work as well putting the config immediately before the hugepages
rather than at the end, as is done in the case of having a cmdline-specified
virtual
address? If so, it should remove the need to change the eal_pci_uio.c file.
> + RTE_LOG(INFO, EAL, "rte_mem_cfg_addr = 0x%llx
> PType=%s\n",(unsigned long long)rte_mem_cfg_addr,rte_config.process_type ==
> RTE_PROC_PRIMARY ? "PRIMARY" : "SECONDARY");
> + }
>
I would suggest making the RTE_LOG statment unconditional. Even having it
printing when the value is specified can be useful to confirm the parameter was
parsed correctly. [It would also reduce the diff as you won't have to add in the
braces to the "if/else" statements].
Also:
* use tabs, not spaces for indentation.
* I would suggest making the LOG statement be at the debug, rather than info
level, as it's not something that normally needs to be printed.
> if (mem_cfg_fd < 0){
> mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0660);
> @@ -227,7 +231,7 @@ rte_eal_config_create(void)
> /* store address of the config in the config itself so that secondary
> * processes could later map the config into this exact location */
> rte_config.mem_config->mem_cfg_addr = (uintptr_t) rte_mem_cfg_addr;
> -
> +
> }
Remove this whitespace change from diff.
>
> /* attach to an existing shared memory config */
> @@ -784,6 +788,13 @@ rte_eal_init(int argc, char **argv)
>
> rte_srand(rte_rdtsc());
>
> + /* Primary process should allocate hugepages before configuration */
> + if(internal_config.process_type == RTE_PROC_PRIMARY){
> + RTE_LOG(INFO, EAL, "Calling rte_eal_memory_init as
> =%s\n",(rte_config.process_type == RTE_PROC_PRIMARY) ? "PRIMARY" :
> "SECONDARY");
Split long lines like this after the comma. However, in this case, the process
type is already known, so just hardcode the printing.
Q: Is the printout actually needed - even when debugging? Are there not already
statements in the code to tell if it's a primary or secondary process? [Without
the printout, the two if statments can be collapsed into one.]
> + if (rte_eal_memory_init() < 0)
> + rte_panic("Cannot init memory\n");
> + }
> +
> rte_config_init();
>
> if (rte_eal_pci_init() < 0)
> @@ -793,9 +804,12 @@ rte_eal_init(int argc, char **argv)
> if (rte_eal_ivshmem_init() < 0)
> rte_panic("Cannot init IVSHMEM\n");
> #endif
> -
> - if (rte_eal_memory_init() < 0)
> - rte_panic("Cannot init memory\n");
> + /* secondary process will call memory init only after calling to
> rte_config_init() */
> + if(internal_config.process_type == RTE_PROC_SECONDARY){
> + RTE_LOG(INFO, EAL, "Calling rte_eal_memory_init as
> =%s\n",rte_config.process_type == RTE_PROC_PRIMARY ? "PRIMARY" : "SECONDARY");
> + if (rte_eal_memory_init() < 0)
> + rte_panic("Cannot init memory\n");
> + }
Same comment as above.
>
> /* the directories are locked during eal_hugepage_info_init */
> eal_hugedirs_unlock();
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index ac50e13..6812c37 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -338,9 +338,13 @@ pci_uio_map_resource_by_index(struct rte_pci_device
> *dev, int res_idx,
> }
>
> /* try mapping somewhere close to the end of hugepages */
> - if (pci_map_addr == NULL)
> - pci_map_addr = pci_find_max_end_va();
> -
> + if (pci_map_addr == NULL){
> + if (internal_config.base_virtaddr != 0){
> + pci_map_addr = pci_find_max_end_va();
> + } else{
> + pci_map_addr =
> (void*)RTE_PTR_ALIGN(((char*)rte_eal_get_configuration()->mem_config->mem_cfg_addr
> + sizeof(struct rte_mem_config)),sysconf(_SC_PAGE_SIZE));
> + }
> + }
> mapaddr = pci_map_resource(pci_map_addr, fd, 0,
> (size_t)dev->mem_resource[res_idx].len, 0);
> close(fd);
> --
> 2.6.1
>
