* Re: [dpdk-dev] [PATCH v2] eal:Map rte cfg and uio at the end of hugepage mem
2015-10-17 7:48 [dpdk-dev] [PATCH v2] eal:Map rte cfg and uio at the end of hugepage mem Nissim Nisimov
@ 2015-10-19 13:07 ` Bruce Richardson
0 siblings, 0 replies; 2+ messages in thread
From: Bruce Richardson @ 2015-10-19 13:07 UTC (permalink / raw)
To: Nissim Nisimov; +Cc: dev
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@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
>
^ permalink raw reply [flat|nested] 2+ messages in thread