[PATCH 1/3] random: defer seeding to EAL init

[PATCH 1/3] random: defer seeding to EAL init

[David Marchand]

The RNG is documented as being seeded as part of EAL init.

/**
 * Seed the pseudo-random generator.
 *
 * The generator is automatically seeded by the EAL init with a timer
 * value. It may need to be re-seeded by the user with a real random
 * value.
 *
...

Move the initialisation (seeding) helper out of a constructor and
call it explicitly from rte_eal_init() as it was done before commit
3f002f069612 ("eal: replace libc-based random generation with LFSR").

This also moves the unconditional lcore variable allocation out of a
constructor.

While at it, mark local symbol rand_state as static.

Fixes: 29c39cd3d54d ("random: keep PRNG state in lcore variable")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_private.h | 6 ++++++
 lib/eal/common/rte_random.c  | 7 +++++--
 lib/eal/freebsd/eal.c        | 2 ++
 lib/eal/linux/eal.c          | 2 ++
 lib/eal/windows/eal.c        | 2 ++
 5 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h
index bb315dab04..a10db6a399 100644
--- a/lib/eal/common/eal_private.h
+++ b/lib/eal/common/eal_private.h
@@ -702,6 +702,12 @@ eal_get_internal_configuration(void);
 rte_usage_hook_t
 eal_get_application_usage_hook(void);
 
+/**
+ * Initialise random subsystem.
+ */
+void
+eal_rand_init(void);
+
 /**
  * Instruct primary process that a secondary process wants to attach.
  */
diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index cf0756f26a..307c26bb7c 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -14,6 +14,8 @@
 #include <rte_lcore_var.h>
 #include <rte_random.h>
 
+#include "eal_private.h"
+
 struct __rte_cache_aligned rte_rand_state {
 	uint64_t z1;
 	uint64_t z2;
@@ -22,7 +24,7 @@ struct __rte_cache_aligned rte_rand_state {
 	uint64_t z5;
 };
 
-RTE_LCORE_VAR_HANDLE(struct rte_rand_state, rand_state);
+static RTE_LCORE_VAR_HANDLE(struct rte_rand_state, rand_state);
 
 /* instance to be shared by all unregistered non-EAL threads */
 static struct rte_rand_state unregistered_rand_state;
@@ -228,7 +230,8 @@ __rte_random_initial_seed(void)
 	return rte_get_tsc_cycles();
 }
 
