[dpdk-dev] [PATCH v1] examples/distributor: detect high frequency cores

[dpdk-dev] [PATCH v1] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the cores
provided in the core mask, and if any high frequency cores are found
(e.g. Turbo Boost is enabled), we will pin the distributor workload to
that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
---
 examples/distributor/main.c      | 185 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 149 insertions(+), 38 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..0541c50b0 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -281,6 +282,7 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -364,6 +366,8 @@ lcore_distributor(struct lcore_params *p)
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
 
+	rte_power_exit(rte_lcore_id());
+
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +439,7 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +580,32 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	rte_power_exit(rte_lcore_id());
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
+		if (rte_lcore_is_enabled(lcore_id)) {
+			/* init power management library */
+			ret = rte_power_init(lcore_id);
+			if (ret)
+				RTE_LOG(ERR, POWER,
+				"Library initialization failed on core %u\n",
+				lcore_id);
+				/*
+				 * Return on first failure, we'll fall back
+				 * to non-power operation
+				 */
+				return ret;
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,11 +685,15 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0, priority_num = 0;
+	unsigned int distr_core_id = 0, rx_core_id = 0, tx_core_id = 0;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
 	uint64_t t, freq;
+	unsigned int counter = 0;
+	unsigned int power_lib_initialised = 0;
 
 	/* catch ctrl-c so we can print on exit */
 	signal(SIGINT, int_handler);
@@ -687,6 +719,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +777,126 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.turbo == 1) {
+				priority_num++;
+				switch (priority_num) {
+				case 1:
+					distr_core_id = lcore_id;
+					printf("Distributor on priority core %d\n",
+							lcore_id);
+					break;
+				case 2:
+					rx_core_id = lcore_id;
+					printf("Rx on preferred core %d\n",
+							lcore_id);
+					break;
+				case 3:
+					tx_core_id = lcore_id;
+					printf("Tx on preferred core %d\n",
+							lcore_id);
+					break;
+				default:
+					break;
+				}
+			}
+		}
+	}
+
+	/*
+	 * If there's  any of the key workloads left without an lcore_id
+	 * after the higer frequency core assignment above, pre-assign
+	 * them here.
+	 */
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
-					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
-				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
+
+		if (distr_core_id == 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+		}
+		if ((rx_core_id == 0) &&
+				(lcore_id != distr_core_id)) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+		}
+		if ((tx_core_id == 0) &&
+				(lcore_id != distr_core_id) &&
+				(lcore_id != rx_core_id)) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+		}
+		counter++;
+	}
+
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+		if ((lcore_id == distr_core_id) ||
+			(lcore_id == rx_core_id) ||
+			(lcore_id == tx_core_id)) {
+
 		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
+
+			printf("Starting thread %d as worker, lcore_id %d\n",
 					worker_id, lcore_id);
 			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
+				rte_malloc(NULL, sizeof(*p), 0);
 			if (!p)
 				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
+			*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+				dist_tx_ring, mbuf_pool};
 
 			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
 					p, lcore_id);
 		}
-		worker_id++;
 	}
 
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +913,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

Re: [dpdk-dev] [PATCH v1] examples/distributor: detect high frequency cores

[Burakov, Anatoly]

On 22-Feb-19 11:45 AM, David Hunt wrote:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
> 
> This patch uses the rte_power_get_capabilities() API to query the cores
> provided in the core mask, and if any high frequency cores are found
> (e.g. Turbo Boost is enabled), we will pin the distributor workload to
> that core.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
>   examples/distributor/main.c      | 185 ++++++++++++++++++++++++-------
>   examples/distributor/meson.build |   2 +-
>   2 files changed, 149 insertions(+), 38 deletions(-)
> 
> diff --git a/examples/distributor/main.c b/examples/distributor/main.c
> index 03a05e3d9..0541c50b0 100644
> --- a/examples/distributor/main.c
> +++ b/examples/distributor/main.c
> @@ -16,6 +16,7 @@
>   #include <rte_prefetch.h>
>   #include <rte_distributor.h>
>   #include <rte_pause.h>
> +#include <rte_power.h>
>   
>   #define RX_RING_SIZE 1024
>   #define TX_RING_SIZE 1024
> @@ -281,6 +282,7 @@ lcore_rx(struct lcore_params *p)
>   		if (++port == nb_ports)
>   			port = 0;
>   	}
> +	rte_power_exit(rte_lcore_id());

why is this being added? it doesn't seem relevant to neither the commit 
message nor the feature. if this was missing before, please add it in a 
separate patch. same applies to all other instances where 
rte_power_exit() is added.

also, your app seems to support power and non-power operation. what 
happens when rte_power_exit is called on an lcore that's not been 
initialized (i.e. the fallback to non-power mode)? does this (and other 
rte_power_exit() instances) code only get called when in power mode?

>   	/* set worker & tx threads quit flag */
>   	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
>   	quit_signal = 1;
> @@ -364,6 +366,8 @@ lcore_distributor(struct lcore_params *p)
>   	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
>   	quit_signal_work = 1;
>   
> +	rte_power_exit(rte_lcore_id());
> +
>   	rte_distributor_flush(d);
>   	/* Unblock any returns so workers can exit */
>   	rte_distributor_clear_returns(d);
> @@ -435,6 +439,7 @@ lcore_tx(struct rte_ring *in_r)
>   			}
>   		}
>   	}
> +	rte_power_exit(rte_lcore_id());
>   	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>   	return 0;
>   }
> @@ -575,9 +580,32 @@ lcore_worker(struct lcore_params *p)
>   		if (num > 0)
>   			app_stats.worker_bursts[p->worker_id][num-1]++;
>   	}
> +	rte_power_exit(rte_lcore_id());
>   	return 0;
>   }
>   
> +static int
> +init_power_library(void)
> +{
> +	int ret = 0, lcore_id;
> +	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {

RTE_LCORE_FOREACH?

> +		if (rte_lcore_is_enabled(lcore_id)) {
> +			/* init power management library */
> +			ret = rte_power_init(lcore_id);
> +			if (ret)
> +				RTE_LOG(ERR, POWER,
> +				"Library initialization failed on core %u\n",
> +				lcore_id);
> +				/*
> +				 * Return on first failure, we'll fall back
> +				 * to non-power operation
> +				 */
> +				return ret;

You'll probably want to fix indentation here, it's misleading.

> +		}
> +	}
> +	return ret;
> +}
> +
>   /* display usage */
>   static void
>   print_usage(const char *prgname)

<...>

> +		 * Here we'll pre-assign lcore ids to the rx, tx and
> +		 * distributor workloads if there's higher frequency
> +		 * on those cores e.g. if Turbo Boost is enabled.
> +		 * It's also worth mentioning that it will assign cores in a
> +		 * specific order, so that if there's less than three
> +		 * available, the higher frequency cores will go to the
> +		 * distributor first, then rx, then tx.
> +		 */
> +		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +
> +			rte_power_get_capabilities(lcore_id, &lcore_cap);
> +
> +			if (lcore_cap.turbo == 1) {
> +				priority_num++;
> +				switch (priority_num) {
> +				case 1:
> +					distr_core_id = lcore_id;
> +					printf("Distributor on priority core %d\n",

This says "priority", other instances say "preferred". Which is it? :)

> +							lcore_id);
> +					break;
> +				case 2:
> +					rx_core_id = lcore_id;
> +					printf("Rx on preferred core %d\n",
> +							lcore_id);
> +					break;
> +				case 3:
> +					tx_core_id = lcore_id;
> +					printf("Tx on preferred core %d\n",
> +							lcore_id);
> +					break;
> +				default:
> +					break;
> +				}
> +			}
> +		}
> +	}
> +
> +	/*
> +	 * If there's  any of the key workloads left without an lcore_id

Double space after "there's".

> +	 * after the higer frequency core assignment above, pre-assign
> +	 * them here.
> +	 */
>   	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> -		if (worker_id == rte_lcore_count() - 3) {
> -			printf("Starting distributor on lcore_id %d\n",
> -					lcore_id);
> -			/* distributor core */
> -			struct lcore_params *p =
> -					rte_malloc(NULL, sizeof(*p), 0);
> -			if (!p)
> -				rte_panic("malloc failure\n");
> -			*p = (struct lcore_params){worker_id, d,
> -				rx_dist_ring, dist_tx_ring, mbuf_pool};
> -			rte_eal_remote_launch(
> -				(lcore_function_t *)lcore_distributor,
> -				p, lcore_id);
> -		} else if (worker_id == rte_lcore_count() - 4) {
> -			printf("Starting tx  on worker_id %d, lcore_id %d\n",
> -					worker_id, lcore_id);
> -			/* tx core */
> -			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
> -					dist_tx_ring, lcore_id);
> -		} else if (worker_id == rte_lcore_count() - 2) {
> -			printf("Starting rx on worker_id %d, lcore_id %d\n",
> -					worker_id, lcore_id);
> -			/* rx core */
> -			struct lcore_params *p =
> -					rte_malloc(NULL, sizeof(*p), 0);
> -			if (!p)
> -				rte_panic("malloc failure\n");
> -			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
> -					dist_tx_ring, mbuf_pool};
> -			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
> -					p, lcore_id);
> +
> +		if (distr_core_id == 0) {

0 is a valid core id. You would probably want to use -1 here.

> +			distr_core_id = lcore_id;
> +			printf("Distributor on core %d\n", lcore_id);
> +		}
> +		if ((rx_core_id == 0) &&
> +				(lcore_id != distr_core_id)) {

You could just check if (lcore_id == distr_core_id || lcore_id == 
rx_core_id || lcore_id == tx_core_id) and skip the iteration entirely, 
rather than checking at every step.

> +			rx_core_id = lcore_id;
> +			printf("Rx on core %d\n", lcore_id);
> +		}
> +		if ((tx_core_id == 0) &&
> +				(lcore_id != distr_core_id) &&
> +				(lcore_id != rx_core_id)) {
> +			tx_core_id = lcore_id;
> +			printf("Tx on core %d\n", lcore_id);
> +		}
> +		counter++;
> +	}
> +
> +	printf(" tx id %d, dist id %d, rx id %d\n",
> +			tx_core_id,
> +			distr_core_id,
> +			rx_core_id);
> +
> +	/*
> +	 * Kick off all the worker threads first, avoiding the pre-assigned
> +	 * lcore_ids for tx, rx and distributor workloads.
> +	 */
> +	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +
> +		if ((lcore_id == distr_core_id) ||
> +			(lcore_id == rx_core_id) ||
> +			(lcore_id == tx_core_id)) {
> +
>   		} else {

This is a very unorthodox way of skipping an iteration :)


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v1] examples/distributor: detect high frequency cores

[Burakov, Anatoly]

On 22-Feb-19 11:45 AM, David Hunt wrote:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
> 
> This patch uses the rte_power_get_capabilities() API to query the cores
> provided in the core mask, and if any high frequency cores are found
> (e.g. Turbo Boost is enabled), we will pin the distributor workload to
> that core.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
>   examples/distributor/main.c      | 185 ++++++++++++++++++++++++-------
>   examples/distributor/meson.build |   2 +-
>   2 files changed, 149 insertions(+), 38 deletions(-)
> 
> diff --git a/examples/distributor/main.c b/examples/distributor/main.c
> index 03a05e3d9..0541c50b0 100644
> --- a/examples/distributor/main.c
> +++ b/examples/distributor/main.c
> @@ -16,6 +16,7 @@
>   #include <rte_prefetch.h>
>   #include <rte_distributor.h>
>   #include <rte_pause.h>
> +#include <rte_power.h>
>   
>   #define RX_RING_SIZE 1024
>   #define TX_RING_SIZE 1024
> @@ -281,6 +282,7 @@ lcore_rx(struct lcore_params *p)
>   		if (++port == nb_ports)
>   			port = 0;
>   	}
> +	rte_power_exit(rte_lcore_id());

why is this being added? it doesn't seem relevant to neither the commit 
message nor the feature. if this was missing before, please add it in a 
separate patch. same applies to all other instances where 
rte_power_exit() is added.

also, your app seems to support power and non-power operation. what 
happens when rte_power_exit is called on an lcore that's not been 
initialized (i.e. the fallback to non-power mode)? does this (and other 
rte_power_exit() instances) code only get called when in power mode?

>   	/* set worker & tx threads quit flag */
>   	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
>   	quit_signal = 1;
> @@ -364,6 +366,8 @@ lcore_distributor(struct lcore_params *p)
>   	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
>   	quit_signal_work = 1;
>   
> +	rte_power_exit(rte_lcore_id());
> +
>   	rte_distributor_flush(d);
>   	/* Unblock any returns so workers can exit */
>   	rte_distributor_clear_returns(d);
> @@ -435,6 +439,7 @@ lcore_tx(struct rte_ring *in_r)
>   			}
>   		}
>   	}
> +	rte_power_exit(rte_lcore_id());
>   	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>   	return 0;
>   }
> @@ -575,9 +580,32 @@ lcore_worker(struct lcore_params *p)
>   		if (num > 0)
>   			app_stats.worker_bursts[p->worker_id][num-1]++;
>   	}
> +	rte_power_exit(rte_lcore_id());
>   	return 0;
>   }
>   
> +static int
> +init_power_library(void)
> +{
> +	int ret = 0, lcore_id;
> +	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {

RTE_LCORE_FOREACH?

> +		if (rte_lcore_is_enabled(lcore_id)) {
> +			/* init power management library */
> +			ret = rte_power_init(lcore_id);
> +			if (ret)
> +				RTE_LOG(ERR, POWER,
> +				"Library initialization failed on core %u\n",
> +				lcore_id);
> +				/*
> +				 * Return on first failure, we'll fall back
> +				 * to non-power operation
> +				 */
> +				return ret;

You'll probably want to fix indentation here, it's misleading.

> +		}
> +	}
> +	return ret;
> +}
> +
>   /* display usage */
>   static void
>   print_usage(const char *prgname)

<...>

