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

[Mattias Rönnblom]

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: sta...@dpdk.org

Signed-off-by: David Marchand <david.march...@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;