Re: [PATCH 2/3] power: defer lcore variable allocation

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-12-05 18:57, David Marchand wrote:
> The lcore variable in this code unit is only used through
> rte_power_ethdev_pmgmt_queue_*() public symbols.
> 
> Defer the unconditional lcore variable allocation in those symbols.
> 
> Fixes: 130643319579 ("power: keep per-lcore state in lcore variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/power/rte_power_pmd_mgmt.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/power/rte_power_pmd_mgmt.c b/lib/power/rte_power_pmd_mgmt.c
> index a2fff3b765..29e2d438a3 100644
> --- a/lib/power/rte_power_pmd_mgmt.c
> +++ b/lib/power/rte_power_pmd_mgmt.c
> @@ -72,6 +72,19 @@ struct __rte_cache_aligned pmd_core_cfg {
>   };
>   static RTE_LCORE_VAR_HANDLE(struct pmd_core_cfg, lcore_cfgs);
>   
> +static void
> +alloc_lcore_cfgs(void)
> +{
> +	struct pmd_core_cfg *lcore_cfg;
> +	unsigned int lcore_id;
> +
> +	RTE_LCORE_VAR_ALLOC(lcore_cfgs);
> +
> +	/* initialize all tailqs */
> +	RTE_LCORE_VAR_FOREACH(lcore_id, lcore_cfg, lcore_cfgs)
> +		TAILQ_INIT(&lcore_cfg->head);
> +}
> +
>   static inline bool
>   queue_equal(const union queue *l, const union queue *r)
>   {
> @@ -517,6 +530,9 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
>   		goto end;
>   	}
>   
> +	if (lcore_cfgs == NULL)
> +		alloc_lcore_cfgs();
> +


I would wrap all RTE_LCORE_VAR_LCORE() and RTE_LCORE_VAR().

static struct pmd_core_cfg *
get_cfg_lcore(unsigned int lcore_id)
{
	assure_lcore_cfgs_alloced();
	return RTE_LCORE_VAR_LCORE(lcore_cfgs, lcore_id);
}

static struct pmd_core_cfg *
get_cfg(void)
{
	get_cfg_lcore(rte_lcore_id());
}

Add

static void
assure_lcore_cfgs_alloced(unsigned int lcore_id)
{
	if (lcore_cfgs != NULL)
		lcore_cfgs_alloc();
}

..or maybe better merge assure_lcore_cfgs_alloced() and lcore_cfgs_alloc().

Makes it a little harder to make mistakes.

A somewhat unrelated question: why is pmd_core_cfg cache-line aligned? I 
don't think it should be.

>   	lcore_cfg = RTE_LCORE_VAR_LCORE(lcore_id, lcore_cfgs);
>   
>   	/* check if other queues are stopped as well */
> @@ -617,6 +633,9 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
>   		return ret < 0 ? -EINVAL : -EBUSY;
>   	}
>   
> +	if (lcore_cfgs == NULL)
> +		alloc_lcore_cfgs();
> +
>   	/* no need to check queue id as wrong queue id would not be enabled */
>   	lcore_cfg = RTE_LCORE_VAR_LCORE(lcore_id, lcore_cfgs);
>   
> @@ -768,16 +787,8 @@ rte_power_pmd_mgmt_get_scaling_freq_max(unsigned int lcore)
>   }
>   
>   RTE_INIT(rte_power_ethdev_pmgmt_init) {
> -	unsigned int lcore_id;
> -	struct pmd_core_cfg *lcore_cfg;
>   	int i;
>   
> -	RTE_LCORE_VAR_ALLOC(lcore_cfgs);
> -
> -	/* initialize all tailqs */
> -	RTE_LCORE_VAR_FOREACH(lcore_id, lcore_cfg, lcore_cfgs)
> -		TAILQ_INIT(&lcore_cfg->head);
> -
>   	/* initialize config defaults */
>   	emptypoll_max = 512;
>   	pause_duration = 1;