> +		 * Here we'll pre-assign lcore ids to the rx, tx and
> +		 * distributor workloads if there's higher frequency
> +		 * on those cores e.g. if Turbo Boost is enabled.
> +		 * It's also worth mentioning that it will assign cores in a
> +		 * specific order, so that if there's less than three
> +		 * available, the higher frequency cores will go to the
> +		 * distributor first, then rx, then tx.
> +		 */
> +		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +
> +			rte_power_get_capabilities(lcore_id, &lcore_cap);
> +
> +			if (lcore_cap.turbo == 1) {
> +				priority_num++;
> +				switch (priority_num) {
> +				case 1:
> +					distr_core_id = lcore_id;
> +					printf("Distributor on priority core %d\n",

This says "priority", other instances say "preferred". Which is it? :)

> +							lcore_id);
> +					break;
> +				case 2:
> +					rx_core_id = lcore_id;
> +					printf("Rx on preferred core %d\n",
> +							lcore_id);
> +					break;
> +				case 3:
> +					tx_core_id = lcore_id;
> +					printf("Tx on preferred core %d\n",
> +							lcore_id);
> +					break;
> +				default:
> +					break;
> +				}
> +			}
> +		}
> +	}
> +
> +	/*
> +	 * If there's  any of the key workloads left without an lcore_id

Double space after "there's".

> +	 * after the higer frequency core assignment above, pre-assign
> +	 * them here.
> +	 */
>   	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> -		if (worker_id == rte_lcore_count() - 3) {
> -			printf("Starting distributor on lcore_id %d\n",
> -					lcore_id);
> -			/* distributor core */
> -			struct lcore_params *p =
> -					rte_malloc(NULL, sizeof(*p), 0);
> -			if (!p)
> -				rte_panic("malloc failure\n");
> -			*p = (struct lcore_params){worker_id, d,
> -				rx_dist_ring, dist_tx_ring, mbuf_pool};
> -			rte_eal_remote_launch(
> -				(lcore_function_t *)lcore_distributor,
> -				p, lcore_id);
> -		} else if (worker_id == rte_lcore_count() - 4) {
> -			printf("Starting tx  on worker_id %d, lcore_id %d\n",
> -					worker_id, lcore_id);
> -			/* tx core */
> -			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
> -					dist_tx_ring, lcore_id);
> -		} else if (worker_id == rte_lcore_count() - 2) {
> -			printf("Starting rx on worker_id %d, lcore_id %d\n",
> -					worker_id, lcore_id);
> -			/* rx core */
> -			struct lcore_params *p =
> -					rte_malloc(NULL, sizeof(*p), 0);
> -			if (!p)
> -				rte_panic("malloc failure\n");
> -			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
> -					dist_tx_ring, mbuf_pool};
> -			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
> -					p, lcore_id);
> +
> +		if (distr_core_id == 0) {

0 is a valid core id. You would probably want to use -1 here.

> +			distr_core_id = lcore_id;
> +			printf("Distributor on core %d\n", lcore_id);
> +		}
> +		if ((rx_core_id == 0) &&
> +				(lcore_id != distr_core_id)) {

You could just check if (lcore_id == distr_core_id || lcore_id == 
rx_core_id || lcore_id == tx_core_id) and skip the iteration entirely, 
rather than checking at every step.

> +			rx_core_id = lcore_id;
> +			printf("Rx on core %d\n", lcore_id);
> +		}
> +		if ((tx_core_id == 0) &&
> +				(lcore_id != distr_core_id) &&
> +				(lcore_id != rx_core_id)) {
> +			tx_core_id = lcore_id;
> +			printf("Tx on core %d\n", lcore_id);
> +		}
> +		counter++;
> +	}
> +
> +	printf(" tx id %d, dist id %d, rx id %d\n",
> +			tx_core_id,
> +			distr_core_id,
> +			rx_core_id);
> +
> +	/*
> +	 * Kick off all the worker threads first, avoiding the pre-assigned
> +	 * lcore_ids for tx, rx and distributor workloads.
> +	 */
> +	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +
> +		if ((lcore_id == distr_core_id) ||
> +			(lcore_id == rx_core_id) ||
> +			(lcore_id == tx_core_id)) {
> +
>   		} else {

This is a very unorthodox way of skipping an iteration :)


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v1] examples/distributor: detect high frequency cores

[Hunt, David]

Hi Anatoly,

On 27/3/2019 1:58 PM, Burakov, Anatoly wrote:
> On 22-Feb-19 11:45 AM, David Hunt wrote:
>> The distributor application is bottlenecked by the distributor core,
>> so if we can give more frequency to this core, then the overall
>> performance of the application may increase.
>>
>> This patch uses the rte_power_get_capabilities() API to query the cores
>> provided in the core mask, and if any high frequency cores are found
>> (e.g. Turbo Boost is enabled), we will pin the distributor workload to
>> that core.
>>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>>   examples/distributor/main.c      | 185 ++++++++++++++++++++++++-------
>>   examples/distributor/meson.build |   2 +-
>>   2 files changed, 149 insertions(+), 38 deletions(-)
>>
>> diff --git a/examples/distributor/main.c b/examples/distributor/main.c
>> index 03a05e3d9..0541c50b0 100644
>> --- a/examples/distributor/main.c
>> +++ b/examples/distributor/main.c
>> @@ -16,6 +16,7 @@
>>   #include <rte_prefetch.h>
>>   #include <rte_distributor.h>
>>   #include <rte_pause.h>
>> +#include <rte_power.h>
>>     #define RX_RING_SIZE 1024
>>   #define TX_RING_SIZE 1024
>> @@ -281,6 +282,7 @@ lcore_rx(struct lcore_params *p)
>>           if (++port == nb_ports)
>>               port = 0;
>>       }
>> +    rte_power_exit(rte_lcore_id());
>
> why is this being added? it doesn't seem relevant to neither the 
> commit message nor the feature. if this was missing before, please add 
> it in a separate patch. same applies to all other instances where 
> rte_power_exit() is added.


I'll make "power_lib_initialised" a global, and check that's set before 
calling the rte_power_exit()


>
> also, your app seems to support power and non-power operation. what 
> happens when rte_power_exit is called on an lcore that's not been 
> initialized (i.e. the fallback to non-power mode)? does this (and 
> other rte_power_exit() instances) code only get called when in power 
> mode?

No issue with calling it on a non-power-enabled core, but I'll make it 
conditional anyway.


