Re: [dpdk-dev] [PATCH v9 01/10] eal: move OS common config objects

[Thomas Monjalon <thomas@monjalon.net>]

24/06/2020 10:28, talshn@mellanox.com:
> +void

Could check the size in the function and return an error and log,
instead of doing the check prior the call.

> +eal_set_runtime_dir(char *run_dir, size_t size)
> +{
> +	strncpy(runtime_dir, run_dir, size);

strlcpy would be better

[...]
> +void
> +eal_config_remap(void *mem_cfg_addr)

"remap" word may be confusing.
What about "eal_config_switch"?

> +{
> +	memcpy(mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));

After review with David, another proposal:
Instead of creating this function,
we could just reference early_mem_config thanks to the getter:
	rte_eal_get_configuration()->mem_config

> +	rte_config.mem_config = mem_cfg_addr;
> +
> +	/* 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) mem_cfg_addr;
> +
> +	rte_config.mem_config->dma_maskbits = 0;
> +}

[...]
> --- a/lib/librte_eal/common/eal_common_dynmem.c
> +++ b/lib/librte_eal/common/eal_common_dynmem.c
> +	struct internal_config *internal_conf =
> +			eal_get_internal_configuration();

single or double indent for assignments?

[...]
> @@ -362,13 +368,16 @@ eal_dynmem_calc_num_pages_per_socket(
>  	unsigned int requested, available;
>  	int total_num_pages = 0;
>  	uint64_t remaining_mem, cur_mem;
> -	uint64_t total_mem = internal_config.memory;
> +	const struct internal_config *internal_conf =
> +			eal_get_internal_configuration();
> +
> +	uint64_t total_mem = internal_conf->memory;

Why this line is moved and separated with a blank line?

[...]
> @@ -491,7 +504,10 @@ int
>  rte_mem_alloc_validator_unregister(const char *name, int socket_id)
>  {
>  	/* FreeBSD boots with legacy mem enabled by default */

This comment must be moved.

> -	if (internal_config.legacy_mem) {
> +	const struct internal_config *internal_conf =
> +			eal_get_internal_configuration();
> +
> +	if (internal_conf->legacy_mem) {
>  		RTE_LOG(DEBUG, EAL, "Registering mem alloc validators not supported\n");

[...]
> -	internal_config.base_virtaddr =
> +	internal_conf->base_virtaddr =
>  		RTE_PTR_ALIGN_CEIL((uintptr_t)addr, (size_t)RTE_PGSIZE_16M);

This assignment had a single indent tab.
Better to be consistent with the new assignments:
[...]
> +	struct internal_config *internal_conf =
> +			eal_get_internal_configuration();

[...]
> @@ -807,7 +815,10 @@ rte_mp_sendmsg(struct rte_mp_msg *msg)
>  	if (check_input(msg) != 0)
>  		return -1;
>  
> -	if (internal_config.no_shconf) {
> +	const struct internal_config *internal_conf =
> +			eal_get_internal_configuration();

Please declare variable at the top of the function.

[...]
> --- a/lib/librte_eal/freebsd/eal.c
> +++ b/lib/librte_eal/freebsd/eal.c
> @@ -287,13 +252,7 @@ rte_eal_config_create(void)
>  		return -1;
>  	}
>  
> -	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
> -	rte_config.mem_config = rte_mem_cfg_addr;
> -
> -	/* 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;
> +	eal_config_remap(rte_mem_cfg_addr);

As described above, we could keep this code in the OS implementation,
and manage to get early_mem_config with the existing getter
rte_eal_get_configuration()->mem_config.

[...]
> @@ -443,7 +447,10 @@ close_hugefile(int fd, char *path, int list_idx)
>  	 * primary process must unlink the file, but only when not in in-memory
>  	 * mode (as in that case there is no file to unlink).
>  	 */
> -	if (!internal_config.in_memory &&
> +	const struct internal_config *internal_conf =
> +			eal_get_internal_configuration();

Better to declare the variable at the top
and keep the comment close to the next lines.

> +
> +	if (!internal_conf->in_memory &&
>  			rte_eal_process_type() == RTE_PROC_PRIMARY &&
>  			unlink(path))
>  		RTE_LOG(ERR, EAL, "%s(): unlinking '%s' failed: %s\n",


Thank you for the cleanup.
Next step will be to review the namespace of the internal functions,
the organization of the internal structures,
and probably add more getter functions.
There are a lot of cleanup to do in EAL. Any help is welcome!