-RTE_INIT(rte_rand_init)
+void
+eal_rand_init(void)
 {
 	uint64_t seed;
 
diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index a96bbf5836..d07cff8651 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -777,6 +777,8 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
+	eal_rand_init();
+
 	eal_check_mem_on_local_socket();
 
 	if (rte_thread_set_affinity_by_id(rte_thread_self(),
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index a6220524a4..b1e63e37fc 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1173,6 +1173,8 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
+	eal_rand_init();
+
 	eal_check_mem_on_local_socket();
 
 	if (rte_thread_set_affinity_by_id(rte_thread_self(),
diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index 5cdc053a02..5c7526f922 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -410,6 +410,8 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
+	eal_rand_init();
+
 	if (rte_thread_set_affinity_by_id(rte_thread_self(),
 			&lcore_config[config->main_lcore].cpuset) != 0) {
 		rte_eal_init_alert("Cannot set affinity");
-- 
2.47.0

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

[David Marchand]

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();
+
 	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;
-- 
2.47.0

[PATCH 3/3] eal/x86: defer power intrinsics variable allocation

[David Marchand]

The lcore variable in this code unit is only used through
rte_power_monitor*() public symbols.

Defer the unconditional lcore variable allocation in those symbols.

Fixes: 18b5049ab4fe ("eal/x86: keep power intrinsics state in lcore variable")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/x86/rte_power_intrinsics.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
index e4cb913590..6fc3b2b1cb 100644
--- a/lib/eal/x86/rte_power_intrinsics.c
+++ b/lib/eal/x86/rte_power_intrinsics.c
@@ -22,8 +22,6 @@ struct power_wait_status {
 
 RTE_LCORE_VAR_HANDLE(struct power_wait_status, wait_status);
 
-RTE_LCORE_VAR_INIT(wait_status);
-
 /*
  * This function uses UMONITOR/UMWAIT instructions and will enter C0.2 state.
  * For more information about usage of these instructions, please refer to
@@ -177,6 +175,9 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
 	if (pmc->fn == NULL)
 		return -EINVAL;
 
+	if (wait_status == NULL)
+		RTE_LCORE_VAR_ALLOC(wait_status);
+
 	s = RTE_LCORE_VAR_LCORE(lcore_id, wait_status);
 
 	/* update sleep address */
@@ -269,6 +270,9 @@ rte_power_monitor_wakeup(const unsigned int lcore_id)
 	if (lcore_id >= RTE_MAX_LCORE)
 		return -EINVAL;
 
+	if (wait_status == NULL)
+		RTE_LCORE_VAR_ALLOC(wait_status);
+
 	s = RTE_LCORE_VAR_LCORE(lcore_id, wait_status);
 
 	/*
@@ -308,7 +312,7 @@ int
 rte_power_monitor_multi(const struct rte_power_monitor_cond pmc[],
 		const uint32_t num, const uint64_t tsc_timestamp)
 {
-	struct power_wait_status *s = RTE_LCORE_VAR(wait_status);
+	struct power_wait_status *s;
 	uint32_t i, rc;
 
 	/* check if supported */
@@ -318,6 +322,11 @@ rte_power_monitor_multi(const struct rte_power_monitor_cond pmc[],
 	if (pmc == NULL || num == 0)
 		return -EINVAL;
 
+	if (wait_status == NULL)
+		RTE_LCORE_VAR_ALLOC(wait_status);
+
+	s = RTE_LCORE_VAR(wait_status);
+
 	/* we are already inside transaction region, return */
 	if (rte_xtest() != 0)
 		return 0;
-- 
2.47.0

Re: [PATCH 1/3] random: defer seeding to EAL init

[Mattias Rönnblom]

On 2024-12-05 18:57, David Marchand wrote:
> The RNG is documented as being seeded as part of EAL init.
> 
> /**
>   * Seed the pseudo-random generator.
>   *
>   * The generator is automatically seeded by the EAL init with a timer
>   * value. It may need to be re-seeded by the user with a real random
>   * value.
>   *
> ...
> 

Remove the superfluous quote.

> Move the initialisation (seeding) helper out of a constructor and
> call it explicitly from rte_eal_init() as it was done before commit
> 3f002f069612 ("eal: replace libc-based random generation with LFSR").
> 
> This also moves the unconditional lcore variable allocation out of a
> constructor.
> 
> While at it, mark local symbol rand_state as static.
> 

Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

> Fixes: 29c39cd3d54d ("random: keep PRNG state in lcore variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/eal/common/eal_private.h | 6 ++++++
>   lib/eal/common/rte_random.c  | 7 +++++--
>   lib/eal/freebsd/eal.c        | 2 ++
>   lib/eal/linux/eal.c          | 2 ++
>   lib/eal/windows/eal.c        | 2 ++
>   5 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h
> index bb315dab04..a10db6a399 100644
> --- a/lib/eal/common/eal_private.h
> +++ b/lib/eal/common/eal_private.h
> @@ -702,6 +702,12 @@ eal_get_internal_configuration(void);
>   rte_usage_hook_t
>   eal_get_application_usage_hook(void);
>   
> +/**
> + * Initialise random subsystem.
> + */
> +void
> +eal_rand_init(void);
> +
>   /**
>    * Instruct primary process that a secondary process wants to attach.
>    */
> diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
> index cf0756f26a..307c26bb7c 100644
> --- a/lib/eal/common/rte_random.c
> +++ b/lib/eal/common/rte_random.c
> @@ -14,6 +14,8 @@
>   #include <rte_lcore_var.h>
>   #include <rte_random.h>
>   
> +#include "eal_private.h"
> +
>   struct __rte_cache_aligned rte_rand_state {
>   	uint64_t z1;
>   	uint64_t z2;
> @@ -22,7 +24,7 @@ struct __rte_cache_aligned rte_rand_state {
>   	uint64_t z5;
>   };
>   
> -RTE_LCORE_VAR_HANDLE(struct rte_rand_state, rand_state);
> +static RTE_LCORE_VAR_HANDLE(struct rte_rand_state, rand_state);
>   
>   /* instance to be shared by all unregistered non-EAL threads */
>   static struct rte_rand_state unregistered_rand_state;
> @@ -228,7 +230,8 @@ __rte_random_initial_seed(void)
>   	return rte_get_tsc_cycles();
>   }
>   
> -RTE_INIT(rte_rand_init)
> +void
> +eal_rand_init(void)
>   {
>   	uint64_t seed;
>   
> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index a96bbf5836..d07cff8651 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -777,6 +777,8 @@ rte_eal_init(int argc, char **argv)
>   		return -1;
>   	}
>   
> +	eal_rand_init();
> +
>   	eal_check_mem_on_local_socket();
>   
>   	if (rte_thread_set_affinity_by_id(rte_thread_self(),
> diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
> index a6220524a4..b1e63e37fc 100644
> --- a/lib/eal/linux/eal.c
> +++ b/lib/eal/linux/eal.c
> @@ -1173,6 +1173,8 @@ rte_eal_init(int argc, char **argv)
>   		return -1;
>   	}
>   
> +	eal_rand_init();
> +
>   	eal_check_mem_on_local_socket();
>   
>   	if (rte_thread_set_affinity_by_id(rte_thread_self(),
> diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
> index 5cdc053a02..5c7526f922 100644
> --- a/lib/eal/windows/eal.c
> +++ b/lib/eal/windows/eal.c
> @@ -410,6 +410,8 @@ rte_eal_init(int argc, char **argv)
>   		return -1;
>   	}
>   
> +	eal_rand_init();
> +
>   	if (rte_thread_set_affinity_by_id(rte_thread_self(),
>   			&lcore_config[config->main_lcore].cpuset) != 0) {
>   		rte_eal_init_alert("Cannot set affinity");

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: 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;

Re: [PATCH 3/3] eal/x86: defer power intrinsics 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_monitor*() public symbols.
> 
> Defer the unconditional lcore variable allocation in those symbols.
> 
> Fixes: 18b5049ab4fe ("eal/x86: keep power intrinsics state in lcore variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/eal/x86/rte_power_intrinsics.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
> index e4cb913590..6fc3b2b1cb 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -22,8 +22,6 @@ struct power_wait_status {
>   
>   RTE_LCORE_VAR_HANDLE(struct power_wait_status, wait_status);
>   
> -RTE_LCORE_VAR_INIT(wait_status);
> -
>   /*
>    * This function uses UMONITOR/UMWAIT instructions and will enter C0.2 state.
>    * For more information about usage of these instructions, please refer to
> @@ -177,6 +175,9 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>   	if (pmc->fn == NULL)
>   		return -EINVAL;
>   
> +	if (wait_status == NULL)
> +		RTE_LCORE_VAR_ALLOC(wait_status);
> +
>   	s = RTE_LCORE_VAR_LCORE(lcore_id, wait_status);
>   
>   	/* update sleep address */
> @@ -269,6 +270,9 @@ rte_power_monitor_wakeup(const unsigned int lcore_id)
>   	if (lcore_id >= RTE_MAX_LCORE)
>   		return -EINVAL;
>   
> +	if (wait_status == NULL)
> +		RTE_LCORE_VAR_ALLOC(wait_status);
> +
>   	s = RTE_LCORE_VAR_LCORE(lcore_id, wait_status);
>   
>   	/*
> @@ -308,7 +312,7 @@ int
>   rte_power_monitor_multi(const struct rte_power_monitor_cond pmc[],
>   		const uint32_t num, const uint64_t tsc_timestamp)
>   {
> -	struct power_wait_status *s = RTE_LCORE_VAR(wait_status);
> +	struct power_wait_status *s;
>   	uint32_t i, rc;
>   
>   	/* check if supported */
> @@ -318,6 +322,11 @@ rte_power_monitor_multi(const struct rte_power_monitor_cond pmc[],
>   	if (pmc == NULL || num == 0)
>   		return -EINVAL;
>   
> +	if (wait_status == NULL)
> +		RTE_LCORE_VAR_ALLOC(wait_status);
> +
> +	s = RTE_LCORE_VAR(wait_status);
> +
>   	/* we are already inside transaction region, return */
>   	if (rte_xtest() != 0)
>   		return 0;

The same kind of structure I suggested for the lib/power patch should be 
employed here.

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

[David Marchand]

On Fri, Dec 6, 2024 at 12:29 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> 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.

clb_multiwait, clb_pause and clb_scale_freq callbacks can only be
reached after a successful call to
rte_power_ethdev_pmgmt_queue_enable.
Triggering an allocation in them means we are hiding a (internal)
programatic error as allocation and use of a lcore variable are
clearly separated atm.
If we keep the lcore var api as is, I would add an assert() (maybe
under a debug build option) in RTE_LCORE_VAR macros themselves, as
calling with a NULL handle means the initialisation path in some
code/RTE_LCORE_VAR API use was incorrect.


Or because you propose to add the same type of helpers in both this
patch and the next, I am considering the other way: hide the
allocation in the RTE_LCORE_VAR* macros.
Checking for already allocated var in RTE_LCORE_VAR_ALLOC seems fine.
But the "fast path" RTE_LCORE_VAR would have an unwanted branch in most cases.


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

Before the conversion to per lcore variable, it was probably useful
(avoiding false sharing).
With the conversion, indeed, it looks like a waste of space.
It seems worth a separate fix.


-- 
David Marchand

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

[Mattias Rönnblom]

On 2024-12-12 08:57, David Marchand wrote:
> On Fri, Dec 6, 2024 at 12:29 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>> 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)
> 
> ==
> 

Oops.

>>                  lcore_cfgs_alloc();
>> }
>>
>> ..or maybe better merge assure_lcore_cfgs_alloced() and lcore_cfgs_alloc().
>>
>> Makes it a little harder to make mistakes.
> 
> clb_multiwait, clb_pause and clb_scale_freq callbacks can only be
> reached after a successful call to
> rte_power_ethdev_pmgmt_queue_enable.
> Triggering an allocation in them means we are hiding a (internal)
> programatic error as allocation and use of a lcore variable are
> clearly separated atm.
> If we keep the lcore var api as is, I would add an assert() (maybe
> under a debug build option) in RTE_LCORE_VAR macros themselves, as
> calling with a NULL handle means the initialisation path in some
> code/RTE_LCORE_VAR API use was incorrect.
> 

Sure, that would make sense. RTE_ASSERT(), that is. RTE_VERIFY() would 
be too expensive.

> 
> Or because you propose to add the same type of helpers in both this
> patch and the next, I am considering the other way: hide the
> allocation in the RTE_LCORE_VAR* macros.
> Checking for already allocated var in RTE_LCORE_VAR_ALLOC seems fine.
> But the "fast path" RTE_LCORE_VAR would have an unwanted branch in most cases.
> 

I would prefer to have the ALLOC() macro with semantics most people 
expect from a macro (or function) with that name, which is, I would 
argue, an unconditional allocation.

It would make sense to have another macro, which performs an allocation 
only if the handle is NULL.

RTE_LCORE_VAR_ASSURE_ALLOCATED(), or just RTE_LCORE_VAR_ASSURE() 
(although the latter sounds a little like an assertion, and not an 
allocation).

RTE_LCORE_VAR_LAZY_ALLOC()

I don't know. Something like that.

> 
>> A somewhat unrelated question: why is pmd_core_cfg cache-line aligned? I
>> don't think it should be.
> 
> Before the conversion to per lcore variable, it was probably useful
> (avoiding false sharing).
> With the conversion, indeed, it looks like a waste of space.
> It seems worth a separate fix.
> 
> 

You will include it, or should I submit a separate patch?

Re: [PATCH 1/3] random: defer seeding to EAL init

[Burakov, Anatoly]

On 12/5/2024 6:57 PM, David Marchand wrote:
> The RNG is documented as being seeded as part of EAL init.
> 
> /**
>   * Seed the pseudo-random generator.
>   *
>   * The generator is automatically seeded by the EAL init with a timer
>   * value. It may need to be re-seeded by the user with a real random
>   * value.
>   *
> ...
> 
> Move the initialisation (seeding) helper out of a constructor and
> call it explicitly from rte_eal_init() as it was done before commit
> 3f002f069612 ("eal: replace libc-based random generation with LFSR").
> 
> This also moves the unconditional lcore variable allocation out of a
> constructor.
> 
> While at it, mark local symbol rand_state as static.
> 
> Fixes: 29c39cd3d54d ("random: keep PRNG state in lcore variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

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

[David Marchand]

On Fri, Dec 13, 2024 at 7:58 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> On 2024-12-12 08:57, David Marchand wrote:
> > clb_multiwait, clb_pause and clb_scale_freq callbacks can only be
> > reached after a successful call to
> > rte_power_ethdev_pmgmt_queue_enable.
> > Triggering an allocation in them means we are hiding a (internal)
> > programatic error as allocation and use of a lcore variable are
> > clearly separated atm.
> > If we keep the lcore var api as is, I would add an assert() (maybe
> > under a debug build option) in RTE_LCORE_VAR macros themselves, as
> > calling with a NULL handle means the initialisation path in some
> > code/RTE_LCORE_VAR API use was incorrect.
> >
>
> Sure, that would make sense. RTE_ASSERT(), that is. RTE_VERIFY() would
> be too expensive.

Yes, I'll send in next revision.


>
> >
> > Or because you propose to add the same type of helpers in both this
> > patch and the next, I am considering the other way: hide the
> > allocation in the RTE_LCORE_VAR* macros.
> > Checking for already allocated var in RTE_LCORE_VAR_ALLOC seems fine.
> > But the "fast path" RTE_LCORE_VAR would have an unwanted branch in most cases.
> >
>
> I would prefer to have the ALLOC() macro with semantics most people
> expect from a macro (or function) with that name, which is, I would
> argue, an unconditional allocation.
> It would make sense to have another macro, which performs an allocation
> only if the handle is NULL.
>
> RTE_LCORE_VAR_ASSURE_ALLOCATED(), or just RTE_LCORE_VAR_ASSURE()
> (although the latter sounds a little like an assertion, and not an
> allocation).
>
> RTE_LCORE_VAR_LAZY_ALLOC()
>
> I don't know. Something like that.

- In the power libary case, allocating the lcore variable is followed
by the initialisation of the lcore variable internals (per lcore
tailqs).
For this patch, I will rename the alloc_lcore_cfgs helper I had in v1 as:

static void
+init_lcore_cfgs(void)
+{
+    struct pmd_core_cfg *lcore_cfg;
+    unsigned int lcore_id;
+
+    if (lcore_cfgs != NULL)
+        return;
+
+    RTE_LCORE_VAR_ALLOC(lcore_cfgs);
+
+    /* initialize all tailqs */
+    RTE_LCORE_VAR_FOREACH(lcore_id, lcore_cfg, lcore_cfgs)
+        TAILQ_INIT(&lcore_cfg->head);
+}

and only keep those checks in the public symbols.


- About more macros, I am wondering if this is needed in the end.
Adding assertions in the lcore var accessor should catch incorrect
initialisation path.


> >
> >> A somewhat unrelated question: why is pmd_core_cfg cache-line aligned? I
> >> don't think it should be.
> >
> > Before the conversion to per lcore variable, it was probably useful
> > (avoiding false sharing).
> > With the conversion, indeed, it looks like a waste of space.
> > It seems worth a separate fix.
> >
> >
>
> You will include it, or should I submit a separate patch?

I'll send it in next revision.


-- 
David Marchand

[PATCH v2 2/5] random: defer seeding to EAL init

[David Marchand]

The RNG is documented as being seeded as part of EAL init.

Move the initialisation (seeding) helper out of a constructor and
call it explicitly from rte_eal_init() as it was done before commit
3f002f069612 ("eal: replace libc-based random generation with LFSR").

This also moves the unconditional lcore variable allocation out of a
constructor.

While at it, mark local symbol rand_state as static.

Fixes: 29c39cd3d54d ("random: keep PRNG state in lcore variable")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
Changes since v1:
- updated commitlog,

---
 lib/eal/common/eal_private.h | 6 ++++++
 lib/eal/common/rte_random.c  | 7 +++++--
 lib/eal/freebsd/eal.c        | 2 ++
 lib/eal/linux/eal.c          | 2 ++
 lib/eal/windows/eal.c        | 2 ++
 5 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h
index bb315dab04..a10db6a399 100644
--- a/lib/eal/common/eal_private.h
+++ b/lib/eal/common/eal_private.h
@@ -702,6 +702,12 @@ eal_get_internal_configuration(void);
 rte_usage_hook_t
 eal_get_application_usage_hook(void);
 
+/**
+ * Initialise random subsystem.
+ */
+void
+eal_rand_init(void);
+
 /**
  * Instruct primary process that a secondary process wants to attach.
  */
diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index cf0756f26a..307c26bb7c 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -14,6 +14,8 @@
 #include <rte_lcore_var.h>
 #include <rte_random.h>
 
+#include "eal_private.h"
+
 struct __rte_cache_aligned rte_rand_state {
 	uint64_t z1;
 	uint64_t z2;
@@ -22,7 +24,7 @@ struct __rte_cache_aligned rte_rand_state {
 	uint64_t z5;
 };
 
-RTE_LCORE_VAR_HANDLE(struct rte_rand_state, rand_state);
+static RTE_LCORE_VAR_HANDLE(struct rte_rand_state, rand_state);
 
 /* instance to be shared by all unregistered non-EAL threads */
 static struct rte_rand_state unregistered_rand_state;
@@ -228,7 +230,8 @@ __rte_random_initial_seed(void)
 	return rte_get_tsc_cycles();
 }
 
-RTE_INIT(rte_rand_init)
+void
+eal_rand_init(void)
 {
 	uint64_t seed;
 
diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index a96bbf5836..d07cff8651 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -777,6 +777,8 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
+	eal_rand_init();
+
 	eal_check_mem_on_local_socket();
 
 	if (rte_thread_set_affinity_by_id(rte_thread_self(),
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index a6220524a4..b1e63e37fc 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1173,6 +1173,8 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
+	eal_rand_init();
+
 	eal_check_mem_on_local_socket();
 
 	if (rte_thread_set_affinity_by_id(rte_thread_self(),
diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index 5cdc053a02..5c7526f922 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -410,6 +410,8 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
+	eal_rand_init();
+
 	if (rte_thread_set_affinity_by_id(rte_thread_self(),
 			&lcore_config[config->main_lcore].cpuset) != 0) {
 		rte_eal_init_alert("Cannot set affinity");
-- 
2.47.0

[PATCH v2 3/5] power: defer lcore variable allocation

[David Marchand]

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>
---
Changes since v1:
- moved some unrelated comment on queue_id,
- renamed initialisation helper,

---
 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..369ce3c354 100644
--- a/lib/power/rte_power_pmd_mgmt.c
+++ b/lib/power/rte_power_pmd_mgmt.c
@@ -72,6 +72,22 @@ struct __rte_cache_aligned pmd_core_cfg {
 };
 static RTE_LCORE_VAR_HANDLE(struct pmd_core_cfg, lcore_cfgs);
 
+static void
+init_lcore_cfgs(void)
+{
+	struct pmd_core_cfg *lcore_cfg;
+	unsigned int lcore_id;
+
+	if (lcore_cfgs != NULL)
+		return;
+
+	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 +533,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
 		goto end;
 	}
 
+	init_lcore_cfgs();
 	lcore_cfg = RTE_LCORE_VAR_LCORE(lcore_id, lcore_cfgs);
 
 	/* check if other queues are stopped as well */
@@ -618,6 +635,8 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
 	}
 
 	/* no need to check queue id as wrong queue id would not be enabled */
+
+	init_lcore_cfgs();
 	lcore_cfg = RTE_LCORE_VAR_LCORE(lcore_id, lcore_cfgs);
 
 	/* check if other queues are stopped as well */
@@ -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;
-- 
2.47.0

[PATCH v2 5/5] eal/x86: defer power intrinsics variable allocation

[David Marchand]

The lcore variable in this code unit is only used through
rte_power_monitor*() public symbols.

Defer the unconditional lcore variable allocation in those symbols.

Fixes: 18b5049ab4fe ("eal/x86: keep power intrinsics state in lcore variable")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- added an initialiser helper,

---
 lib/eal/x86/rte_power_intrinsics.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
index e4cb913590..f7d558153e 100644
--- a/lib/eal/x86/rte_power_intrinsics.c
+++ b/lib/eal/x86/rte_power_intrinsics.c
@@ -22,7 +22,13 @@ struct power_wait_status {
 
 RTE_LCORE_VAR_HANDLE(struct power_wait_status, wait_status);
 
-RTE_LCORE_VAR_INIT(wait_status);
+static void
+init_wait_status(void)
+{
+	if (wait_status != NULL)
+		return;
+	RTE_LCORE_VAR_ALLOC(wait_status);
+}
 
 /*
  * This function uses UMONITOR/UMWAIT instructions and will enter C0.2 state.
@@ -177,6 +183,7 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
 	if (pmc->fn == NULL)
 		return -EINVAL;
 
+	init_wait_status();
 	s = RTE_LCORE_VAR_LCORE(lcore_id, wait_status);
 
 	/* update sleep address */
@@ -269,6 +276,7 @@ rte_power_monitor_wakeup(const unsigned int lcore_id)
 	if (lcore_id >= RTE_MAX_LCORE)
 		return -EINVAL;
 
+	init_wait_status();
 	s = RTE_LCORE_VAR_LCORE(lcore_id, wait_status);
 
 	/*
@@ -308,7 +316,7 @@ int
 rte_power_monitor_multi(const struct rte_power_monitor_cond pmc[],
 		const uint32_t num, const uint64_t tsc_timestamp)
 {
-	struct power_wait_status *s = RTE_LCORE_VAR(wait_status);
+	struct power_wait_status *s;
 	uint32_t i, rc;
 
 	/* check if supported */
@@ -318,6 +326,9 @@ rte_power_monitor_multi(const struct rte_power_monitor_cond pmc[],
 	if (pmc == NULL || num == 0)
 		return -EINVAL;
 
+	init_wait_status();
+	s = RTE_LCORE_VAR(wait_status);
+
 	/* we are already inside transaction region, return */
 	if (rte_xtest() != 0)
 		return 0;
-- 
2.47.0

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

[Burakov, Anatoly]

On 12/17/2024 9:59 AM, 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>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH v2 5/5] eal/x86: defer power intrinsics variable allocation

[Burakov, Anatoly]

On 12/17/2024 9:59 AM, David Marchand wrote:
> The lcore variable in this code unit is only used through
> rte_power_monitor*() public symbols.
> 
> Defer the unconditional lcore variable allocation in those symbols.
> 
> Fixes: 18b5049ab4fe ("eal/x86: keep power intrinsics state in lcore variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH v2 2/5] random: defer seeding to EAL init

[Stephen Hemminger]

On Tue, 17 Dec 2024 09:59:49 +0100
David Marchand <david.marchand@redhat.com> wrote:

> The RNG is documented as being seeded as part of EAL init.
> 
> Move the initialisation (seeding) helper out of a constructor and
> call it explicitly from rte_eal_init() as it was done before commit
> 3f002f069612 ("eal: replace libc-based random generation with LFSR").
> 
> This also moves the unconditional lcore variable allocation out of a
> constructor.
> 
> While at it, mark local symbol rand_state as static.
> 
> Fixes: 29c39cd3d54d ("random: keep PRNG state in lcore variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Probably need to add a check to rte_random() so it crashes
if called before initialization, rather than returning an un-random
number which could be a hidden long term bug.

Re: [PATCH v2 2/5] random: defer seeding to EAL init

[Mattias Rönnblom]

On 2024-12-18 17:35, Stephen Hemminger wrote:
> On Tue, 17 Dec 2024 09:59:49 +0100
> David Marchand <david.marchand@redhat.com> wrote:
> 
>> The RNG is documented as being seeded as part of EAL init.
>>
>> Move the initialisation (seeding) helper out of a constructor and
>> call it explicitly from rte_eal_init() as it was done before commit
>> 3f002f069612 ("eal: replace libc-based random generation with LFSR").
>>
>> This also moves the unconditional lcore variable allocation out of a
>> constructor.
>>
>> While at it, mark local symbol rand_state as static.
>>
>> Fixes: 29c39cd3d54d ("random: keep PRNG state in lcore variable")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Probably need to add a check to rte_random() so it crashes
> if called before initialization, rather than returning an un-random
> number which could be a hidden long term bug.

If you do, do it either as a RTE_ASSERT() or an RTE_VERIFY() in the 
(lcore_id == LCORE_ID_ANY) path, since that is what will be taken.

Preferably, you should have as little as possible in rte_rand() fast 
path, because this function is used in packet processing.

That said, the "unrandom" number will always be 0, so it shouldn't go 
unnoticed for too long.