>
>>       /* set worker & tx threads quit flag */
>>       printf("\nCore %u exiting rx task.\n", rte_lcore_id());
>>       quit_signal = 1;
>> @@ -364,6 +366,8 @@ lcore_distributor(struct lcore_params *p)
>>       printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
>>       quit_signal_work = 1;
>>   +    rte_power_exit(rte_lcore_id());
>> +
>>       rte_distributor_flush(d);
>>       /* Unblock any returns so workers can exit */
>>       rte_distributor_clear_returns(d);
>> @@ -435,6 +439,7 @@ lcore_tx(struct rte_ring *in_r)
>>               }
>>           }
>>       }
>> +    rte_power_exit(rte_lcore_id());
>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>       return 0;
>>   }
>> @@ -575,9 +580,32 @@ lcore_worker(struct lcore_params *p)
>>           if (num > 0)
>>               app_stats.worker_bursts[p->worker_id][num-1]++;
>>       }
>> +    rte_power_exit(rte_lcore_id());
>>       return 0;
>>   }
>>   +static int
>> +init_power_library(void)
>> +{
>> +    int ret = 0, lcore_id;
>> +    for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>
> RTE_LCORE_FOREACH?


Done in v2


>
>> +        if (rte_lcore_is_enabled(lcore_id)) {
>> +            /* init power management library */
>> +            ret = rte_power_init(lcore_id);
>> +            if (ret)
>> +                RTE_LOG(ERR, POWER,
>> +                "Library initialization failed on core %u\n",
>> +                lcore_id);
>> +                /*
>> +                 * Return on first failure, we'll fall back
>> +                 * to non-power operation
>> +                 */
>> +                return ret;
>
> You'll probably want to fix indentation here, it's misleading.


Fixed in v2. I also added braces around the RTE_LOG and return(). :)


>
>> +        }
>> +    }
>> +    return ret;
>> +}
>> +
>>   /* display usage */
>>   static void
>>   print_usage(const char *prgname)
>
> <...>
>
>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>> +         * distributor workloads if there's higher frequency
>> +         * on those cores e.g. if Turbo Boost is enabled.
>> +         * It's also worth mentioning that it will assign cores in a
>> +         * specific order, so that if there's less than three
>> +         * available, the higher frequency cores will go to the
>> +         * distributor first, then rx, then tx.
>> +         */
>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +
>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>> +
>> +            if (lcore_cap.turbo == 1) {
>> +                priority_num++;
>> +                switch (priority_num) {
>> +                case 1:
>> +                    distr_core_id = lcore_id;
>> +                    printf("Distributor on priority core %d\n",
>
> This says "priority", other instances say "preferred". Which is it? :)


Will change to priority.


>
>> +                            lcore_id);
>> +                    break;
>> +                case 2:
>> +                    rx_core_id = lcore_id;
>> +                    printf("Rx on preferred core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                case 3:
>> +                    tx_core_id = lcore_id;
>> +                    printf("Tx on preferred core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                default:
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    /*
>> +     * If there's  any of the key workloads left without an lcore_id
>
> Double space after "there's".


Fixed in v2


>
>> +     * after the higer frequency core assignment above, pre-assign
>> +     * them here.
>> +     */
>>       RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> -        if (worker_id == rte_lcore_count() - 3) {
>> -            printf("Starting distributor on lcore_id %d\n",
>> -                    lcore_id);
>> -            /* distributor core */
>> -            struct lcore_params *p =
>> -                    rte_malloc(NULL, sizeof(*p), 0);
>> -            if (!p)
>> -                rte_panic("malloc failure\n");
>> -            *p = (struct lcore_params){worker_id, d,
>> -                rx_dist_ring, dist_tx_ring, mbuf_pool};
>> -            rte_eal_remote_launch(
>> -                (lcore_function_t *)lcore_distributor,
>> -                p, lcore_id);
>> -        } else if (worker_id == rte_lcore_count() - 4) {
>> -            printf("Starting tx  on worker_id %d, lcore_id %d\n",
>> -                    worker_id, lcore_id);
>> -            /* tx core */
>> -            rte_eal_remote_launch((lcore_function_t *)lcore_tx,
>> -                    dist_tx_ring, lcore_id);
>> -        } else if (worker_id == rte_lcore_count() - 2) {
>> -            printf("Starting rx on worker_id %d, lcore_id %d\n",
>> -                    worker_id, lcore_id);
>> -            /* rx core */
>> -            struct lcore_params *p =
>> -                    rte_malloc(NULL, sizeof(*p), 0);
>> -            if (!p)
>> -                rte_panic("malloc failure\n");
>> -            *p = (struct lcore_params){worker_id, d, rx_dist_ring,
>> -                    dist_tx_ring, mbuf_pool};
>> -            rte_eal_remote_launch((lcore_function_t *)lcore_rx,
>> -                    p, lcore_id);
>> +
>> +        if (distr_core_id == 0) {
>
> 0 is a valid core id. You would probably want to use -1 here.


I've changed to int using -1 for invalid cores across the app.


>
>> +            distr_core_id = lcore_id;
>> +            printf("Distributor on core %d\n", lcore_id);
>> +        }
>> +        if ((rx_core_id == 0) &&
>> +                (lcore_id != distr_core_id)) {
>
> You could just check if (lcore_id == distr_core_id || lcore_id == 
> rx_core_id || lcore_id == tx_core_id) and skip the iteration entirely, 
> rather than checking at every step.


Done in v2.


>
>> +            rx_core_id = lcore_id;
>> +            printf("Rx on core %d\n", lcore_id);
>> +        }
>> +        if ((tx_core_id == 0) &&
>> +                (lcore_id != distr_core_id) &&
>> +                (lcore_id != rx_core_id)) {
>> +            tx_core_id = lcore_id;
>> +            printf("Tx on core %d\n", lcore_id);
>> +        }
>> +        counter++;
>> +    }
>> +
>> +    printf(" tx id %d, dist id %d, rx id %d\n",
>> +            tx_core_id,
>> +            distr_core_id,
>> +            rx_core_id);
>> +
>> +    /*
>> +     * Kick off all the worker threads first, avoiding the pre-assigned
>> +     * lcore_ids for tx, rx and distributor workloads.
>> +     */
>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +
>> +        if ((lcore_id == distr_core_id) ||
>> +            (lcore_id == rx_core_id) ||
>> +            (lcore_id == tx_core_id)) {
>> +
>>           } else {
>
> This is a very unorthodox way of skipping an iteration :)
>

Fixed in v2 to be like you're previous suggestion above, using continue.


Thanks for the review, v2 coming in a few hours.

Rgds,

Dave.

Re: [dpdk-dev] [PATCH v1] examples/distributor: detect high frequency cores

[Hunt, David]

Hi Anatoly,

On 27/3/2019 1:58 PM, Burakov, Anatoly wrote:
> On 22-Feb-19 11:45 AM, David Hunt wrote:
>> The distributor application is bottlenecked by the distributor core,
>> so if we can give more frequency to this core, then the overall
>> performance of the application may increase.
>>
>> This patch uses the rte_power_get_capabilities() API to query the cores
>> provided in the core mask, and if any high frequency cores are found
>> (e.g. Turbo Boost is enabled), we will pin the distributor workload to
>> that core.
>>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>>   examples/distributor/main.c      | 185 ++++++++++++++++++++++++-------
>>   examples/distributor/meson.build |   2 +-
>>   2 files changed, 149 insertions(+), 38 deletions(-)
>>
>> diff --git a/examples/distributor/main.c b/examples/distributor/main.c
>> index 03a05e3d9..0541c50b0 100644
>> --- a/examples/distributor/main.c
>> +++ b/examples/distributor/main.c
>> @@ -16,6 +16,7 @@
>>   #include <rte_prefetch.h>
>>   #include <rte_distributor.h>
>>   #include <rte_pause.h>
>> +#include <rte_power.h>
>>     #define RX_RING_SIZE 1024
>>   #define TX_RING_SIZE 1024
>> @@ -281,6 +282,7 @@ lcore_rx(struct lcore_params *p)
>>           if (++port == nb_ports)
>>               port = 0;
>>       }
>> +    rte_power_exit(rte_lcore_id());
>
> why is this being added? it doesn't seem relevant to neither the 
> commit message nor the feature. if this was missing before, please add 
> it in a separate patch. same applies to all other instances where 
> rte_power_exit() is added.


I'll make "power_lib_initialised" a global, and check that's set before 
calling the rte_power_exit()


>
> also, your app seems to support power and non-power operation. what 
> happens when rte_power_exit is called on an lcore that's not been 
> initialized (i.e. the fallback to non-power mode)? does this (and 
> other rte_power_exit() instances) code only get called when in power 
> mode?

No issue with calling it on a non-power-enabled core, but I'll make it 
conditional anyway.


>
>>       /* set worker & tx threads quit flag */
>>       printf("\nCore %u exiting rx task.\n", rte_lcore_id());
>>       quit_signal = 1;
>> @@ -364,6 +366,8 @@ lcore_distributor(struct lcore_params *p)
>>       printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
>>       quit_signal_work = 1;
>>   +    rte_power_exit(rte_lcore_id());
>> +
>>       rte_distributor_flush(d);
>>       /* Unblock any returns so workers can exit */
>>       rte_distributor_clear_returns(d);
>> @@ -435,6 +439,7 @@ lcore_tx(struct rte_ring *in_r)
>>               }
>>           }
>>       }
>> +    rte_power_exit(rte_lcore_id());
>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>       return 0;
>>   }
>> @@ -575,9 +580,32 @@ lcore_worker(struct lcore_params *p)
>>           if (num > 0)
>>               app_stats.worker_bursts[p->worker_id][num-1]++;
>>       }
>> +    rte_power_exit(rte_lcore_id());
>>       return 0;
>>   }
>>   +static int
>> +init_power_library(void)
>> +{
>> +    int ret = 0, lcore_id;
>> +    for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>
> RTE_LCORE_FOREACH?


Done in v2


>
>> +        if (rte_lcore_is_enabled(lcore_id)) {
>> +            /* init power management library */
>> +            ret = rte_power_init(lcore_id);
>> +            if (ret)
>> +                RTE_LOG(ERR, POWER,
>> +                "Library initialization failed on core %u\n",
>> +                lcore_id);
>> +                /*
>> +                 * Return on first failure, we'll fall back
>> +                 * to non-power operation
>> +                 */
>> +                return ret;
>
> You'll probably want to fix indentation here, it's misleading.


Fixed in v2. I also added braces around the RTE_LOG and return(). :)


>
>> +        }
>> +    }
>> +    return ret;
>> +}
>> +
>>   /* display usage */
>>   static void
>>   print_usage(const char *prgname)
>
> <...>
>
>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>> +         * distributor workloads if there's higher frequency
>> +         * on those cores e.g. if Turbo Boost is enabled.
>> +         * It's also worth mentioning that it will assign cores in a
>> +         * specific order, so that if there's less than three
>> +         * available, the higher frequency cores will go to the
>> +         * distributor first, then rx, then tx.
>> +         */
>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +
>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>> +
>> +            if (lcore_cap.turbo == 1) {
>> +                priority_num++;
>> +                switch (priority_num) {
>> +                case 1:
>> +                    distr_core_id = lcore_id;
>> +                    printf("Distributor on priority core %d\n",
>
> This says "priority", other instances say "preferred". Which is it? :)


Will change to priority.


>
>> +                            lcore_id);
>> +                    break;
>> +                case 2:
>> +                    rx_core_id = lcore_id;
>> +                    printf("Rx on preferred core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                case 3:
>> +                    tx_core_id = lcore_id;
>> +                    printf("Tx on preferred core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                default:
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    /*
>> +     * If there's  any of the key workloads left without an lcore_id
>
> Double space after "there's".


Fixed in v2


>
>> +     * after the higer frequency core assignment above, pre-assign
>> +     * them here.
>> +     */
>>       RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> -        if (worker_id == rte_lcore_count() - 3) {
>> -            printf("Starting distributor on lcore_id %d\n",
>> -                    lcore_id);
>> -            /* distributor core */
>> -            struct lcore_params *p =
>> -                    rte_malloc(NULL, sizeof(*p), 0);
>> -            if (!p)
>> -                rte_panic("malloc failure\n");
>> -            *p = (struct lcore_params){worker_id, d,
>> -                rx_dist_ring, dist_tx_ring, mbuf_pool};
>> -            rte_eal_remote_launch(
>> -                (lcore_function_t *)lcore_distributor,
>> -                p, lcore_id);
>> -        } else if (worker_id == rte_lcore_count() - 4) {
>> -            printf("Starting tx  on worker_id %d, lcore_id %d\n",
>> -                    worker_id, lcore_id);
>> -            /* tx core */
>> -            rte_eal_remote_launch((lcore_function_t *)lcore_tx,
>> -                    dist_tx_ring, lcore_id);
>> -        } else if (worker_id == rte_lcore_count() - 2) {
>> -            printf("Starting rx on worker_id %d, lcore_id %d\n",
>> -                    worker_id, lcore_id);
>> -            /* rx core */
>> -            struct lcore_params *p =
>> -                    rte_malloc(NULL, sizeof(*p), 0);
>> -            if (!p)
>> -                rte_panic("malloc failure\n");
>> -            *p = (struct lcore_params){worker_id, d, rx_dist_ring,
>> -                    dist_tx_ring, mbuf_pool};
>> -            rte_eal_remote_launch((lcore_function_t *)lcore_rx,
>> -                    p, lcore_id);
>> +
>> +        if (distr_core_id == 0) {
>
> 0 is a valid core id. You would probably want to use -1 here.


I've changed to int using -1 for invalid cores across the app.


>
>> +            distr_core_id = lcore_id;
>> +            printf("Distributor on core %d\n", lcore_id);
>> +        }
>> +        if ((rx_core_id == 0) &&
>> +                (lcore_id != distr_core_id)) {
>
> You could just check if (lcore_id == distr_core_id || lcore_id == 
> rx_core_id || lcore_id == tx_core_id) and skip the iteration entirely, 
> rather than checking at every step.


Done in v2.


>
>> +            rx_core_id = lcore_id;
>> +            printf("Rx on core %d\n", lcore_id);
>> +        }
>> +        if ((tx_core_id == 0) &&
>> +                (lcore_id != distr_core_id) &&
>> +                (lcore_id != rx_core_id)) {
>> +            tx_core_id = lcore_id;
>> +            printf("Tx on core %d\n", lcore_id);
>> +        }
>> +        counter++;
>> +    }
>> +
>> +    printf(" tx id %d, dist id %d, rx id %d\n",
>> +            tx_core_id,
>> +            distr_core_id,
>> +            rx_core_id);
>> +
>> +    /*
>> +     * Kick off all the worker threads first, avoiding the pre-assigned
>> +     * lcore_ids for tx, rx and distributor workloads.
>> +     */
>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +
>> +        if ((lcore_id == distr_core_id) ||
>> +            (lcore_id == rx_core_id) ||
>> +            (lcore_id == tx_core_id)) {
>> +
>>           } else {
>
> This is a very unorthodox way of skipping an iteration :)
>

Fixed in v2 to be like you're previous suggestion above, using continue.


Thanks for the review, v2 coming in a few hours.

Rgds,

Dave.

[dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
---
 examples/distributor/main.c      | 204 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 159 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..1f8bcd672 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (rte_lcore_is_enabled(lcore_id)) {
+			/* init power management library */
+			ret = rte_power_init(lcore_id);
+			if (ret) {
+				RTE_LOG(ERR, POWER,
+					"Library initialization failed on core %u\n",
+					lcore_id);
+				/*
+				 * Return on first failure, we'll fall back
+				 * to non-power operation
+				 */
+				return ret;
+			}
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +690,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0, priority_num = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +722,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +780,124 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.turbo == 1) {
+				priority_num++;
+				switch (priority_num) {
+				case 1:
+					distr_core_id = lcore_id;
+					printf("Distributor on priority core %d\n",
+							lcore_id);
+					break;
+				case 2:
+					rx_core_id = lcore_id;
+					printf("Rx on priority core %d\n",
+							lcore_id);
+					break;
+				case 3:
+					tx_core_id = lcore_id;
+					printf("Tx on priority core %d\n",
+							lcore_id);
+					break;
+				default:
+					break;
+				}
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
+		}
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
+		}
+	}
+
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
-					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
 				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
-		}
-		worker_id++;
 	}
 
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +914,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

[dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
---
 examples/distributor/main.c      | 204 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 159 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..1f8bcd672 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (rte_lcore_is_enabled(lcore_id)) {
+			/* init power management library */
+			ret = rte_power_init(lcore_id);
+			if (ret) {
+				RTE_LOG(ERR, POWER,
+					"Library initialization failed on core %u\n",
+					lcore_id);
+				/*
+				 * Return on first failure, we'll fall back
+				 * to non-power operation
+				 */
+				return ret;
+			}
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +690,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0, priority_num = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +722,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +780,124 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.turbo == 1) {
+				priority_num++;
+				switch (priority_num) {
+				case 1:
+					distr_core_id = lcore_id;
+					printf("Distributor on priority core %d\n",
+							lcore_id);
+					break;
+				case 2:
+					rx_core_id = lcore_id;
+					printf("Rx on priority core %d\n",
+							lcore_id);
+					break;
+				case 3:
+					tx_core_id = lcore_id;
+					printf("Tx on priority core %d\n",
+							lcore_id);
+					break;
+				default:
+					break;
+				}
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
+		}
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
+		}
+	}
+
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
-					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
 				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
-		}
-		worker_id++;
 	}
 
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +914,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Burakov, Anatoly]

On 28-Mar-19 1:13 PM, David Hunt wrote:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
> 
> This patch uses the rte_power_get_capabilities() API to query the
> cores provided in the core mask, and if any high frequency cores are
> found (e.g. Turbo Boost is enabled), we will pin the distributor
> workload to that core.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---

<...>

> +	if (power_lib_initialised)
> +		rte_power_exit(rte_lcore_id());
>   	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>   	return 0;
>   }
> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>   		if (num > 0)
>   			app_stats.worker_bursts[p->worker_id][num-1]++;
>   	}
> +	if (power_lib_initialised)
> +		rte_power_exit(rte_lcore_id());
> +	rte_free(p);
>   	return 0;
>   }
>   
> +static int
> +init_power_library(void)
> +{
> +	int ret = 0, lcore_id;
> +	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +		if (rte_lcore_is_enabled(lcore_id)) {

Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
checks if the lcore is enabled.

<...>

>   
> +	if (power_lib_initialised) {
> +		/*
> +		 * Here we'll pre-assign lcore ids to the rx, tx and
> +		 * distributor workloads if there's higher frequency
> +		 * on those cores e.g. if Turbo Boost is enabled.
> +		 * It's also worth mentioning that it will assign cores in a
> +		 * specific order, so that if there's less than three
> +		 * available, the higher frequency cores will go to the
> +		 * distributor first, then rx, then tx.
> +		 */
> +		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +
> +			rte_power_get_capabilities(lcore_id, &lcore_cap);
> +
> +			if (lcore_cap.turbo == 1) {
> +				priority_num++;
> +				switch (priority_num) {
> +				case 1:
> +					distr_core_id = lcore_id;
> +					printf("Distributor on priority core %d\n",
> +							lcore_id);
> +					break;
> +				case 2:
> +					rx_core_id = lcore_id;
> +					printf("Rx on priority core %d\n",
> +							lcore_id);
> +					break;
> +				case 3:
> +					tx_core_id = lcore_id;
> +					printf("Tx on priority core %d\n",
> +							lcore_id);
> +					break;
> +				default:
> +					break;
> +				}

This seems to be doing the same thing as right below (assigning lcore 
id's in order), yet in one case you use a switch, and in the other you 
use a simple loop. I don't see priority_num used anywhere else, so you 
might as well simplify this loop to be similar to what you have below, 
with "skip-if-not-turbo, if not assigned, assign-and-continue" type flow.

Once that is fixed,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Burakov, Anatoly]

On 28-Mar-19 1:13 PM, David Hunt wrote:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
> 
> This patch uses the rte_power_get_capabilities() API to query the
> cores provided in the core mask, and if any high frequency cores are
> found (e.g. Turbo Boost is enabled), we will pin the distributor
> workload to that core.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---

<...>

> +	if (power_lib_initialised)
> +		rte_power_exit(rte_lcore_id());
>   	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>   	return 0;
>   }
> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>   		if (num > 0)
>   			app_stats.worker_bursts[p->worker_id][num-1]++;
>   	}
> +	if (power_lib_initialised)
> +		rte_power_exit(rte_lcore_id());
> +	rte_free(p);
>   	return 0;
>   }
>   
> +static int
> +init_power_library(void)
> +{
> +	int ret = 0, lcore_id;
> +	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +		if (rte_lcore_is_enabled(lcore_id)) {

Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
checks if the lcore is enabled.

<...>

>   
> +	if (power_lib_initialised) {
> +		/*
> +		 * Here we'll pre-assign lcore ids to the rx, tx and
> +		 * distributor workloads if there's higher frequency
> +		 * on those cores e.g. if Turbo Boost is enabled.
> +		 * It's also worth mentioning that it will assign cores in a
> +		 * specific order, so that if there's less than three
> +		 * available, the higher frequency cores will go to the
> +		 * distributor first, then rx, then tx.
> +		 */
> +		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> +
> +			rte_power_get_capabilities(lcore_id, &lcore_cap);
> +
> +			if (lcore_cap.turbo == 1) {
> +				priority_num++;
> +				switch (priority_num) {
> +				case 1:
> +					distr_core_id = lcore_id;
> +					printf("Distributor on priority core %d\n",
> +							lcore_id);
> +					break;
> +				case 2:
> +					rx_core_id = lcore_id;
> +					printf("Rx on priority core %d\n",
> +							lcore_id);
> +					break;
> +				case 3:
> +					tx_core_id = lcore_id;
> +					printf("Tx on priority core %d\n",
> +							lcore_id);
> +					break;
> +				default:
> +					break;
> +				}

This seems to be doing the same thing as right below (assigning lcore 
id's in order), yet in one case you use a switch, and in the other you 
use a simple loop. I don't see priority_num used anywhere else, so you 
might as well simplify this loop to be similar to what you have below, 
with "skip-if-not-turbo, if not assigned, assign-and-continue" type flow.

Once that is fixed,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Hunt, David]

On 28/3/2019 1:58 PM, Burakov, Anatoly wrote:
> On 28-Mar-19 1:13 PM, David Hunt wrote:
>> The distributor application is bottlenecked by the distributor core,
>> so if we can give more frequency to this core, then the overall
>> performance of the application may increase.
>>
>> This patch uses the rte_power_get_capabilities() API to query the
>> cores provided in the core mask, and if any high frequency cores are
>> found (e.g. Turbo Boost is enabled), we will pin the distributor
>> workload to that core.
>>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>
> <...>
>
>> +    if (power_lib_initialised)
>> +        rte_power_exit(rte_lcore_id());
>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>       return 0;
>>   }
>> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>>           if (num > 0)
>>               app_stats.worker_bursts[p->worker_id][num-1]++;
>>       }
>> +    if (power_lib_initialised)
>> +        rte_power_exit(rte_lcore_id());
>> +    rte_free(p);
>>       return 0;
>>   }
>>   +static int
>> +init_power_library(void)
>> +{
>> +    int ret = 0, lcore_id;
>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +        if (rte_lcore_is_enabled(lcore_id)) {
>
> Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
> checks if the lcore is enabled.


You're correct, I'll fix in next version.


>
> <...>
>
>>   +    if (power_lib_initialised) {
>> +        /*
>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>> +         * distributor workloads if there's higher frequency
>> +         * on those cores e.g. if Turbo Boost is enabled.
>> +         * It's also worth mentioning that it will assign cores in a
>> +         * specific order, so that if there's less than three
>> +         * available, the higher frequency cores will go to the
>> +         * distributor first, then rx, then tx.
>> +         */
>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +
>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>> +
>> +            if (lcore_cap.turbo == 1) {
>> +                priority_num++;
>> +                switch (priority_num) {
>> +                case 1:
>> +                    distr_core_id = lcore_id;
>> +                    printf("Distributor on priority core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                case 2:
>> +                    rx_core_id = lcore_id;
>> +                    printf("Rx on priority core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                case 3:
>> +                    tx_core_id = lcore_id;
>> +                    printf("Tx on priority core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                default:
>> +                    break;
>> +                }
>
> This seems to be doing the same thing as right below (assigning lcore 
> id's in order), yet in one case you use a switch, and in the other you 
> use a simple loop. I don't see priority_num used anywhere else, so you 
> might as well simplify this loop to be similar to what you have below, 
> with "skip-if-not-turbo, if not assigned, assign-and-continue" type flow.


There doing different things. The loop with the switch is looking for up 
to three priority cores, and storing those choices in distr_core_id, 
tx_core_id and rx_core_id. This is because we don't know which are the 
priority cores ahead of time. priority_num is used in the switch 
statement, and when it finds a priority core, it increments, so we know 
which variable to assign with the next available priority core. Imagine 
we have turbo enabled on cores 2,4 and 6. That's what I'm trying to 
solve here.

Then, when we get to the next loop, we're just assigning the 
non-priority cores if the three key workloads have not already been 
assigned a core, hence the simple loop, using the remaining cores.

I looked at simplifying the flow, but as far as I can see, I need two 
stages, a 'discovery' for the priority cores first, then whatever is 
left can be done in a normal loop.

Does that make sense, or my I missing an obvious refactor opportunity?


>
> Once that is fixed,
>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Hunt, David]

On 28/3/2019 1:58 PM, Burakov, Anatoly wrote:
> On 28-Mar-19 1:13 PM, David Hunt wrote:
>> The distributor application is bottlenecked by the distributor core,
>> so if we can give more frequency to this core, then the overall
>> performance of the application may increase.
>>
>> This patch uses the rte_power_get_capabilities() API to query the
>> cores provided in the core mask, and if any high frequency cores are
>> found (e.g. Turbo Boost is enabled), we will pin the distributor
>> workload to that core.
>>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>
> <...>
>
>> +    if (power_lib_initialised)
>> +        rte_power_exit(rte_lcore_id());
>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>       return 0;
>>   }
>> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>>           if (num > 0)
>>               app_stats.worker_bursts[p->worker_id][num-1]++;
>>       }
>> +    if (power_lib_initialised)
>> +        rte_power_exit(rte_lcore_id());
>> +    rte_free(p);
>>       return 0;
>>   }
>>   +static int
>> +init_power_library(void)
>> +{
>> +    int ret = 0, lcore_id;
>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +        if (rte_lcore_is_enabled(lcore_id)) {
>
> Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
> checks if the lcore is enabled.


You're correct, I'll fix in next version.


>
> <...>
>
>>   +    if (power_lib_initialised) {
>> +        /*
>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>> +         * distributor workloads if there's higher frequency
>> +         * on those cores e.g. if Turbo Boost is enabled.
>> +         * It's also worth mentioning that it will assign cores in a
>> +         * specific order, so that if there's less than three
>> +         * available, the higher frequency cores will go to the
>> +         * distributor first, then rx, then tx.
>> +         */
>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> +
>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>> +
>> +            if (lcore_cap.turbo == 1) {
>> +                priority_num++;
>> +                switch (priority_num) {
>> +                case 1:
>> +                    distr_core_id = lcore_id;
>> +                    printf("Distributor on priority core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                case 2:
>> +                    rx_core_id = lcore_id;
>> +                    printf("Rx on priority core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                case 3:
>> +                    tx_core_id = lcore_id;
>> +                    printf("Tx on priority core %d\n",
>> +                            lcore_id);
>> +                    break;
>> +                default:
>> +                    break;
>> +                }
>
> This seems to be doing the same thing as right below (assigning lcore 
> id's in order), yet in one case you use a switch, and in the other you 
> use a simple loop. I don't see priority_num used anywhere else, so you 
> might as well simplify this loop to be similar to what you have below, 
> with "skip-if-not-turbo, if not assigned, assign-and-continue" type flow.


There doing different things. The loop with the switch is looking for up 
to three priority cores, and storing those choices in distr_core_id, 
tx_core_id and rx_core_id. This is because we don't know which are the 
priority cores ahead of time. priority_num is used in the switch 
statement, and when it finds a priority core, it increments, so we know 
which variable to assign with the next available priority core. Imagine 
we have turbo enabled on cores 2,4 and 6. That's what I'm trying to 
solve here.

Then, when we get to the next loop, we're just assigning the 
non-priority cores if the three key workloads have not already been 
assigned a core, hence the simple loop, using the remaining cores.

I looked at simplifying the flow, but as far as I can see, I need two 
stages, a 'discovery' for the priority cores first, then whatever is 
left can be done in a normal loop.

Does that make sense, or my I missing an obvious refactor opportunity?


>
> Once that is fixed,
>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Burakov, Anatoly]

On 28-Mar-19 2:42 PM, Hunt, David wrote:
> 
> On 28/3/2019 1:58 PM, Burakov, Anatoly wrote:
>> On 28-Mar-19 1:13 PM, David Hunt wrote:
>>> The distributor application is bottlenecked by the distributor core,
>>> so if we can give more frequency to this core, then the overall
>>> performance of the application may increase.
>>>
>>> This patch uses the rte_power_get_capabilities() API to query the
>>> cores provided in the core mask, and if any high frequency cores are
>>> found (e.g. Turbo Boost is enabled), we will pin the distributor
>>> workload to that core.
>>>
>>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>> ---
>>
>> <...>
>>
>>> +    if (power_lib_initialised)
>>> +        rte_power_exit(rte_lcore_id());
>>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>>       return 0;
>>>   }
>>> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>>>           if (num > 0)
>>>               app_stats.worker_bursts[p->worker_id][num-1]++;
>>>       }
>>> +    if (power_lib_initialised)
>>> +        rte_power_exit(rte_lcore_id());
>>> +    rte_free(p);
>>>       return 0;
>>>   }
>>>   +static int
>>> +init_power_library(void)
>>> +{
>>> +    int ret = 0, lcore_id;
>>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>> +        if (rte_lcore_is_enabled(lcore_id)) {
>>
>> Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
>> checks if the lcore is enabled.
> 
> 
> You're correct, I'll fix in next version.
> 
> 
>>
>> <...>
>>
>>>   +    if (power_lib_initialised) {
>>> +        /*
>>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>>> +         * distributor workloads if there's higher frequency
>>> +         * on those cores e.g. if Turbo Boost is enabled.
>>> +         * It's also worth mentioning that it will assign cores in a
>>> +         * specific order, so that if there's less than three
>>> +         * available, the higher frequency cores will go to the
>>> +         * distributor first, then rx, then tx.
>>> +         */
>>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>> +
>>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>>> +
>>> +            if (lcore_cap.turbo == 1) {
>>> +                priority_num++;
>>> +                switch (priority_num) {
>>> +                case 1:
>>> +                    distr_core_id = lcore_id;
>>> +                    printf("Distributor on priority core %d\n",
>>> +                            lcore_id);
>>> +                    break;
>>> +                case 2:
>>> +                    rx_core_id = lcore_id;
>>> +                    printf("Rx on priority core %d\n",
>>> +                            lcore_id);
>>> +                    break;
>>> +                case 3:
>>> +                    tx_core_id = lcore_id;
>>> +                    printf("Tx on priority core %d\n",
>>> +                            lcore_id);
>>> +                    break;
>>> +                default:
>>> +                    break;
>>> +                }
>>
>> This seems to be doing the same thing as right below (assigning lcore 
>> id's in order), yet in one case you use a switch, and in the other you 
>> use a simple loop. I don't see priority_num used anywhere else, so you 
>> might as well simplify this loop to be similar to what you have below, 
>> with "skip-if-not-turbo, if not assigned, assign-and-continue" type flow.
> 
> 
> There doing different things. The loop with the switch is looking for up 
> to three priority cores, and storing those choices in distr_core_id, 
> tx_core_id and rx_core_id. This is because we don't know which are the 
> priority cores ahead of time. priority_num is used in the switch 
> statement, and when it finds a priority core, it increments, so we know 
> which variable to assign with the next available priority core. Imagine 
> we have turbo enabled on cores 2,4 and 6. That's what I'm trying to 
> solve here.
> 
> Then, when we get to the next loop, we're just assigning the 
> non-priority cores if the three key workloads have not already been 
> assigned a core, hence the simple loop, using the remaining cores.
> 
> I looked at simplifying the flow, but as far as I can see, I need two 
> stages, a 'discovery' for the priority cores first, then whatever is 
> left can be done in a normal loop.
> 
> Does that make sense, or my I missing an obvious refactor opportunity?

I don't see how this is different from what you're doing below.

You are looping over cores, checking if it's a priority core, and 
assigning any priority cores found to distributor, Rx and Tx cores, in 
that order.

Below, you're looping over cores, checking if the core is already 
assigned, and assigning these cores to distributor, Rx and Tx cores, in 
that order.

So, the only tangible difference between the two is 1) the check for 
whether the cores are already assigned (you don't need that because 
these cores *cannot* be attached - you haven't looped over them yet!), 
and 2) check for whether the core is priority.

Just to clarify: i'm not saying merge the two loops, that can't work :) 
I'm saying, drop the switch and rewrite it like this:

for (cores) {
	if (core not priority)
		continue;
	if (dist_core not assigned) {
		assign dist core;
		continue;
	}
	if (rx core not assigned) {
		assign rx core;
		continue;
	}
	if (tx core not assigned) {
		assign tx core;
		continue;
	}
}

The functionality would be equivalent to the current switch method, but 
the code would be much clearer :)

> 
> 
>>
>> Once that is fixed,
>>
>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Burakov, Anatoly]

On 28-Mar-19 2:42 PM, Hunt, David wrote:
> 
> On 28/3/2019 1:58 PM, Burakov, Anatoly wrote:
>> On 28-Mar-19 1:13 PM, David Hunt wrote:
>>> The distributor application is bottlenecked by the distributor core,
>>> so if we can give more frequency to this core, then the overall
>>> performance of the application may increase.
>>>
>>> This patch uses the rte_power_get_capabilities() API to query the
>>> cores provided in the core mask, and if any high frequency cores are
>>> found (e.g. Turbo Boost is enabled), we will pin the distributor
>>> workload to that core.
>>>
>>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>> ---
>>
>> <...>
>>
>>> +    if (power_lib_initialised)
>>> +        rte_power_exit(rte_lcore_id());
>>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>>       return 0;
>>>   }
>>> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>>>           if (num > 0)
>>>               app_stats.worker_bursts[p->worker_id][num-1]++;
>>>       }
>>> +    if (power_lib_initialised)
>>> +        rte_power_exit(rte_lcore_id());
>>> +    rte_free(p);
>>>       return 0;
>>>   }
>>>   +static int
>>> +init_power_library(void)
>>> +{
>>> +    int ret = 0, lcore_id;
>>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>> +        if (rte_lcore_is_enabled(lcore_id)) {
>>
>> Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
>> checks if the lcore is enabled.
> 
> 
> You're correct, I'll fix in next version.
> 
> 
>>
>> <...>
>>
>>>   +    if (power_lib_initialised) {
>>> +        /*
>>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>>> +         * distributor workloads if there's higher frequency
>>> +         * on those cores e.g. if Turbo Boost is enabled.
>>> +         * It's also worth mentioning that it will assign cores in a
>>> +         * specific order, so that if there's less than three
>>> +         * available, the higher frequency cores will go to the
>>> +         * distributor first, then rx, then tx.
>>> +         */
>>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>> +
>>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>>> +
>>> +            if (lcore_cap.turbo == 1) {
>>> +                priority_num++;
>>> +                switch (priority_num) {
>>> +                case 1:
>>> +                    distr_core_id = lcore_id;
>>> +                    printf("Distributor on priority core %d\n",
>>> +                            lcore_id);
>>> +                    break;
>>> +                case 2:
>>> +                    rx_core_id = lcore_id;
>>> +                    printf("Rx on priority core %d\n",
>>> +                            lcore_id);
>>> +                    break;
>>> +                case 3:
>>> +                    tx_core_id = lcore_id;
>>> +                    printf("Tx on priority core %d\n",
>>> +                            lcore_id);
>>> +                    break;
>>> +                default:
>>> +                    break;
>>> +                }
>>
>> This seems to be doing the same thing as right below (assigning lcore 
>> id's in order), yet in one case you use a switch, and in the other you 
>> use a simple loop. I don't see priority_num used anywhere else, so you 
>> might as well simplify this loop to be similar to what you have below, 
>> with "skip-if-not-turbo, if not assigned, assign-and-continue" type flow.
> 
> 
> There doing different things. The loop with the switch is looking for up 
> to three priority cores, and storing those choices in distr_core_id, 
> tx_core_id and rx_core_id. This is because we don't know which are the 
> priority cores ahead of time. priority_num is used in the switch 
> statement, and when it finds a priority core, it increments, so we know 
> which variable to assign with the next available priority core. Imagine 
> we have turbo enabled on cores 2,4 and 6. That's what I'm trying to 
> solve here.
> 
> Then, when we get to the next loop, we're just assigning the 
> non-priority cores if the three key workloads have not already been 
> assigned a core, hence the simple loop, using the remaining cores.
> 
> I looked at simplifying the flow, but as far as I can see, I need two 
> stages, a 'discovery' for the priority cores first, then whatever is 
> left can be done in a normal loop.
> 
> Does that make sense, or my I missing an obvious refactor opportunity?

I don't see how this is different from what you're doing below.

You are looping over cores, checking if it's a priority core, and 
assigning any priority cores found to distributor, Rx and Tx cores, in 
that order.

Below, you're looping over cores, checking if the core is already 
assigned, and assigning these cores to distributor, Rx and Tx cores, in 
that order.

So, the only tangible difference between the two is 1) the check for 
whether the cores are already assigned (you don't need that because 
these cores *cannot* be attached - you haven't looped over them yet!), 
and 2) check for whether the core is priority.

Just to clarify: i'm not saying merge the two loops, that can't work :) 
I'm saying, drop the switch and rewrite it like this:

for (cores) {
	if (core not priority)
		continue;
	if (dist_core not assigned) {
		assign dist core;
		continue;
	}
	if (rx core not assigned) {
		assign rx core;
		continue;
	}
	if (tx core not assigned) {
		assign tx core;
		continue;
	}
}

The functionality would be equivalent to the current switch method, but 
the code would be much clearer :)

> 
> 
>>
>> Once that is fixed,
>>
>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Hunt, David]

On 28/3/2019 3:10 PM, Burakov, Anatoly wrote:
> On 28-Mar-19 2:42 PM, Hunt, David wrote:
>>
>> On 28/3/2019 1:58 PM, Burakov, Anatoly wrote:
>>> On 28-Mar-19 1:13 PM, David Hunt wrote:
>>>> The distributor application is bottlenecked by the distributor core,
>>>> so if we can give more frequency to this core, then the overall
>>>> performance of the application may increase.
>>>>
>>>> This patch uses the rte_power_get_capabilities() API to query the
>>>> cores provided in the core mask, and if any high frequency cores are
>>>> found (e.g. Turbo Boost is enabled), we will pin the distributor
>>>> workload to that core.
>>>>
>>>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>>> ---
>>>
>>> <...>
>>>
>>>> +    if (power_lib_initialised)
>>>> +        rte_power_exit(rte_lcore_id());
>>>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>>>       return 0;
>>>>   }
>>>> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>>>>           if (num > 0)
>>>> app_stats.worker_bursts[p->worker_id][num-1]++;
>>>>       }
>>>> +    if (power_lib_initialised)
>>>> +        rte_power_exit(rte_lcore_id());
>>>> +    rte_free(p);
>>>>       return 0;
>>>>   }
>>>>   +static int
>>>> +init_power_library(void)
>>>> +{
>>>> +    int ret = 0, lcore_id;
>>>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>>> +        if (rte_lcore_is_enabled(lcore_id)) {
>>>
>>> Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
>>> checks if the lcore is enabled.
>>
>>
>> You're correct, I'll fix in next version.
>>
>>
>>>
>>> <...>
>>>
>>>>   +    if (power_lib_initialised) {
>>>> +        /*
>>>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>>>> +         * distributor workloads if there's higher frequency
>>>> +         * on those cores e.g. if Turbo Boost is enabled.
>>>> +         * It's also worth mentioning that it will assign cores in a
>>>> +         * specific order, so that if there's less than three
>>>> +         * available, the higher frequency cores will go to the
>>>> +         * distributor first, then rx, then tx.
>>>> +         */
>>>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>>> +
>>>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>>>> +
>>>> +            if (lcore_cap.turbo == 1) {
>>>> +                priority_num++;
>>>> +                switch (priority_num) {
>>>> +                case 1:
>>>> +                    distr_core_id = lcore_id;
>>>> +                    printf("Distributor on priority core %d\n",
>>>> +                            lcore_id);
>>>> +                    break;
>>>> +                case 2:
>>>> +                    rx_core_id = lcore_id;
>>>> +                    printf("Rx on priority core %d\n",
>>>> +                            lcore_id);
>>>> +                    break;
>>>> +                case 3:
>>>> +                    tx_core_id = lcore_id;
>>>> +                    printf("Tx on priority core %d\n",
>>>> +                            lcore_id);
>>>> +                    break;
>>>> +                default:
>>>> +                    break;
>>>> +                }
>>>
>>> This seems to be doing the same thing as right below (assigning 
>>> lcore id's in order), yet in one case you use a switch, and in the 
>>> other you use a simple loop. I don't see priority_num used anywhere 
>>> else, so you might as well simplify this loop to be similar to what 
>>> you have below, with "skip-if-not-turbo, if not assigned, 
>>> assign-and-continue" type flow.
>>
>>
>> There doing different things. The loop with the switch is looking for 
>> up to three priority cores, and storing those choices in 
>> distr_core_id, tx_core_id and rx_core_id. This is because we don't 
>> know which are the priority cores ahead of time. priority_num is used 
>> in the switch statement, and when it finds a priority core, it 
>> increments, so we know which variable to assign with the next 
>> available priority core. Imagine we have turbo enabled on cores 2,4 
>> and 6. That's what I'm trying to solve here.
>>
>> Then, when we get to the next loop, we're just assigning the 
>> non-priority cores if the three key workloads have not already been 
>> assigned a core, hence the simple loop, using the remaining cores.
>>
>> I looked at simplifying the flow, but as far as I can see, I need two 
>> stages, a 'discovery' for the priority cores first, then whatever is 
>> left can be done in a normal loop.
>>
>> Does that make sense, or my I missing an obvious refactor opportunity?
>
> I don't see how this is different from what you're doing below.
>
> You are looping over cores, checking if it's a priority core, and 
> assigning any priority cores found to distributor, Rx and Tx cores, in 
> that order.
>
> Below, you're looping over cores, checking if the core is already 
> assigned, and assigning these cores to distributor, Rx and Tx cores, 
> in that order.
>
> So, the only tangible difference between the two is 1) the check for 
> whether the cores are already assigned (you don't need that because 
> these cores *cannot* be attached - you haven't looped over them yet!), 
> and 2) check for whether the core is priority.
>
> Just to clarify: i'm not saying merge the two loops, that can't work 
> :) I'm saying, drop the switch and rewrite it like this:
>

Ah, gocha now. Will do in next rev. Thanks!


> for (cores) {
>     if (core not priority)
>         continue;
>     if (dist_core not assigned) {
>         assign dist core;
>         continue;
>     }
>     if (rx core not assigned) {
>         assign rx core;
>         continue;
>     }
>     if (tx core not assigned) {
>         assign tx core;
>         continue;
>     }
> }
>
> The functionality would be equivalent to the current switch method, 
> but the code would be much clearer :)
>
>>
>>
>>>
>>> Once that is fixed,
>>>
>>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>
>>
>
>

Re: [dpdk-dev] [PATCH v2] examples/distributor: detect high frequency cores

[Hunt, David]

On 28/3/2019 3:10 PM, Burakov, Anatoly wrote:
> On 28-Mar-19 2:42 PM, Hunt, David wrote:
>>
>> On 28/3/2019 1:58 PM, Burakov, Anatoly wrote:
>>> On 28-Mar-19 1:13 PM, David Hunt wrote:
>>>> The distributor application is bottlenecked by the distributor core,
>>>> so if we can give more frequency to this core, then the overall
>>>> performance of the application may increase.
>>>>
>>>> This patch uses the rte_power_get_capabilities() API to query the
>>>> cores provided in the core mask, and if any high frequency cores are
>>>> found (e.g. Turbo Boost is enabled), we will pin the distributor
>>>> workload to that core.
>>>>
>>>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>>> ---
>>>
>>> <...>
>>>
>>>> +    if (power_lib_initialised)
>>>> +        rte_power_exit(rte_lcore_id());
>>>>       printf("\nCore %u exiting tx task.\n", rte_lcore_id());
>>>>       return 0;
>>>>   }
>>>> @@ -575,9 +582,35 @@ lcore_worker(struct lcore_params *p)
>>>>           if (num > 0)
>>>> app_stats.worker_bursts[p->worker_id][num-1]++;
>>>>       }
>>>> +    if (power_lib_initialised)
>>>> +        rte_power_exit(rte_lcore_id());
>>>> +    rte_free(p);
>>>>       return 0;
>>>>   }
>>>>   +static int
>>>> +init_power_library(void)
>>>> +{
>>>> +    int ret = 0, lcore_id;
>>>> +    RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>>> +        if (rte_lcore_is_enabled(lcore_id)) {
>>>
>>> Please correct me if i'm wrong, but RTE_LCORE_FOREACH_SLAVE already 
>>> checks if the lcore is enabled.
>>
>>
>> You're correct, I'll fix in next version.
>>
>>
>>>
>>> <...>
>>>
>>>>   +    if (power_lib_initialised) {
>>>> +        /*
>>>> +         * Here we'll pre-assign lcore ids to the rx, tx and
>>>> +         * distributor workloads if there's higher frequency
>>>> +         * on those cores e.g. if Turbo Boost is enabled.
>>>> +         * It's also worth mentioning that it will assign cores in a
>>>> +         * specific order, so that if there's less than three
>>>> +         * available, the higher frequency cores will go to the
>>>> +         * distributor first, then rx, then tx.
>>>> +         */
>>>> +        RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>>> +
>>>> +            rte_power_get_capabilities(lcore_id, &lcore_cap);
>>>> +
>>>> +            if (lcore_cap.turbo == 1) {
>>>> +                priority_num++;
>>>> +                switch (priority_num) {
>>>> +                case 1:
>>>> +                    distr_core_id = lcore_id;
>>>> +                    printf("Distributor on priority core %d\n",
>>>> +                            lcore_id);
>>>> +                    break;
>>>> +                case 2:
>>>> +                    rx_core_id = lcore_id;
>>>> +                    printf("Rx on priority core %d\n",
>>>> +                            lcore_id);
>>>> +                    break;
>>>> +                case 3:
>>>> +                    tx_core_id = lcore_id;
>>>> +                    printf("Tx on priority core %d\n",
>>>> +                            lcore_id);
>>>> +                    break;
>>>> +                default:
>>>> +                    break;
>>>> +                }
>>>
>>> This seems to be doing the same thing as right below (assigning 
>>> lcore id's in order), yet in one case you use a switch, and in the 
>>> other you use a simple loop. I don't see priority_num used anywhere 
>>> else, so you might as well simplify this loop to be similar to what 
>>> you have below, with "skip-if-not-turbo, if not assigned, 
>>> assign-and-continue" type flow.
>>
>>
>> There doing different things. The loop with the switch is looking for 
>> up to three priority cores, and storing those choices in 
>> distr_core_id, tx_core_id and rx_core_id. This is because we don't 
>> know which are the priority cores ahead of time. priority_num is used 
>> in the switch statement, and when it finds a priority core, it 
>> increments, so we know which variable to assign with the next 
>> available priority core. Imagine we have turbo enabled on cores 2,4 
>> and 6. That's what I'm trying to solve here.
>>
>> Then, when we get to the next loop, we're just assigning the 
>> non-priority cores if the three key workloads have not already been 
>> assigned a core, hence the simple loop, using the remaining cores.
>>
>> I looked at simplifying the flow, but as far as I can see, I need two 
>> stages, a 'discovery' for the priority cores first, then whatever is 
>> left can be done in a normal loop.
>>
>> Does that make sense, or my I missing an obvious refactor opportunity?
>
> I don't see how this is different from what you're doing below.
>
> You are looping over cores, checking if it's a priority core, and 
> assigning any priority cores found to distributor, Rx and Tx cores, in 
> that order.
>
> Below, you're looping over cores, checking if the core is already 
> assigned, and assigning these cores to distributor, Rx and Tx cores, 
> in that order.
>
> So, the only tangible difference between the two is 1) the check for 
> whether the cores are already assigned (you don't need that because 
> these cores *cannot* be attached - you haven't looped over them yet!), 
> and 2) check for whether the core is priority.
>
> Just to clarify: i'm not saying merge the two loops, that can't work 
> :) I'm saying, drop the switch and rewrite it like this:
>

Ah, gocha now. Will do in next rev. Thanks!


> for (cores) {
>     if (core not priority)
>         continue;
>     if (dist_core not assigned) {
>         assign dist core;
>         continue;
>     }
>     if (rx core not assigned) {
>         assign rx core;
>         continue;
>     }
>     if (tx core not assigned) {
>         assign tx core;
>         continue;
>     }
> }
>
> The functionality would be equivalent to the current switch method, 
> but the code would be much clearer :)
>
>>
>>
>>>
>>> Once that is fixed,
>>>
>>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>
>>
>
>

[dpdk-dev] [PATCH v3] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/distributor/main.c      | 200 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 155 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..3ade4e461 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,33 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		/* init power management library */
+		ret = rte_power_init(lcore_id);
+		if (ret) {
+			RTE_LOG(ERR, POWER,
+				"Library initialization failed on core %u\n",
+				lcore_id);
+			/*
+			 * Return on first failure, we'll fall back
+			 * to non-power operation
+			 */
+			return ret;
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +688,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +720,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +778,122 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.turbo == 1) {
+				if (distr_core_id < 0) {
+					distr_core_id = lcore_id;
+					printf("Distributor on priority core %d\n",
+						lcore_id);
+					continue;
+				}
+				if (rx_core_id < 0) {
+					rx_core_id = lcore_id;
+					printf("Rx on priority core %d\n",
+						lcore_id);
+					continue;
+				}
+				if (tx_core_id < 0) {
+					tx_core_id = lcore_id;
+					printf("Tx on priority core %d\n",
+						lcore_id);
+					continue;
+				}
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
-					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
-				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
 		}
-		worker_id++;
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
+		}
+	}
+
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
+				p, lcore_id);
 	}
 
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +910,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

[dpdk-dev] [PATCH v3] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/distributor/main.c      | 200 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 155 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..3ade4e461 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,33 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		/* init power management library */
+		ret = rte_power_init(lcore_id);
+		if (ret) {
+			RTE_LOG(ERR, POWER,
+				"Library initialization failed on core %u\n",
+				lcore_id);
+			/*
+			 * Return on first failure, we'll fall back
+			 * to non-power operation
+			 */
+			return ret;
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +688,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +720,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +778,122 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.turbo == 1) {
+				if (distr_core_id < 0) {
+					distr_core_id = lcore_id;
+					printf("Distributor on priority core %d\n",
+						lcore_id);
+					continue;
+				}
+				if (rx_core_id < 0) {
+					rx_core_id = lcore_id;
+					printf("Rx on priority core %d\n",
+						lcore_id);
+					continue;
+				}
+				if (tx_core_id < 0) {
+					tx_core_id = lcore_id;
+					printf("Tx on priority core %d\n",
+						lcore_id);
+					continue;
+				}
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
-					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
-				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
 		}
-		worker_id++;
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
+		}
+	}
+
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
+				p, lcore_id);
 	}
 
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +910,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3] examples/distributor: detect high frequency cores

[Hunt, David]

Hi Thomas

On 29/3/2019 1:15 PM, David Hunt wrote:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
>
> This patch uses the rte_power_get_capabilities() API to query the
> cores provided in the core mask, and if any high frequency cores are
> found (e.g. Turbo Boost is enabled), we will pin the distributor
> workload to that core.
>
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---


Please wait for v4, I'm working on a change for later today.

Thanks,
Dave.

Re: [dpdk-dev] [PATCH v3] examples/distributor: detect high frequency cores

[Hunt, David]

Hi Thomas

On 29/3/2019 1:15 PM, David Hunt wrote:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
>
> This patch uses the rte_power_get_capabilities() API to query the
> cores provided in the core mask, and if any high frequency cores are
> found (e.g. Turbo Boost is enabled), we will pin the distributor
> workload to that core.
>
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---


Please wait for v4, I'm working on a change for later today.

Thanks,
Dave.

[dpdk-dev] [PATCH v4 1/2] lib/power: add bit for high frequency cores

[David Hunt]

This patch adds a new bit in the capabilities mask that's returned by
rte_power_get_capabilities(), allowing application to query which cores
have the higher frequencies, and can then pin the workloads accordingly.

Returned Bits:
 0 - Turbo Boost enabled
 1 - Higher core base_frequency

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_power/power_pstate_cpufreq.c | 56 ++++++++++++++++++++++---
 lib/librte_power/rte_power.h            |  1 +
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 9c1a1625f..25f61699a 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -67,6 +67,8 @@
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_max_freq"
 #define POWER_SYSFILE_BASE_MIN_FREQ  \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_min_freq"
+#define POWER_SYSFILE_BASE_FREQ  \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/base_frequency"
 #define POWER_MSR_PATH  "/dev/cpu/%u/msr"
 
 /*
@@ -94,9 +96,11 @@ struct pstate_power_info {
 	uint32_t curr_idx;                   /**< Freq index in freqs array */
 	uint32_t non_turbo_max_ratio;        /**< Non Turbo Max ratio  */
 	uint32_t sys_max_freq;               /**< system wide max freq  */
+	uint32_t core_base_freq;             /**< core base freq  */
 	volatile uint32_t state;             /**< Power in use state */
 	uint16_t turbo_available;            /**< Turbo Boost available */
 	uint16_t turbo_enable;               /**< Turbo Boost enable/disable */
+	uint16_t priority_core;              /**< High Performance core */
 } __rte_cache_aligned;
 
 
@@ -145,9 +149,13 @@ out:	close(fd);
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min, *f_max, *f_base;
 	char fullpath_min[PATH_MAX];
 	char fullpath_max[PATH_MAX];
+	char fullpath_base[PATH_MAX];
+	char buf_base[BUFSIZ];
+	char *s_base;
+	uint32_t base_ratio = 0;
 	uint64_t max_non_turbo = 0;
 
 	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
@@ -168,6 +176,26 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 	pi->f_cur_min = f_min;
 	pi->f_cur_max = f_max;
 
+	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
+			pi->lcore_id);
+
+	f_base = fopen(fullpath_base, "r");
+	if (f_base == NULL) {
+		/* No sysfs base_frequency, that's OK, continue without */
+		base_ratio = 0;
+	} else {
+		s_base = fgets(buf_base, sizeof(buf_base), f_base);
+		FOPS_OR_NULL_GOTO(s_base, out);
+
+		buf_base[BUFSIZ-1] = '\0';
+		if (strlen(buf_base))
+			/* Strip off terminating '\n' */
+			strtok(buf_base, "\n");
+
+		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
+				/ BUS_FREQ;
+	}
+
 	/* Add MSR read to detect turbo status */
 
 	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
@@ -179,6 +207,14 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 
 	pi->non_turbo_max_ratio = max_non_turbo;
 
+	/*Add the compare for base frequency  */
+	if (base_ratio > max_non_turbo)
+		pi->priority_core = 1;
+	else
+		pi->priority_core = 0;
+	pi->core_base_freq = base_ratio * BUS_FREQ;
+
+out:
 	return 0;
 }
 
@@ -215,9 +251,15 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 	}
 
 	/* Turbo is available and enabled, first freq bucket is sys max freq */
-	if (pi->turbo_available && pi->turbo_enable && (idx == 0))
-		target_freq = pi->sys_max_freq;
-	else
+	if (pi->turbo_available && idx == 0) {
+		if (pi->turbo_enable)
+			target_freq = pi->sys_max_freq;
+		else {
+			RTE_LOG(ERR, POWER, "Turbo is off, frequency can't be scaled up more %u\n",
+					pi->lcore_id);
+			return -1;
+		}
+	} else
 		target_freq = pi->freqs[idx];
 
 	/* Decrease freq, the min freq should be updated first */
@@ -430,7 +472,10 @@ power_get_available_freqs(struct pstate_power_info *pi)
 
 	pi->sys_max_freq = sys_max_freq;
 
-	base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
+	if (pi->priority_core == 1)
+		base_max_freq = pi->core_base_freq;
+	else
+		base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
 
 	POWER_DEBUG_TRACE("sys min %u, sys max %u, base_max %u\n",
 			sys_min_freq,
@@ -781,6 +826,7 @@ int power_pstate_get_capabilities(unsigned int lcore_id,
 	pi = &lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
+	caps->priority = pi->priority_core;
 
 	return 0;
 }
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index c5e8f6b5b..dee7af345 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -258,6 +258,7 @@ struct rte_power_core_capabilities {
 		RTE_STD_C11
 		struct {
 			uint64_t turbo:1;	/**< Turbo can be enabled. */
+			uint64_t priority:1;	/**< Priority core */
 		};
 	};
 };
-- 
2.17.1

[dpdk-dev] [PATCH v4 1/2] lib/power: add bit for high frequency cores

[David Hunt]

This patch adds a new bit in the capabilities mask that's returned by
rte_power_get_capabilities(), allowing application to query which cores
have the higher frequencies, and can then pin the workloads accordingly.

Returned Bits:
 0 - Turbo Boost enabled
 1 - Higher core base_frequency

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/librte_power/power_pstate_cpufreq.c | 56 ++++++++++++++++++++++---
 lib/librte_power/rte_power.h            |  1 +
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 9c1a1625f..25f61699a 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -67,6 +67,8 @@
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_max_freq"
 #define POWER_SYSFILE_BASE_MIN_FREQ  \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_min_freq"
+#define POWER_SYSFILE_BASE_FREQ  \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/base_frequency"
 #define POWER_MSR_PATH  "/dev/cpu/%u/msr"
 
 /*
@@ -94,9 +96,11 @@ struct pstate_power_info {
 	uint32_t curr_idx;                   /**< Freq index in freqs array */
 	uint32_t non_turbo_max_ratio;        /**< Non Turbo Max ratio  */
 	uint32_t sys_max_freq;               /**< system wide max freq  */
+	uint32_t core_base_freq;             /**< core base freq  */
 	volatile uint32_t state;             /**< Power in use state */
 	uint16_t turbo_available;            /**< Turbo Boost available */
 	uint16_t turbo_enable;               /**< Turbo Boost enable/disable */
+	uint16_t priority_core;              /**< High Performance core */
 } __rte_cache_aligned;
 
 
@@ -145,9 +149,13 @@ out:	close(fd);
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min, *f_max, *f_base;
 	char fullpath_min[PATH_MAX];
 	char fullpath_max[PATH_MAX];
+	char fullpath_base[PATH_MAX];
+	char buf_base[BUFSIZ];
+	char *s_base;
+	uint32_t base_ratio = 0;
 	uint64_t max_non_turbo = 0;
 
 	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
@@ -168,6 +176,26 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 	pi->f_cur_min = f_min;
 	pi->f_cur_max = f_max;
 
+	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
+			pi->lcore_id);
+
+	f_base = fopen(fullpath_base, "r");
+	if (f_base == NULL) {
+		/* No sysfs base_frequency, that's OK, continue without */
+		base_ratio = 0;
+	} else {
+		s_base = fgets(buf_base, sizeof(buf_base), f_base);
+		FOPS_OR_NULL_GOTO(s_base, out);
+
+		buf_base[BUFSIZ-1] = '\0';
+		if (strlen(buf_base))
+			/* Strip off terminating '\n' */
+			strtok(buf_base, "\n");
+
+		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
+				/ BUS_FREQ;
+	}
+
 	/* Add MSR read to detect turbo status */
 
 	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
@@ -179,6 +207,14 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 
 	pi->non_turbo_max_ratio = max_non_turbo;
 
+	/*Add the compare for base frequency  */
+	if (base_ratio > max_non_turbo)
+		pi->priority_core = 1;
+	else
+		pi->priority_core = 0;
+	pi->core_base_freq = base_ratio * BUS_FREQ;
+
+out:
 	return 0;
 }
 
@@ -215,9 +251,15 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 	}
 
 	/* Turbo is available and enabled, first freq bucket is sys max freq */
-	if (pi->turbo_available && pi->turbo_enable && (idx == 0))
-		target_freq = pi->sys_max_freq;
-	else
+	if (pi->turbo_available && idx == 0) {
+		if (pi->turbo_enable)
+			target_freq = pi->sys_max_freq;
+		else {
+			RTE_LOG(ERR, POWER, "Turbo is off, frequency can't be scaled up more %u\n",
+					pi->lcore_id);
+			return -1;
+		}
+	} else
 		target_freq = pi->freqs[idx];
 
 	/* Decrease freq, the min freq should be updated first */
@@ -430,7 +472,10 @@ power_get_available_freqs(struct pstate_power_info *pi)
 
 	pi->sys_max_freq = sys_max_freq;
 
-	base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
+	if (pi->priority_core == 1)
+		base_max_freq = pi->core_base_freq;
+	else
+		base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
 
 	POWER_DEBUG_TRACE("sys min %u, sys max %u, base_max %u\n",
 			sys_min_freq,
@@ -781,6 +826,7 @@ int power_pstate_get_capabilities(unsigned int lcore_id,
 	pi = &lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
+	caps->priority = pi->priority_core;
 
 	return 0;
 }
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index c5e8f6b5b..dee7af345 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -258,6 +258,7 @@ struct rte_power_core_capabilities {
 		RTE_STD_C11
 		struct {
 			uint64_t turbo:1;	/**< Turbo can be enabled. */
+			uint64_t priority:1;	/**< Priority core */
 		};
 	};
 };
-- 
2.17.1

[dpdk-dev] [PATCH v4 2/2] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/distributor/main.c      | 201 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 156 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..b5499bb12 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,33 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		/* init power management library */
+		ret = rte_power_init(lcore_id);
+		if (ret) {
+			RTE_LOG(ERR, POWER,
+				"Library initialization failed on core %u\n",
+				lcore_id);
+			/*
+			 * Return on first failure, we'll fall back
+			 * to non-power operation
+			 */
+			return ret;
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +688,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +720,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +778,123 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
-	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.priority != 1)
+				continue;
+
+			if (distr_core_id < 0) {
+				distr_core_id = lcore_id;
+				printf("Distributor on priority core %d\n",
 					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
-				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
+				continue;
+			}
+			if (rx_core_id < 0) {
+				rx_core_id = lcore_id;
+				printf("Rx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+			if (tx_core_id < 0) {
+				tx_core_id = lcore_id;
+				printf("Tx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
+		}
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
 		}
-		worker_id++;
 	}
 
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
+				p, lcore_id);
+	}
+
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +911,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

[dpdk-dev] [PATCH v4 2/2] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/distributor/main.c      | 201 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 156 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..b5499bb12 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,33 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		/* init power management library */
+		ret = rte_power_init(lcore_id);
+		if (ret) {
+			RTE_LOG(ERR, POWER,
+				"Library initialization failed on core %u\n",
+				lcore_id);
+			/*
+			 * Return on first failure, we'll fall back
+			 * to non-power operation
+			 */
+			return ret;
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +688,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +720,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +778,123 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
-	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.priority != 1)
+				continue;
+
+			if (distr_core_id < 0) {
+				distr_core_id = lcore_id;
+				printf("Distributor on priority core %d\n",
 					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
-				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
+				continue;
+			}
+			if (rx_core_id < 0) {
+				rx_core_id = lcore_id;
+				printf("Rx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+			if (tx_core_id < 0) {
+				tx_core_id = lcore_id;
+				printf("Tx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
+		}
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
 		}
-		worker_id++;
 	}
 
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
+				p, lcore_id);
+	}
+
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +911,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4 1/2] lib/power: add bit for high frequency cores

[Burakov, Anatoly]

On 01-Apr-19 4:30 PM, David Hunt wrote:
> This patch adds a new bit in the capabilities mask that's returned by
> rte_power_get_capabilities(), allowing application to query which cores
> have the higher frequencies, and can then pin the workloads accordingly.
> 
> Returned Bits:
>   0 - Turbo Boost enabled
>   1 - Higher core base_frequency
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---

<snip>

>   	/* Add MSR read to detect turbo status */
>   
>   	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
> @@ -179,6 +207,14 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
>   
>   	pi->non_turbo_max_ratio = max_non_turbo;
>   
> +	/*Add the compare for base frequency  */

The comment here looks meaningless, and needs to be reworded to explain 
why this is done, not what is done.

Otherwise,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v4 1/2] lib/power: add bit for high frequency cores

[Burakov, Anatoly]

On 01-Apr-19 4:30 PM, David Hunt wrote:
> This patch adds a new bit in the capabilities mask that's returned by
> rte_power_get_capabilities(), allowing application to query which cores
> have the higher frequencies, and can then pin the workloads accordingly.
> 
> Returned Bits:
>   0 - Turbo Boost enabled
>   1 - Higher core base_frequency
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---

<snip>

>   	/* Add MSR read to detect turbo status */
>   
>   	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
> @@ -179,6 +207,14 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
>   
>   	pi->non_turbo_max_ratio = max_non_turbo;
>   
> +	/*Add the compare for base frequency  */

The comment here looks meaningless, and needs to be reworded to explain 
why this is done, not what is done.

Otherwise,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v4 1/2] lib/power: add bit for high frequency cores

[Hunt, David]

Hi Anatoly,

On 1/4/2019 4:43 PM, Burakov, Anatoly wrote:
> On 01-Apr-19 4:30 PM, David Hunt wrote:
>> This patch adds a new bit in the capabilities mask that's returned by
>> rte_power_get_capabilities(), allowing application to query which cores
>> have the higher frequencies, and can then pin the workloads accordingly.
>>
>> Returned Bits:
>>   0 - Turbo Boost enabled
>>   1 - Higher core base_frequency
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>
> <snip>
>
>>       /* Add MSR read to detect turbo status */
>>         if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) 
>> < 0)
>> @@ -179,6 +207,14 @@ power_init_for_setting_freq(struct 
>> pstate_power_info *pi)
>>         pi->non_turbo_max_ratio = max_non_turbo;
>>   +    /*Add the compare for base frequency  */
>
> The comment here looks meaningless, and needs to be reworded to 
> explain why this is done, not what is done.
>
> Otherwise,
>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>

OK, Thanks for that. I'll push a v5 shortly, including your Reviewed-by tag.

Rgds,
Dave.

Re: [dpdk-dev] [PATCH v4 1/2] lib/power: add bit for high frequency cores

[Hunt, David]

Hi Anatoly,

On 1/4/2019 4:43 PM, Burakov, Anatoly wrote:
> On 01-Apr-19 4:30 PM, David Hunt wrote:
>> This patch adds a new bit in the capabilities mask that's returned by
>> rte_power_get_capabilities(), allowing application to query which cores
>> have the higher frequencies, and can then pin the workloads accordingly.
>>
>> Returned Bits:
>>   0 - Turbo Boost enabled
>>   1 - Higher core base_frequency
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>
> <snip>
>
>>       /* Add MSR read to detect turbo status */
>>         if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) 
>> < 0)
>> @@ -179,6 +207,14 @@ power_init_for_setting_freq(struct 
>> pstate_power_info *pi)
>>         pi->non_turbo_max_ratio = max_non_turbo;
>>   +    /*Add the compare for base frequency  */
>
> The comment here looks meaningless, and needs to be reworded to 
> explain why this is done, not what is done.
>
> Otherwise,
>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>

OK, Thanks for that. I'll push a v5 shortly, including your Reviewed-by tag.

Rgds,
Dave.

[dpdk-dev] [PATCH v5 1/2] lib/power: add bit for high frequency cores

[David Hunt]

This patch adds a new bit in the capabilities mask that's returned by
rte_power_get_capabilities(), allowing application to query which cores
have the higher frequencies, and can then pin the workloads accordingly.

Returned Bits:
 0 - Turbo Boost enabled
 1 - Higher core base_frequency

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/power_pstate_cpufreq.c | 59 ++++++++++++++++++++++---
 lib/librte_power/rte_power.h            |  1 +
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 9c1a1625f..beac00740 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -67,6 +67,8 @@
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_max_freq"
 #define POWER_SYSFILE_BASE_MIN_FREQ  \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_min_freq"
+#define POWER_SYSFILE_BASE_FREQ  \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/base_frequency"
 #define POWER_MSR_PATH  "/dev/cpu/%u/msr"
 
 /*
@@ -94,9 +96,11 @@ struct pstate_power_info {
 	uint32_t curr_idx;                   /**< Freq index in freqs array */
 	uint32_t non_turbo_max_ratio;        /**< Non Turbo Max ratio  */
 	uint32_t sys_max_freq;               /**< system wide max freq  */
+	uint32_t core_base_freq;             /**< core base freq  */
 	volatile uint32_t state;             /**< Power in use state */
 	uint16_t turbo_available;            /**< Turbo Boost available */
 	uint16_t turbo_enable;               /**< Turbo Boost enable/disable */
+	uint16_t priority_core;              /**< High Performance core */
 } __rte_cache_aligned;
 
 
@@ -145,9 +149,13 @@ out:	close(fd);
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min, *f_max, *f_base;
 	char fullpath_min[PATH_MAX];
 	char fullpath_max[PATH_MAX];
+	char fullpath_base[PATH_MAX];
+	char buf_base[BUFSIZ];
+	char *s_base;
+	uint32_t base_ratio = 0;
 	uint64_t max_non_turbo = 0;
 
 	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
@@ -168,6 +176,26 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 	pi->f_cur_min = f_min;
 	pi->f_cur_max = f_max;
 
+	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
+			pi->lcore_id);
+
+	f_base = fopen(fullpath_base, "r");
+	if (f_base == NULL) {
+		/* No sysfs base_frequency, that's OK, continue without */
+		base_ratio = 0;
+	} else {
+		s_base = fgets(buf_base, sizeof(buf_base), f_base);
+		FOPS_OR_NULL_GOTO(s_base, out);
+
+		buf_base[BUFSIZ-1] = '\0';
+		if (strlen(buf_base))
+			/* Strip off terminating '\n' */
+			strtok(buf_base, "\n");
+
+		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
+				/ BUS_FREQ;
+	}
+
 	/* Add MSR read to detect turbo status */
 
 	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
@@ -179,6 +207,17 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 
 	pi->non_turbo_max_ratio = max_non_turbo;
 
+	/*
+	 * If base_frequency is reported as greater than the maximum
+	 * non-turbo frequency, then mark it as a high priority core.
+	 */
+	if (base_ratio > max_non_turbo)
+		pi->priority_core = 1;
+	else
+		pi->priority_core = 0;
+	pi->core_base_freq = base_ratio * BUS_FREQ;
+
+out:
 	return 0;
 }
 
@@ -215,9 +254,15 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 	}
 
 	/* Turbo is available and enabled, first freq bucket is sys max freq */
-	if (pi->turbo_available && pi->turbo_enable && (idx == 0))
-		target_freq = pi->sys_max_freq;
-	else
+	if (pi->turbo_available && idx == 0) {
+		if (pi->turbo_enable)
+			target_freq = pi->sys_max_freq;
+		else {
+			RTE_LOG(ERR, POWER, "Turbo is off, frequency can't be scaled up more %u\n",
+					pi->lcore_id);
+			return -1;
+		}
+	} else
 		target_freq = pi->freqs[idx];
 
 	/* Decrease freq, the min freq should be updated first */
@@ -430,7 +475,10 @@ power_get_available_freqs(struct pstate_power_info *pi)
 
 	pi->sys_max_freq = sys_max_freq;
 
-	base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
+	if (pi->priority_core == 1)
+		base_max_freq = pi->core_base_freq;
+	else
+		base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
 
 	POWER_DEBUG_TRACE("sys min %u, sys max %u, base_max %u\n",
 			sys_min_freq,
@@ -781,6 +829,7 @@ int power_pstate_get_capabilities(unsigned int lcore_id,
 	pi = &lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
+	caps->priority = pi->priority_core;
 
 	return 0;
 }
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index c5e8f6b5b..dee7af345 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -258,6 +258,7 @@ struct rte_power_core_capabilities {
 		RTE_STD_C11
 		struct {
 			uint64_t turbo:1;	/**< Turbo can be enabled. */
+			uint64_t priority:1;	/**< Priority core */
 		};
 	};
 };
-- 
2.17.1

[dpdk-dev] [PATCH v5 1/2] lib/power: add bit for high frequency cores

[David Hunt]

This patch adds a new bit in the capabilities mask that's returned by
rte_power_get_capabilities(), allowing application to query which cores
have the higher frequencies, and can then pin the workloads accordingly.

Returned Bits:
 0 - Turbo Boost enabled
 1 - Higher core base_frequency

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_power/power_pstate_cpufreq.c | 59 ++++++++++++++++++++++---
 lib/librte_power/rte_power.h            |  1 +
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 9c1a1625f..beac00740 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -67,6 +67,8 @@
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_max_freq"
 #define POWER_SYSFILE_BASE_MIN_FREQ  \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_min_freq"
+#define POWER_SYSFILE_BASE_FREQ  \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/base_frequency"
 #define POWER_MSR_PATH  "/dev/cpu/%u/msr"
 
 /*
@@ -94,9 +96,11 @@ struct pstate_power_info {
 	uint32_t curr_idx;                   /**< Freq index in freqs array */
 	uint32_t non_turbo_max_ratio;        /**< Non Turbo Max ratio  */
 	uint32_t sys_max_freq;               /**< system wide max freq  */
+	uint32_t core_base_freq;             /**< core base freq  */
 	volatile uint32_t state;             /**< Power in use state */
 	uint16_t turbo_available;            /**< Turbo Boost available */
 	uint16_t turbo_enable;               /**< Turbo Boost enable/disable */
+	uint16_t priority_core;              /**< High Performance core */
 } __rte_cache_aligned;
 
 
@@ -145,9 +149,13 @@ out:	close(fd);
 static int
 power_init_for_setting_freq(struct pstate_power_info *pi)
 {
-	FILE *f_min, *f_max;
+	FILE *f_min, *f_max, *f_base;
 	char fullpath_min[PATH_MAX];
 	char fullpath_max[PATH_MAX];
+	char fullpath_base[PATH_MAX];
+	char buf_base[BUFSIZ];
+	char *s_base;
+	uint32_t base_ratio = 0;
 	uint64_t max_non_turbo = 0;
 
 	snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
@@ -168,6 +176,26 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 	pi->f_cur_min = f_min;
 	pi->f_cur_max = f_max;
 
+	snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
+			pi->lcore_id);
+
+	f_base = fopen(fullpath_base, "r");
+	if (f_base == NULL) {
+		/* No sysfs base_frequency, that's OK, continue without */
+		base_ratio = 0;
+	} else {
+		s_base = fgets(buf_base, sizeof(buf_base), f_base);
+		FOPS_OR_NULL_GOTO(s_base, out);
+
+		buf_base[BUFSIZ-1] = '\0';
+		if (strlen(buf_base))
+			/* Strip off terminating '\n' */
+			strtok(buf_base, "\n");
+
+		base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
+				/ BUS_FREQ;
+	}
+
 	/* Add MSR read to detect turbo status */
 
 	if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
@@ -179,6 +207,17 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
 
 	pi->non_turbo_max_ratio = max_non_turbo;
 
+	/*
+	 * If base_frequency is reported as greater than the maximum
+	 * non-turbo frequency, then mark it as a high priority core.
+	 */
+	if (base_ratio > max_non_turbo)
+		pi->priority_core = 1;
+	else
+		pi->priority_core = 0;
+	pi->core_base_freq = base_ratio * BUS_FREQ;
+
+out:
 	return 0;
 }
 
@@ -215,9 +254,15 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
 	}
 
 	/* Turbo is available and enabled, first freq bucket is sys max freq */
-	if (pi->turbo_available && pi->turbo_enable && (idx == 0))
-		target_freq = pi->sys_max_freq;
-	else
+	if (pi->turbo_available && idx == 0) {
+		if (pi->turbo_enable)
+			target_freq = pi->sys_max_freq;
+		else {
+			RTE_LOG(ERR, POWER, "Turbo is off, frequency can't be scaled up more %u\n",
+					pi->lcore_id);
+			return -1;
+		}
+	} else
 		target_freq = pi->freqs[idx];
 
 	/* Decrease freq, the min freq should be updated first */
@@ -430,7 +475,10 @@ power_get_available_freqs(struct pstate_power_info *pi)
 
 	pi->sys_max_freq = sys_max_freq;
 
-	base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
+	if (pi->priority_core == 1)
+		base_max_freq = pi->core_base_freq;
+	else
+		base_max_freq = pi->non_turbo_max_ratio * BUS_FREQ;
 
 	POWER_DEBUG_TRACE("sys min %u, sys max %u, base_max %u\n",
 			sys_min_freq,
@@ -781,6 +829,7 @@ int power_pstate_get_capabilities(unsigned int lcore_id,
 	pi = &lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
+	caps->priority = pi->priority_core;
 
 	return 0;
 }
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index c5e8f6b5b..dee7af345 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -258,6 +258,7 @@ struct rte_power_core_capabilities {
 		RTE_STD_C11
 		struct {
 			uint64_t turbo:1;	/**< Turbo can be enabled. */
+			uint64_t priority:1;	/**< Priority core */
 		};
 	};
 };
-- 
2.17.1

[dpdk-dev] [PATCH v5 2/2] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/distributor/main.c      | 201 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 156 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..b5499bb12 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,33 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		/* init power management library */
+		ret = rte_power_init(lcore_id);
+		if (ret) {
+			RTE_LOG(ERR, POWER,
+				"Library initialization failed on core %u\n",
+				lcore_id);
+			/*
+			 * Return on first failure, we'll fall back
+			 * to non-power operation
+			 */
+			return ret;
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +688,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +720,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +778,123 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
-	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.priority != 1)
+				continue;
+
+			if (distr_core_id < 0) {
+				distr_core_id = lcore_id;
+				printf("Distributor on priority core %d\n",
 					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
-				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
+				continue;
+			}
+			if (rx_core_id < 0) {
+				rx_core_id = lcore_id;
+				printf("Rx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+			if (tx_core_id < 0) {
+				tx_core_id = lcore_id;
+				printf("Tx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
+		}
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
 		}
-		worker_id++;
 	}
 
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
+				p, lcore_id);
+	}
+
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +911,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

[dpdk-dev] [PATCH v5 2/2] examples/distributor: detect high frequency cores

[David Hunt]

The distributor application is bottlenecked by the distributor core,
so if we can give more frequency to this core, then the overall
performance of the application may increase.

This patch uses the rte_power_get_capabilities() API to query the
cores provided in the core mask, and if any high frequency cores are
found (e.g. Turbo Boost is enabled), we will pin the distributor
workload to that core.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/distributor/main.c      | 201 ++++++++++++++++++++++++-------
 examples/distributor/meson.build |   2 +-
 2 files changed, 156 insertions(+), 47 deletions(-)

diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 03a05e3d9..b5499bb12 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -16,6 +16,7 @@
 #include <rte_prefetch.h>
 #include <rte_distributor.h>
 #include <rte_pause.h>
+#include <rte_power.h>
 
 #define RX_RING_SIZE 1024
 #define TX_RING_SIZE 1024
@@ -37,6 +38,7 @@ volatile uint8_t quit_signal;
 volatile uint8_t quit_signal_rx;
 volatile uint8_t quit_signal_dist;
 volatile uint8_t quit_signal_work;
+unsigned int power_lib_initialised;
 
 static volatile struct app_stats {
 	struct {
@@ -281,6 +283,8 @@ lcore_rx(struct lcore_params *p)
 		if (++port == nb_ports)
 			port = 0;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	/* set worker & tx threads quit flag */
 	printf("\nCore %u exiting rx task.\n", rte_lcore_id());
 	quit_signal = 1;
@@ -363,7 +367,8 @@ lcore_distributor(struct lcore_params *p)
 	}
 	printf("\nCore %u exiting distributor task.\n", rte_lcore_id());
 	quit_signal_work = 1;
-
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	rte_distributor_flush(d);
 	/* Unblock any returns so workers can exit */
 	rte_distributor_clear_returns(d);
@@ -435,6 +440,8 @@ lcore_tx(struct rte_ring *in_r)
 			}
 		}
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
 	printf("\nCore %u exiting tx task.\n", rte_lcore_id());
 	return 0;
 }
@@ -575,9 +582,33 @@ lcore_worker(struct lcore_params *p)
 		if (num > 0)
 			app_stats.worker_bursts[p->worker_id][num-1]++;
 	}
+	if (power_lib_initialised)
+		rte_power_exit(rte_lcore_id());
+	rte_free(p);
 	return 0;
 }
 
+static int
+init_power_library(void)
+{
+	int ret = 0, lcore_id;
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		/* init power management library */
+		ret = rte_power_init(lcore_id);
+		if (ret) {
+			RTE_LOG(ERR, POWER,
+				"Library initialization failed on core %u\n",
+				lcore_id);
+			/*
+			 * Return on first failure, we'll fall back
+			 * to non-power operation
+			 */
+			return ret;
+		}
+	}
+	return ret;
+}
+
 /* display usage */
 static void
 print_usage(const char *prgname)
@@ -657,7 +688,9 @@ main(int argc, char *argv[])
 	struct rte_distributor *d;
 	struct rte_ring *dist_tx_ring;
 	struct rte_ring *rx_dist_ring;
-	unsigned lcore_id, worker_id = 0;
+	struct rte_power_core_capabilities lcore_cap;
+	unsigned int lcore_id, worker_id = 0;
+	int distr_core_id = -1, rx_core_id = -1, tx_core_id = -1;
 	unsigned nb_ports;
 	uint16_t portid;
 	uint16_t nb_ports_available;
@@ -687,6 +720,9 @@ main(int argc, char *argv[])
 				"1 lcore for packet TX\n"
 				"and at least 1 lcore for worker threads\n");
 
+	if (init_power_library() == 0)
+		power_lib_initialised = 1;
+
 	nb_ports = rte_eth_dev_count_avail();
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "Error: no ethernet ports detected\n");
@@ -742,54 +778,123 @@ main(int argc, char *argv[])
 	if (rx_dist_ring == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot create output ring\n");
 
-	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
-		if (worker_id == rte_lcore_count() - 3) {
-			printf("Starting distributor on lcore_id %d\n",
+	if (power_lib_initialised) {
+		/*
+		 * Here we'll pre-assign lcore ids to the rx, tx and
+		 * distributor workloads if there's higher frequency
+		 * on those cores e.g. if Turbo Boost is enabled.
+		 * It's also worth mentioning that it will assign cores in a
+		 * specific order, so that if there's less than three
+		 * available, the higher frequency cores will go to the
+		 * distributor first, then rx, then tx.
+		 */
+		RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+
+			rte_power_get_capabilities(lcore_id, &lcore_cap);
+
+			if (lcore_cap.priority != 1)
+				continue;
+
+			if (distr_core_id < 0) {
+				distr_core_id = lcore_id;
+				printf("Distributor on priority core %d\n",
 					lcore_id);
-			/* distributor core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d,
-				rx_dist_ring, dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch(
-				(lcore_function_t *)lcore_distributor,
-				p, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 4) {
-			printf("Starting tx  on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* tx core */
-			rte_eal_remote_launch((lcore_function_t *)lcore_tx,
-					dist_tx_ring, lcore_id);
-		} else if (worker_id == rte_lcore_count() - 2) {
-			printf("Starting rx on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			/* rx core */
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-			rte_eal_remote_launch((lcore_function_t *)lcore_rx,
-					p, lcore_id);
-		} else {
-			printf("Starting worker on worker_id %d, lcore_id %d\n",
-					worker_id, lcore_id);
-			struct lcore_params *p =
-					rte_malloc(NULL, sizeof(*p), 0);
-			if (!p)
-				rte_panic("malloc failure\n");
-			*p = (struct lcore_params){worker_id, d, rx_dist_ring,
-					dist_tx_ring, mbuf_pool};
-
-			rte_eal_remote_launch((lcore_function_t *)lcore_worker,
-					p, lcore_id);
+				continue;
+			}
+			if (rx_core_id < 0) {
+				rx_core_id = lcore_id;
+				printf("Rx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+			if (tx_core_id < 0) {
+				tx_core_id = lcore_id;
+				printf("Tx on priority core %d\n",
+					lcore_id);
+				continue;
+			}
+		}
+	}
+
+	/*
+	 * If there's any of the key workloads left without an lcore_id
+	 * after the high performing core assignment above, pre-assign
+	 * them here.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		if (distr_core_id < 0) {
+			distr_core_id = lcore_id;
+			printf("Distributor on core %d\n", lcore_id);
+			continue;
+		}
+		if (rx_core_id < 0) {
+			rx_core_id = lcore_id;
+			printf("Rx on core %d\n", lcore_id);
+			continue;
+		}
+		if (tx_core_id < 0) {
+			tx_core_id = lcore_id;
+			printf("Tx on core %d\n", lcore_id);
+			continue;
 		}
-		worker_id++;
 	}
 
+	printf(" tx id %d, dist id %d, rx id %d\n",
+			tx_core_id,
+			distr_core_id,
+			rx_core_id);
+
+	/*
+	 * Kick off all the worker threads first, avoiding the pre-assigned
+	 * lcore_ids for tx, rx and distributor workloads.
+	 */
+	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
+		if (lcore_id == (unsigned int)distr_core_id ||
+				lcore_id == (unsigned int)rx_core_id ||
+				lcore_id == (unsigned int)tx_core_id)
+			continue;
+		printf("Starting thread %d as worker, lcore_id %d\n",
+				worker_id, lcore_id);
+		struct lcore_params *p =
+			rte_malloc(NULL, sizeof(*p), 0);
+		if (!p)
+			rte_panic("malloc failure\n");
+		*p = (struct lcore_params){worker_id++, d, rx_dist_ring,
+			dist_tx_ring, mbuf_pool};
+
+		rte_eal_remote_launch((lcore_function_t *)lcore_worker,
+				p, lcore_id);
+	}
+
+	/* Start tx core */
+	rte_eal_remote_launch((lcore_function_t *)lcore_tx,
+			dist_tx_ring, tx_core_id);
+
+	/* Start distributor core */
+	struct lcore_params *pd =
+		rte_malloc(NULL, sizeof(*pd), 0);
+	if (!pd)
+		rte_panic("malloc failure\n");
+	*pd = (struct lcore_params){worker_id++, d,
+		rx_dist_ring, dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch(
+			(lcore_function_t *)lcore_distributor,
+			pd, distr_core_id);
+
+	/* Start rx core */
+	struct lcore_params *pr =
+		rte_malloc(NULL, sizeof(*pr), 0);
+	if (!pr)
+		rte_panic("malloc failure\n");
+	*pr = (struct lcore_params){worker_id++, d, rx_dist_ring,
+		dist_tx_ring, mbuf_pool};
+	rte_eal_remote_launch((lcore_function_t *)lcore_rx,
+			pr, rx_core_id);
+
 	freq = rte_get_timer_hz();
 	t = rte_rdtsc() + freq;
 	while (!quit_signal_dist) {
@@ -806,5 +911,9 @@ main(int argc, char *argv[])
 	}
 
 	print_stats();
+
+	rte_free(pd);
+	rte_free(pr);
+
 	return 0;
 }
diff --git a/examples/distributor/meson.build b/examples/distributor/meson.build
index 88c001f56..8cf2ca1da 100644
--- a/examples/distributor/meson.build
+++ b/examples/distributor/meson.build
@@ -6,7 +6,7 @@
 # To build this example as a standalone application with an already-installed
 # DPDK instance, use 'make'
 
-deps += 'distributor'
+deps += ['distributor', 'power']
 sources = files(
 	'main.c'
 )
-- 
2.17.1

Re: [dpdk-dev] [PATCH v5 2/2] examples/distributor: detect high frequency cores

[Thomas Monjalon]

01/04/2019 18:14, David Hunt:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
> 
> This patch uses the rte_power_get_capabilities() API to query the
> cores provided in the core mask, and if any high frequency cores are
> found (e.g. Turbo Boost is enabled), we will pin the distributor
> workload to that core.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

Series applied, thanks

Re: [dpdk-dev] [PATCH v5 2/2] examples/distributor: detect high frequency cores

[Thomas Monjalon]

01/04/2019 18:14, David Hunt:
> The distributor application is bottlenecked by the distributor core,
> so if we can give more frequency to this core, then the overall
> performance of the application may increase.
> 
> This patch uses the rte_power_get_capabilities() API to query the
> cores provided in the core mask, and if any high frequency cores are
> found (e.g. Turbo Boost is enabled), we will pin the distributor
> workload to that core.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

Series applied, thanks

Re: [dpdk-dev] [PATCH v5 2/2] examples/distributor: detect high frequency cores

[Thomas Monjalon]

02/04/2019 02:06, Thomas Monjalon:
> 01/04/2019 18:14, David Hunt:
> > The distributor application is bottlenecked by the distributor core,
> > so if we can give more frequency to this core, then the overall
> > performance of the application may increase.
> > 
> > This patch uses the rte_power_get_capabilities() API to query the
> > cores provided in the core mask, and if any high frequency cores are
> > found (e.g. Turbo Boost is enabled), we will pin the distributor
> > workload to that core.
> > 
> > Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> > Signed-off-by: David Hunt <david.hunt@intel.com>
> > Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Series applied, thanks

Just amended the commit with this fix:

--- a/examples/Makefile
+++ b/examples/Makefile
+ifeq ($(CONFIG_RTE_LIBRTE_POWER),y)
 DIRS-$(CONFIG_RTE_LIBRTE_DISTRIBUTOR) += distributor
+endif

Re: [dpdk-dev] [PATCH v5 2/2] examples/distributor: detect high frequency cores

[Thomas Monjalon]

02/04/2019 02:06, Thomas Monjalon:
> 01/04/2019 18:14, David Hunt:
> > The distributor application is bottlenecked by the distributor core,
> > so if we can give more frequency to this core, then the overall
> > performance of the application may increase.
> > 
> > This patch uses the rte_power_get_capabilities() API to query the
> > cores provided in the core mask, and if any high frequency cores are
> > found (e.g. Turbo Boost is enabled), we will pin the distributor
> > workload to that core.
> > 
> > Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> > Signed-off-by: David Hunt <david.hunt@intel.com>
> > Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Series applied, thanks

Just amended the commit with this fix:

--- a/examples/Makefile
+++ b/examples/Makefile
+ifeq ($(CONFIG_RTE_LIBRTE_POWER),y)
 DIRS-$(CONFIG_RTE_LIBRTE_DISTRIBUTOR) += distributor
+endif