[dpdk-dev] [PATCH 0/3] Add interrupt-only mode to l3fwd-power

[dpdk-dev] [PATCH 0/3] Add interrupt-only mode to l3fwd-power

[Anatoly Burakov]

Since 20.05, l3fwd-power has become much more stringent about
whether it allows initialization without initializing the
librte_power library with it. This means that while previously
the app could have been used to test RX interrupts functionality
even if the app itself was in a half-working state, it is now
no longer possible to do so.

To address this use case, we're adding an interrupt-only mode
that does not rely on librte_power or telemetry. This enables
using l3fwd-power in environments where librte_power is not
expected to work (such as inside a VM or on non-IA
architectures). The RX/TX path is basically copy paste from
legacy RX/TX path but with librte_power bits taken out.

There seem to be two opposing schools of thought on whether we
should have more or less examples. This patchset goes in the
"less" direction where we add a new mode to an existing app,
rather than creating a new one like it could be argued it
deserves.

Anatoly Burakov (3):
  l3fwd-power: disable interrupts by default
  l3fwd-power: only allow supported power library envs
  l3fwd-power: add interrupt-only mode

 examples/l3fwd-power/main.c | 234 +++++++++++++++++++++++++++++++++---
 1 file changed, 217 insertions(+), 17 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/3] l3fwd-power: disable interrupts by default

[Anatoly Burakov]

Currently, interrupts are enabled in telemetry and empty poll modes, but
they are not used. Switch to disabling interrupts by default, and only
enable interrupts for modes that require them.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 9db94ce044..2cc5d7b121 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -256,10 +256,7 @@ static struct rte_eth_conf port_conf = {
 	},
 	.txmode = {
 		.mq_mode = ETH_MQ_TX_NONE,
-	},
-	.intr_conf = {
-		.rxq = 1,
-	},
+	}
 };
 
 static struct rte_mempool * pktmbuf_pool[NB_SOCKETS];
@@ -2270,6 +2267,8 @@ main(int argc, char **argv)
 	/* initialize all ports */
 	RTE_ETH_FOREACH_DEV(portid) {
 		struct rte_eth_conf local_port_conf = port_conf;
+		/* not all app modes need interrupts */
+		bool need_intr = app_mode == APP_MODE_LEGACY;
 
 		/* skip ports that are not enabled */
 		if ((enabled_port_mask & (1 << portid)) == 0) {
@@ -2303,7 +2302,10 @@ main(int argc, char **argv)
 			nb_rx_queue, (unsigned)n_tx_queue );
 		/* If number of Rx queue is 0, no need to enable Rx interrupt */
 		if (nb_rx_queue == 0)
-			local_port_conf.intr_conf.rxq = 0;
+			need_intr = false;
+
+		if (need_intr)
+			local_port_conf.intr_conf.rxq = 1;
 
 		ret = rte_eth_dev_info_get(portid, &dev_info);
 		if (ret != 0)
-- 
2.17.1

[dpdk-dev] [PATCH 2/3] l3fwd-power: only allow supported power library envs

[Anatoly Burakov]

Currently, l3fwd-power will attempt to run even if the power env
is set to KVM, which is not supported. Fix this by preventing the
app from initializing unless the env is set to one of the supported
modes.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 2cc5d7b121..5cee9d5387 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -2043,6 +2043,7 @@ static int check_ptype(uint16_t portid)
 static int
 init_power_library(void)
 {
+	enum power_management_env env;
 	unsigned int lcore_id;
 	int ret = 0;
 
@@ -2055,6 +2056,14 @@ init_power_library(void)
 				lcore_id);
 			return ret;
 		}
+		/* we're not supporting the VM channel mode */
+		env = rte_power_get_env();
+		if (env != PM_ENV_ACPI_CPUFREQ &&
+				env != PM_ENV_PSTATE_CPUFREQ) {
+			RTE_LOG(ERR, POWER,
+				"Only ACPI and PSTATE mode are supported\n");
+			return -1;
+		}
 	}
 	return ret;
 }
-- 
2.17.1

[dpdk-dev] [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Anatoly Burakov]

In addition to existing modes, add a mode which is very similar to
legacy mode, but does not do frequency scaling, and thus does not
depend on the power library.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 215 +++++++++++++++++++++++++++++++++---
 1 file changed, 202 insertions(+), 13 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 5cee9d5387..4161e01974 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -195,9 +195,11 @@ static int parse_ptype; /**< Parse packet type using rx callback, and */
 			/**< disabled by default */
 
 enum appmode {
-	APP_MODE_LEGACY = 0,
+	APP_MODE_DEFAULT = 0,
+	APP_MODE_LEGACY,
 	APP_MODE_EMPTY_POLL,
-	APP_MODE_TELEMETRY
+	APP_MODE_TELEMETRY,
+	APP_MODE_INTERRUPT
 };
 
 enum appmode app_mode;
@@ -900,6 +902,170 @@ static int event_register(struct lcore_conf *qconf)
 
 	return 0;
 }
+
+/* main processing loop */
+static int main_intr_loop(__rte_unused void *dummy)
+{
+	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
+	unsigned int lcore_id;
+	uint64_t prev_tsc, diff_tsc, cur_tsc;
+	int i, j, nb_rx;
+	uint8_t queueid;
+	uint16_t portid;
+	struct lcore_conf *qconf;
+	struct lcore_rx_queue *rx_queue;
+	uint32_t lcore_rx_idle_count = 0;
+	uint32_t lcore_idle_hint = 0;
+	int intr_en = 0;
+
+	const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
+				   US_PER_S * BURST_TX_DRAIN_US;
+
+	prev_tsc = 0;
+
+	lcore_id = rte_lcore_id();
+	qconf = &lcore_conf[lcore_id];
+
+	if (qconf->n_rx_queue == 0) {
+		RTE_LOG(INFO, L3FWD_POWER, "lcore %u has nothing to do\n",
+				lcore_id);
+		return 0;
+	}
+
+	RTE_LOG(INFO, L3FWD_POWER, "entering main interrupt loop on lcore %u\n",
+			lcore_id);
+
+	for (i = 0; i < qconf->n_rx_queue; i++) {
+		portid = qconf->rx_queue_list[i].port_id;
+		queueid = qconf->rx_queue_list[i].queue_id;
+		RTE_LOG(INFO, L3FWD_POWER,
+				" -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
+				lcore_id, portid, queueid);
+	}
+
+	/* add into event wait list */
+	if (event_register(qconf) == 0)
+		intr_en = 1;
+	else
+		RTE_LOG(INFO, L3FWD_POWER, "RX interrupt won't enable.\n");
+
+	while (!is_done()) {
+		stats[lcore_id].nb_iteration_looped++;
+
+		cur_tsc = rte_rdtsc();
+
+		/*
+		 * TX burst queue drain
+		 */
+		diff_tsc = cur_tsc - prev_tsc;
+		if (unlikely(diff_tsc > drain_tsc)) {
+			for (i = 0; i < qconf->n_tx_port; ++i) {
+				portid = qconf->tx_port_id[i];
+				rte_eth_tx_buffer_flush(portid,
+						qconf->tx_queue_id[portid],
+						qconf->tx_buffer[portid]);
+			}
+			prev_tsc = cur_tsc;
+		}
+
+start_rx:
+		/*
+		 * Read packet from RX queues
+		 */
+		lcore_rx_idle_count = 0;
+		for (i = 0; i < qconf->n_rx_queue; ++i) {
+			rx_queue = &(qconf->rx_queue_list[i]);
+			rx_queue->idle_hint = 0;
+			portid = rx_queue->port_id;
+			queueid = rx_queue->queue_id;
+
+			nb_rx = rte_eth_rx_burst(portid, queueid, pkts_burst,
+					MAX_PKT_BURST);
+
+			stats[lcore_id].nb_rx_processed += nb_rx;
+			if (unlikely(nb_rx == 0)) {
+				/**
+				 * no packet received from rx queue, try to
+				 * sleep for a while forcing CPU enter deeper
+				 * C states.
+				 */
+				rx_queue->zero_rx_packet_count++;
+
+				if (rx_queue->zero_rx_packet_count <=
+						MIN_ZERO_POLL_COUNT)
+					continue;
+
+				rx_queue->idle_hint = power_idle_heuristic(
+						rx_queue->zero_rx_packet_count);
+				lcore_rx_idle_count++;
+			} else {
+				rx_queue->zero_rx_packet_count = 0;
+			}
+
+			/* Prefetch first packets */
+			for (j = 0; j < PREFETCH_OFFSET && j < nb_rx; j++) {
+				rte_prefetch0(rte_pktmbuf_mtod(
+						pkts_burst[j], void *));
+			}
+
+			/* Prefetch and forward already prefetched packets */
+			for (j = 0; j < (nb_rx - PREFETCH_OFFSET); j++) {
+				rte_prefetch0(rte_pktmbuf_mtod(
+						pkts_burst[j + PREFETCH_OFFSET],
+						void *));
+				l3fwd_simple_forward(
+						pkts_burst[j], portid, qconf);
+			}
+
+			/* Forward remaining prefetched packets */
+			for (; j < nb_rx; j++) {
+				l3fwd_simple_forward(
+						pkts_burst[j], portid, qconf);
+			}
+		}
+
+		if (unlikely(lcore_rx_idle_count == qconf->n_rx_queue)) {
+			/**
+			 * All Rx queues empty in recent consecutive polls,
+			 * sleep in a conservative manner, meaning sleep as
+			 * less as possible.
+			 */
+			for (i = 1,
+			    lcore_idle_hint = qconf->rx_queue_list[0].idle_hint;
+					i < qconf->n_rx_queue; ++i) {
+				rx_queue = &(qconf->rx_queue_list[i]);
+				if (rx_queue->idle_hint < lcore_idle_hint)
+					lcore_idle_hint = rx_queue->idle_hint;
+			}
+
+			if (lcore_idle_hint < SUSPEND_THRESHOLD)
+				/**
+				 * execute "pause" instruction to avoid context
+				 * switch which generally take hundred of
+				 * microseconds for short sleep.
+				 */
+				rte_delay_us(lcore_idle_hint);
+			else {
+				/* suspend until rx interrupt triggers */
+				if (intr_en) {
+					turn_on_off_intr(qconf, 1);
+					sleep_until_rx_interrupt(
+							qconf->n_rx_queue);
+					turn_on_off_intr(qconf, 0);
+					/**
+					 * start receiving packets immediately
+					 */
+					if (likely(!is_done()))
+						goto start_rx;
+				}
+			}
+			stats[lcore_id].sleep_time += lcore_idle_hint;
+		}
+	}
+
+	return 0;
+}
+
 /* main processing loop */
 static int
 main_telemetry_loop(__rte_unused void *dummy)
@@ -1126,7 +1292,7 @@ main_empty_poll_loop(__rte_unused void *dummy)
 }
 /* main processing loop */
 static int
-main_loop(__rte_unused void *dummy)
+main_legacy_loop(__rte_unused void *dummy)
 {
 	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
 	unsigned lcore_id;
@@ -1438,7 +1604,8 @@ print_usage(const char *prgname)
 		"  --empty-poll: enable empty poll detection"
 		" follow (training_flag, high_threshold, med_threshold)\n"
 		" --telemetry: enable telemetry mode, to update"
-		" empty polls, full polls, and core busyness to telemetry\n",
+		" empty polls, full polls, and core busyness to telemetry\n"
+		" --interrupt-only: enable interrupt-only mode\n",
 		prgname);
 }
 
@@ -1582,6 +1749,7 @@ parse_ep_config(const char *q_arg)
 }
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
 #define CMD_LINE_OPT_TELEMETRY "telemetry"
+#define CMD_LINE_OPT_INTERRUPT_ONLY "interrupt-only"
 
 /* Parse the argument given in the command line of the application */
 static int
@@ -1601,6 +1769,7 @@ parse_args(int argc, char **argv)
 		{"empty-poll", 1, 0, 0},
 		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
+		{CMD_LINE_OPT_INTERRUPT_ONLY, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -1674,8 +1843,8 @@ parse_args(int argc, char **argv)
 
 			if (!strncmp(lgopts[option_index].name,
 						"empty-poll", 10)) {
-				if (app_mode == APP_MODE_TELEMETRY) {
-					printf(" empty-poll cannot be enabled as telemetry mode is enabled\n");
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" empty-poll mode is mutually exclusive with other modes\n");
 					return -1;
 				}
 				app_mode = APP_MODE_EMPTY_POLL;
@@ -1692,14 +1861,25 @@ parse_args(int argc, char **argv)
 			if (!strncmp(lgopts[option_index].name,
 					CMD_LINE_OPT_TELEMETRY,
 					sizeof(CMD_LINE_OPT_TELEMETRY))) {
-				if (app_mode == APP_MODE_EMPTY_POLL) {
-					printf("telemetry mode cannot be enabled as empty poll mode is enabled\n");
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" telemetry mode is mutually exclusive with other modes\n");
 					return -1;
 				}
 				app_mode = APP_MODE_TELEMETRY;
 				printf("telemetry mode is enabled\n");
 			}
 
+			if (!strncmp(lgopts[option_index].name,
+					CMD_LINE_OPT_INTERRUPT_ONLY,
+					sizeof(CMD_LINE_OPT_INTERRUPT_ONLY))) {
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" interrupt-only mode is mutually exclusive with other modes\n");
+					return -1;
+				}
+				app_mode = APP_MODE_INTERRUPT;
+				printf("interrupt-only mode is enabled\n");
+			}
+
 			if (!strncmp(lgopts[option_index].name,
 					"enable-jumbo", 12)) {
 				struct option lenopts =
@@ -2253,7 +2433,12 @@ main(int argc, char **argv)
 	if (ret < 0)
 		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
 
-	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
+	if (app_mode == APP_MODE_DEFAULT)
+		app_mode = APP_MODE_LEGACY;
+
+	/* only legacy and empty poll mode rely on power library */
+	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
+			init_power_library())
 		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
 
 	if (update_lcore_params() < 0)
@@ -2277,7 +2462,8 @@ main(int argc, char **argv)
 	RTE_ETH_FOREACH_DEV(portid) {
 		struct rte_eth_conf local_port_conf = port_conf;
 		/* not all app modes need interrupts */
-		bool need_intr = app_mode == APP_MODE_LEGACY;
+		bool need_intr = app_mode == APP_MODE_LEGACY ||
+				app_mode == APP_MODE_INTERRUPT;
 
 		/* skip ports that are not enabled */
 		if ((enabled_port_mask & (1 << portid)) == 0) {
@@ -2526,12 +2712,12 @@ main(int argc, char **argv)
 
 	/* launch per-lcore init on every lcore */
 	if (app_mode == APP_MODE_LEGACY) {
-		rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
+		rte_eal_mp_remote_launch(main_legacy_loop, NULL, CALL_MASTER);
 	} else if (app_mode == APP_MODE_EMPTY_POLL) {
 		empty_poll_stop = false;
 		rte_eal_mp_remote_launch(main_empty_poll_loop, NULL,
 				SKIP_MASTER);
-	} else {
+	} else if (app_mode == APP_MODE_TELEMETRY) {
 		unsigned int i;
 
 		/* Init metrics library */
@@ -2555,6 +2741,8 @@ main(int argc, char **argv)
 				"Returns global power stats. Parameters: None");
 		rte_eal_mp_remote_launch(main_telemetry_loop, NULL,
 						SKIP_MASTER);
+	} else if (app_mode == APP_MODE_INTERRUPT) {
+		rte_eal_mp_remote_launch(main_intr_loop, NULL, CALL_MASTER);
 	}
 
 	if (app_mode == APP_MODE_EMPTY_POLL || app_mode == APP_MODE_TELEMETRY)
@@ -2577,7 +2765,8 @@ main(int argc, char **argv)
 	if (app_mode == APP_MODE_EMPTY_POLL)
 		rte_power_empty_poll_stat_free();
 
-	if (app_mode != APP_MODE_TELEMETRY && deinit_power_library())
+	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
+			deinit_power_library())
 		rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
 
 	if (rte_eal_cleanup() < 0)
-- 
2.17.1

Re: [dpdk-dev] [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Harman Kalra]

On Thu, May 28, 2020 at 10:13:51AM +0100, Anatoly Burakov wrote:
> In addition to existing modes, add a mode which is very similar to
> legacy mode, but does not do frequency scaling, and thus does not
> depend on the power library.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  examples/l3fwd-power/main.c | 215 +++++++++++++++++++++++++++++++++---
>  1 file changed, 202 insertions(+), 13 deletions(-)
> 
> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index 5cee9d5387..4161e01974 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -195,9 +195,11 @@ static int parse_ptype; /**< Parse packet type using rx callback, and */
>  			/**< disabled by default */
>  
>  enum appmode {
> -	APP_MODE_LEGACY = 0,
> +	APP_MODE_DEFAULT = 0,
> +	APP_MODE_LEGACY,
>  	APP_MODE_EMPTY_POLL,
> -	APP_MODE_TELEMETRY
> +	APP_MODE_TELEMETRY,
> +	APP_MODE_INTERRUPT
>  };
>  
>  enum appmode app_mode;
> @@ -900,6 +902,170 @@ static int event_register(struct lcore_conf *qconf)
>  
>  	return 0;
>  }
> +
> +/* main processing loop */
> +static int main_intr_loop(__rte_unused void *dummy)
> +{
> +	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
> +	unsigned int lcore_id;
> +	uint64_t prev_tsc, diff_tsc, cur_tsc;
> +	int i, j, nb_rx;
> +	uint8_t queueid;
> +	uint16_t portid;
> +	struct lcore_conf *qconf;
> +	struct lcore_rx_queue *rx_queue;
> +	uint32_t lcore_rx_idle_count = 0;
> +	uint32_t lcore_idle_hint = 0;
> +	int intr_en = 0;
> +
> +	const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
> +				   US_PER_S * BURST_TX_DRAIN_US;
> +
> +	prev_tsc = 0;
> +
> +	lcore_id = rte_lcore_id();
> +	qconf = &lcore_conf[lcore_id];
> +
> +	if (qconf->n_rx_queue == 0) {
> +		RTE_LOG(INFO, L3FWD_POWER, "lcore %u has nothing to do\n",
> +				lcore_id);
> +		return 0;
> +	}
> +
> +	RTE_LOG(INFO, L3FWD_POWER, "entering main interrupt loop on lcore %u\n",
> +			lcore_id);
> +
> +	for (i = 0; i < qconf->n_rx_queue; i++) {
> +		portid = qconf->rx_queue_list[i].port_id;
> +		queueid = qconf->rx_queue_list[i].queue_id;
> +		RTE_LOG(INFO, L3FWD_POWER,
> +				" -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
> +				lcore_id, portid, queueid);
> +	}
> +
> +	/* add into event wait list */
> +	if (event_register(qconf) == 0)
> +		intr_en = 1;
> +	else
> +		RTE_LOG(INFO, L3FWD_POWER, "RX interrupt won't enable.\n");
> +
> +	while (!is_done()) {
> +		stats[lcore_id].nb_iteration_looped++;
> +
> +		cur_tsc = rte_rdtsc();
> +
> +		/*
> +		 * TX burst queue drain
> +		 */
> +		diff_tsc = cur_tsc - prev_tsc;
> +		if (unlikely(diff_tsc > drain_tsc)) {
> +			for (i = 0; i < qconf->n_tx_port; ++i) {
> +				portid = qconf->tx_port_id[i];
> +				rte_eth_tx_buffer_flush(portid,
> +						qconf->tx_queue_id[portid],
> +						qconf->tx_buffer[portid]);
> +			}
> +			prev_tsc = cur_tsc;
> +		}
> +
> +start_rx:
> +		/*
> +		 * Read packet from RX queues
> +		 */
> +		lcore_rx_idle_count = 0;
> +		for (i = 0; i < qconf->n_rx_queue; ++i) {
> +			rx_queue = &(qconf->rx_queue_list[i]);
> +			rx_queue->idle_hint = 0;
> +			portid = rx_queue->port_id;
> +			queueid = rx_queue->queue_id;
> +
> +			nb_rx = rte_eth_rx_burst(portid, queueid, pkts_burst,
> +					MAX_PKT_BURST);
> +
> +			stats[lcore_id].nb_rx_processed += nb_rx;
> +			if (unlikely(nb_rx == 0)) {
> +				/**
> +				 * no packet received from rx queue, try to
> +				 * sleep for a while forcing CPU enter deeper
> +				 * C states.
> +				 */
> +				rx_queue->zero_rx_packet_count++;
> +
> +				if (rx_queue->zero_rx_packet_count <=
> +						MIN_ZERO_POLL_COUNT)
> +					continue;
> +
> +				rx_queue->idle_hint = power_idle_heuristic(
> +						rx_queue->zero_rx_packet_count);
> +				lcore_rx_idle_count++;
> +			} else {
> +				rx_queue->zero_rx_packet_count = 0;
> +			}
> +
> +			/* Prefetch first packets */
> +			for (j = 0; j < PREFETCH_OFFSET && j < nb_rx; j++) {
> +				rte_prefetch0(rte_pktmbuf_mtod(
> +						pkts_burst[j], void *));
> +			}
> +
> +			/* Prefetch and forward already prefetched packets */
> +			for (j = 0; j < (nb_rx - PREFETCH_OFFSET); j++) {
> +				rte_prefetch0(rte_pktmbuf_mtod(
> +						pkts_burst[j + PREFETCH_OFFSET],
> +						void *));
> +				l3fwd_simple_forward(
> +						pkts_burst[j], portid, qconf);
> +			}
> +
> +			/* Forward remaining prefetched packets */
> +			for (; j < nb_rx; j++) {
> +				l3fwd_simple_forward(
> +						pkts_burst[j], portid, qconf);
> +			}
> +		}
> +
> +		if (unlikely(lcore_rx_idle_count == qconf->n_rx_queue)) {
> +			/**
> +			 * All Rx queues empty in recent consecutive polls,
> +			 * sleep in a conservative manner, meaning sleep as
> +			 * less as possible.
> +			 */
> +			for (i = 1,
> +			    lcore_idle_hint = qconf->rx_queue_list[0].idle_hint;
> +					i < qconf->n_rx_queue; ++i) {
> +				rx_queue = &(qconf->rx_queue_list[i]);
> +				if (rx_queue->idle_hint < lcore_idle_hint)
> +					lcore_idle_hint = rx_queue->idle_hint;
> +			}
> +
> +			if (lcore_idle_hint < SUSPEND_THRESHOLD)
> +				/**
> +				 * execute "pause" instruction to avoid context
> +				 * switch which generally take hundred of
> +				 * microseconds for short sleep.
> +				 */
> +				rte_delay_us(lcore_idle_hint);
> +			else {
> +				/* suspend until rx interrupt triggers */
> +				if (intr_en) {
> +					turn_on_off_intr(qconf, 1);
> +					sleep_until_rx_interrupt(
> +							qconf->n_rx_queue);
> +					turn_on_off_intr(qconf, 0);
> +					/**
> +					 * start receiving packets immediately
> +					 */
> +					if (likely(!is_done()))
> +						goto start_rx;
> +				}
> +			}
> +			stats[lcore_id].sleep_time += lcore_idle_hint;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /* main processing loop */
>  static int
>  main_telemetry_loop(__rte_unused void *dummy)
> @@ -1126,7 +1292,7 @@ main_empty_poll_loop(__rte_unused void *dummy)
>  }
>  /* main processing loop */
>  static int
> -main_loop(__rte_unused void *dummy)
> +main_legacy_loop(__rte_unused void *dummy)
>  {
>  	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>  	unsigned lcore_id;
> @@ -1438,7 +1604,8 @@ print_usage(const char *prgname)
>  		"  --empty-poll: enable empty poll detection"
>  		" follow (training_flag, high_threshold, med_threshold)\n"
>  		" --telemetry: enable telemetry mode, to update"
> -		" empty polls, full polls, and core busyness to telemetry\n",
> +		" empty polls, full polls, and core busyness to telemetry\n"
> +		" --interrupt-only: enable interrupt-only mode\n",
>  		prgname);
>  }
>  
> @@ -1582,6 +1749,7 @@ parse_ep_config(const char *q_arg)
>  }
>  #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
>  #define CMD_LINE_OPT_TELEMETRY "telemetry"
> +#define CMD_LINE_OPT_INTERRUPT_ONLY "interrupt-only"
>  
>  /* Parse the argument given in the command line of the application */
>  static int
> @@ -1601,6 +1769,7 @@ parse_args(int argc, char **argv)
>  		{"empty-poll", 1, 0, 0},
>  		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
>  		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
> +		{CMD_LINE_OPT_INTERRUPT_ONLY, 0, 0, 0},
>  		{NULL, 0, 0, 0}
>  	};
>  
> @@ -1674,8 +1843,8 @@ parse_args(int argc, char **argv)
>  
>  			if (!strncmp(lgopts[option_index].name,
>  						"empty-poll", 10)) {
> -				if (app_mode == APP_MODE_TELEMETRY) {
> -					printf(" empty-poll cannot be enabled as telemetry mode is enabled\n");
> +				if (app_mode != APP_MODE_DEFAULT) {
> +					printf(" empty-poll mode is mutually exclusive with other modes\n");
>  					return -1;
>  				}
>  				app_mode = APP_MODE_EMPTY_POLL;
> @@ -1692,14 +1861,25 @@ parse_args(int argc, char **argv)
>  			if (!strncmp(lgopts[option_index].name,
>  					CMD_LINE_OPT_TELEMETRY,
>  					sizeof(CMD_LINE_OPT_TELEMETRY))) {
> -				if (app_mode == APP_MODE_EMPTY_POLL) {
> -					printf("telemetry mode cannot be enabled as empty poll mode is enabled\n");
> +				if (app_mode != APP_MODE_DEFAULT) {
> +					printf(" telemetry mode is mutually exclusive with other modes\n");
>  					return -1;
>  				}
>  				app_mode = APP_MODE_TELEMETRY;
>  				printf("telemetry mode is enabled\n");
>  			}
>  
> +			if (!strncmp(lgopts[option_index].name,
> +					CMD_LINE_OPT_INTERRUPT_ONLY,
> +					sizeof(CMD_LINE_OPT_INTERRUPT_ONLY))) {
> +				if (app_mode != APP_MODE_DEFAULT) {
> +					printf(" interrupt-only mode is mutually exclusive with other modes\n");
> +					return -1;
> +				}
> +				app_mode = APP_MODE_INTERRUPT;
> +				printf("interrupt-only mode is enabled\n");
> +			}
> +
>  			if (!strncmp(lgopts[option_index].name,
>  					"enable-jumbo", 12)) {
>  				struct option lenopts =
> @@ -2253,7 +2433,12 @@ main(int argc, char **argv)
>  	if (ret < 0)
>  		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>  
> -	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> +	if (app_mode == APP_MODE_DEFAULT)
> +		app_mode = APP_MODE_LEGACY;
> +
> +	/* only legacy and empty poll mode rely on power library */
> +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> +			init_power_library())
>  		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
>  
Hi,

Rather than just exiting from here can we have a else condition to
automatically enter into the "interrupt only" mode.
Please correct me if I am missing something.

Thanks
Harman
	
>  	if (update_lcore_params() < 0)
> @@ -2277,7 +2462,8 @@ main(int argc, char **argv)
>  	RTE_ETH_FOREACH_DEV(portid) {
>  		struct rte_eth_conf local_port_conf = port_conf;
>  		/* not all app modes need interrupts */
> -		bool need_intr = app_mode == APP_MODE_LEGACY;
> +		bool need_intr = app_mode == APP_MODE_LEGACY ||
> +				app_mode == APP_MODE_INTERRUPT;
>  
>  		/* skip ports that are not enabled */
>  		if ((enabled_port_mask & (1 << portid)) == 0) {
> @@ -2526,12 +2712,12 @@ main(int argc, char **argv)
>  
>  	/* launch per-lcore init on every lcore */
>  	if (app_mode == APP_MODE_LEGACY) {
> -		rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
> +		rte_eal_mp_remote_launch(main_legacy_loop, NULL, CALL_MASTER);
>  	} else if (app_mode == APP_MODE_EMPTY_POLL) {
>  		empty_poll_stop = false;
>  		rte_eal_mp_remote_launch(main_empty_poll_loop, NULL,
>  				SKIP_MASTER);
> -	} else {
> +	} else if (app_mode == APP_MODE_TELEMETRY) {
>  		unsigned int i;
>  
>  		/* Init metrics library */
> @@ -2555,6 +2741,8 @@ main(int argc, char **argv)
>  				"Returns global power stats. Parameters: None");
>  		rte_eal_mp_remote_launch(main_telemetry_loop, NULL,
>  						SKIP_MASTER);
> +	} else if (app_mode == APP_MODE_INTERRUPT) {
> +		rte_eal_mp_remote_launch(main_intr_loop, NULL, CALL_MASTER);
>  	}
>  
>  	if (app_mode == APP_MODE_EMPTY_POLL || app_mode == APP_MODE_TELEMETRY)
> @@ -2577,7 +2765,8 @@ main(int argc, char **argv)
>  	if (app_mode == APP_MODE_EMPTY_POLL)
>  		rte_power_empty_poll_stat_free();
>  
> -	if (app_mode != APP_MODE_TELEMETRY && deinit_power_library())
> +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> +			deinit_power_library())
>  		rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
>  
>  	if (rte_eal_cleanup() < 0)
> -- 
> 2.17.1

Re: [dpdk-dev] [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Burakov, Anatoly]

On 29-May-20 2:19 PM, Harman Kalra wrote:

>>   	if (ret < 0)
>>   		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>>   
>> -	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
>> +	if (app_mode == APP_MODE_DEFAULT)
>> +		app_mode = APP_MODE_LEGACY;
>> +
>> +	/* only legacy and empty poll mode rely on power library */
>> +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
>> +			init_power_library())
>>   		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
>>   
> Hi,
> 
> Rather than just exiting from here can we have a else condition to
> automatically enter into the "interrupt only" mode.
> Please correct me if I am missing something.

Hi,

Thanks for your review. I don't think silently proceeding is a good 
idea. If the user wants interrupt-only mode, they should request it. 
Silently falling back to interrupt-only mode will create an illusion of 
successful initialization and set the wrong expectation for how the 
application will behave.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Harman Kalra]

On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On 29-May-20 2:19 PM, Harman Kalra wrote:
> 
> > >   	if (ret < 0)
> > >   		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> > > -	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> > > +	if (app_mode == APP_MODE_DEFAULT)
> > > +		app_mode = APP_MODE_LEGACY;
> > > +
> > > +	/* only legacy and empty poll mode rely on power library */
> > > +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> > > +			init_power_library())
> > >   		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> > Hi,
> > 
> > Rather than just exiting from here can we have a else condition to
> > automatically enter into the "interrupt only" mode.
> > Please correct me if I am missing something.
> 
> Hi,
> 
> Thanks for your review. I don't think silently proceeding is a good idea. If
> the user wants interrupt-only mode, they should request it. Silently falling
> back to interrupt-only mode will create an illusion of successful
> initialization and set the wrong expectation for how the application will
> behave.
> 

Hi,

Thanks for the explanation which even I also believe is logically perfect.

But since l3fwd-power is an old application and has many users around
which are currently using this app in interrupt only mode without giving
an extra argument. But suddenly they will start getting failure messages with
the new patchset.

My only intent with else condition was backward compatibility.
Or may be we can have more descriptive failure message, something like
"init_power_library failed, check manual for other modes".

Thanks
Harman


> -- 
> Thanks,
> Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Burakov, Anatoly]

On 30-May-20 11:02 AM, Harman Kalra wrote:
> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 29-May-20 2:19 PM, Harman Kalra wrote:
>>
>>>>    	if (ret < 0)
>>>>    		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>>>> -	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
>>>> +	if (app_mode == APP_MODE_DEFAULT)
>>>> +		app_mode = APP_MODE_LEGACY;
>>>> +
>>>> +	/* only legacy and empty poll mode rely on power library */
>>>> +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
>>>> +			init_power_library())
>>>>    		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
>>> Hi,
>>>
>>> Rather than just exiting from here can we have a else condition to
>>> automatically enter into the "interrupt only" mode.
>>> Please correct me if I am missing something.
>>
>> Hi,
>>
>> Thanks for your review. I don't think silently proceeding is a good idea. If
>> the user wants interrupt-only mode, they should request it. Silently falling
>> back to interrupt-only mode will create an illusion of successful
>> initialization and set the wrong expectation for how the application will
>> behave.
>>
> 
> Hi,
> 
> Thanks for the explanation which even I also believe is logically perfect.
> 
> But since l3fwd-power is an old application and has many users around
> which are currently using this app in interrupt only mode without giving
> an extra argument. But suddenly they will start getting failure messages with
> the new patchset.
> 
> My only intent with else condition was backward compatibility.
> Or may be we can have more descriptive failure message, something like
> "init_power_library failed, check manual for other modes".
> 
> Thanks
> Harman
> 
> 

I think we can compormise on an informative log message suggesting to 
use interrupt mode. I'm not keen on reverting to previous buggy behavior :)

>> -- 
>> Thanks,
>> Anatoly


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Harman Kalra]

On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
> On 30-May-20 11:02 AM, Harman Kalra wrote:
> > On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> > > External Email
> > > 
> > > ----------------------------------------------------------------------
> > > On 29-May-20 2:19 PM, Harman Kalra wrote:
> > > 
> > > > >    	if (ret < 0)
> > > > >    		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> > > > > -	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> > > > > +	if (app_mode == APP_MODE_DEFAULT)
> > > > > +		app_mode = APP_MODE_LEGACY;
> > > > > +
> > > > > +	/* only legacy and empty poll mode rely on power library */
> > > > > +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> > > > > +			init_power_library())
> > > > >    		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> > > > Hi,
> > > > 
> > > > Rather than just exiting from here can we have a else condition to
> > > > automatically enter into the "interrupt only" mode.
> > > > Please correct me if I am missing something.
> > > 
> > > Hi,
> > > 
> > > Thanks for your review. I don't think silently proceeding is a good idea. If
> > > the user wants interrupt-only mode, they should request it. Silently falling
> > > back to interrupt-only mode will create an illusion of successful
> > > initialization and set the wrong expectation for how the application will
> > > behave.
> > > 
> > 
> > Hi,
> > 
> > Thanks for the explanation which even I also believe is logically perfect.
> > 
> > But since l3fwd-power is an old application and has many users around
> > which are currently using this app in interrupt only mode without giving
> > an extra argument. But suddenly they will start getting failure messages with
> > the new patchset.
> > 
> > My only intent with else condition was backward compatibility.
> > Or may be we can have more descriptive failure message, something like
> > "init_power_library failed, check manual for other modes".
> > 
> > Thanks
> > Harman
> > 
> > 
> 
> I think we can compormise on an informative log message suggesting to use
> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
> 
Hi

I am not insisting to revert to previous behavior, I am just trying to
highlight some probable issues that many users might face as its an old
application.
Since many arm based soc might not be supporting frequency scaling, can
we add the following check as soon as the application starts, probe the
platform if it supports frequency scaling, if not automatically set the
mode to interrupt mode, something like:
if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
			F_OK))
	app_mode = APP_MODE_INTERRUPT;


Thanks
Harman

> > > -- 
> > > Thanks,
> > > Anatoly
> 
> 
> -- 
> Thanks,
> Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Harman Kalra]

On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
> > On 30-May-20 11:02 AM, Harman Kalra wrote:
> > > On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> > > > External Email
> > > > 
> > > > ----------------------------------------------------------------------
> > > > On 29-May-20 2:19 PM, Harman Kalra wrote:
> > > > 
> > > > > >    	if (ret < 0)
> > > > > >    		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> > > > > > -	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> > > > > > +	if (app_mode == APP_MODE_DEFAULT)
> > > > > > +		app_mode = APP_MODE_LEGACY;
> > > > > > +
> > > > > > +	/* only legacy and empty poll mode rely on power library */
> > > > > > +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> > > > > > +			init_power_library())
> > > > > >    		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> > > > > Hi,
> > > > > 
> > > > > Rather than just exiting from here can we have a else condition to
> > > > > automatically enter into the "interrupt only" mode.
> > > > > Please correct me if I am missing something.
> > > > 
> > > > Hi,
> > > > 
> > > > Thanks for your review. I don't think silently proceeding is a good idea. If
> > > > the user wants interrupt-only mode, they should request it. Silently falling
> > > > back to interrupt-only mode will create an illusion of successful
> > > > initialization and set the wrong expectation for how the application will
> > > > behave.
> > > > 
> > > 
> > > Hi,
> > > 
> > > Thanks for the explanation which even I also believe is logically perfect.
> > > 
> > > But since l3fwd-power is an old application and has many users around
> > > which are currently using this app in interrupt only mode without giving
> > > an extra argument. But suddenly they will start getting failure messages with
> > > the new patchset.
> > > 
> > > My only intent with else condition was backward compatibility.
> > > Or may be we can have more descriptive failure message, something like
> > > "init_power_library failed, check manual for other modes".
> > > 
> > > Thanks
> > > Harman
> > > 
> > > 
> > 
> > I think we can compormise on an informative log message suggesting to use
> > interrupt mode. I'm not keen on reverting to previous buggy behavior :)
> > 
> Hi
> 
> I am not insisting to revert to previous behavior, I am just trying to
> highlight some probable issues that many users might face as its an old
> application.
> Since many arm based soc might not be supporting frequency scaling, can
> we add the following check as soon as the application starts, probe the
> platform if it supports frequency scaling, if not automatically set the
> mode to interrupt mode, something like:
> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> 			F_OK))
> 	app_mode = APP_MODE_INTERRUPT;

Sorry, no direct check in application but we can introduce a new API in
power library:
   bool rte_is_freq_scaling() {
	   return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
			   F_OK);
   }

and in the application we can use "rte_is_freq_scaling()" at the start.

> 
> 
> Thanks
> Harman
> 
> > > > -- 
> > > > Thanks,
> > > > Anatoly
> > 
> > 
> > -- 
> > Thanks,
> > Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Burakov, Anatoly]

On 02-Jun-20 1:16 PM, Harman Kalra wrote:
> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
>>>>> External Email
>>>>>
>>>>> ----------------------------------------------------------------------
>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
>>>>>
>>>>>>>     	if (ret < 0)
>>>>>>>     		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>>>>>>> -	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
>>>>>>> +	if (app_mode == APP_MODE_DEFAULT)
>>>>>>> +		app_mode = APP_MODE_LEGACY;
>>>>>>> +
>>>>>>> +	/* only legacy and empty poll mode rely on power library */
>>>>>>> +	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
>>>>>>> +			init_power_library())
>>>>>>>     		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
>>>>>> Hi,
>>>>>>
>>>>>> Rather than just exiting from here can we have a else condition to
>>>>>> automatically enter into the "interrupt only" mode.
>>>>>> Please correct me if I am missing something.
>>>>>
>>>>> Hi,
>>>>>
>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
>>>>> the user wants interrupt-only mode, they should request it. Silently falling
>>>>> back to interrupt-only mode will create an illusion of successful
>>>>> initialization and set the wrong expectation for how the application will
>>>>> behave.
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Thanks for the explanation which even I also believe is logically perfect.
>>>>
>>>> But since l3fwd-power is an old application and has many users around
>>>> which are currently using this app in interrupt only mode without giving
>>>> an extra argument. But suddenly they will start getting failure messages with
>>>> the new patchset.
>>>>
>>>> My only intent with else condition was backward compatibility.
>>>> Or may be we can have more descriptive failure message, something like
>>>> "init_power_library failed, check manual for other modes".
>>>>
>>>> Thanks
>>>> Harman
>>>>
>>>>
>>>
>>> I think we can compormise on an informative log message suggesting to use
>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
>>>
>> Hi
>>
>> I am not insisting to revert to previous behavior, I am just trying to
>> highlight some probable issues that many users might face as its an old
>> application.
>> Since many arm based soc might not be supporting frequency scaling, can
>> we add the following check as soon as the application starts, probe the
>> platform if it supports frequency scaling, if not automatically set the
>> mode to interrupt mode, something like:
>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
>> 			F_OK))
>> 	app_mode = APP_MODE_INTERRUPT;
> 
> Sorry, no direct check in application but we can introduce a new API in
> power library:
>     bool rte_is_freq_scaling() {
> 	   return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> 			   F_OK);
>     }
> 
> and in the application we can use "rte_is_freq_scaling()" at the start.
> 

What you're suggesting here is effectively what you have already 
suggested: silently fall back to interrupt-only mode if power lib init 
failed. I already outlined why i don't think it's a good approach.

>>
>>
>> Thanks
>> Harman
>>
>>>>> -- 
>>>>> Thanks,
>>>>> Anatoly
>>>
>>>
>>> -- 
>>> Thanks,
>>> Anatoly


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Jerin Jacob]

On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
> > On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
> >> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
> >>> On 30-May-20 11:02 AM, Harman Kalra wrote:
> >>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> >>>>> External Email
> >>>>>
> >>>>> ----------------------------------------------------------------------
> >>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
> >>>>>
> >>>>>>>         if (ret < 0)
> >>>>>>>                 rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> >>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> >>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
> >>>>>>> +               app_mode = APP_MODE_LEGACY;
> >>>>>>> +
> >>>>>>> +       /* only legacy and empty poll mode rely on power library */
> >>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> >>>>>>> +                       init_power_library())
> >>>>>>>                 rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> >>>>>> Hi,
> >>>>>>
> >>>>>> Rather than just exiting from here can we have a else condition to
> >>>>>> automatically enter into the "interrupt only" mode.
> >>>>>> Please correct me if I am missing something.
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
> >>>>> the user wants interrupt-only mode, they should request it. Silently falling
> >>>>> back to interrupt-only mode will create an illusion of successful
> >>>>> initialization and set the wrong expectation for how the application will
> >>>>> behave.
> >>>>>
> >>>>
> >>>> Hi,
> >>>>
> >>>> Thanks for the explanation which even I also believe is logically perfect.
> >>>>
> >>>> But since l3fwd-power is an old application and has many users around
> >>>> which are currently using this app in interrupt only mode without giving
> >>>> an extra argument. But suddenly they will start getting failure messages with
> >>>> the new patchset.
> >>>>
> >>>> My only intent with else condition was backward compatibility.
> >>>> Or may be we can have more descriptive failure message, something like
> >>>> "init_power_library failed, check manual for other modes".
> >>>>
> >>>> Thanks
> >>>> Harman
> >>>>
> >>>>
> >>>
> >>> I think we can compormise on an informative log message suggesting to use
> >>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
> >>>
> >> Hi
> >>
> >> I am not insisting to revert to previous behavior, I am just trying to
> >> highlight some probable issues that many users might face as its an old
> >> application.
> >> Since many arm based soc might not be supporting frequency scaling, can
> >> we add the following check as soon as the application starts, probe the
> >> platform if it supports frequency scaling, if not automatically set the
> >> mode to interrupt mode, something like:
> >> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>                      F_OK))
> >>      app_mode = APP_MODE_INTERRUPT;
> >
> > Sorry, no direct check in application but we can introduce a new API in
> > power library:
> >     bool rte_is_freq_scaling() {
> >          return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >                          F_OK);
> >     }
> >
> > and in the application we can use "rte_is_freq_scaling()" at the start.
> >
>
> What you're suggesting here is effectively what you have already
> suggested: silently fall back to interrupt-only mode if power lib init
> failed. I already outlined why i don't think it's a good approach.

Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
file presence,
detects the power lib availability . Right?  Not the failure. Right?
IMO, it make sense to have following case:
# first check, Is the system is capable of power lib if so use power lib
# if the system is not capable of using power lib use interrupt mode.

I think, there is difference between system capable of using power lib
vs power lib not available or power lib failure.




>
> >>
> >>
> >> Thanks
> >> Harman
> >>
> >>>>> --
> >>>>> Thanks,
> >>>>> Anatoly
> >>>
> >>>
> >>> --
> >>> Thanks,
> >>> Anatoly
>
>
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Burakov, Anatoly]

On 15-Jun-20 12:43 PM, Jerin Jacob wrote:
> On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>
>> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
>>> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
>>>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
>>>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
>>>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
>>>>>>> External Email
>>>>>>>
>>>>>>> ----------------------------------------------------------------------
>>>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
>>>>>>>
>>>>>>>>>          if (ret < 0)
>>>>>>>>>                  rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>>>>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
>>>>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
>>>>>>>>> +               app_mode = APP_MODE_LEGACY;
>>>>>>>>> +
>>>>>>>>> +       /* only legacy and empty poll mode rely on power library */
>>>>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
>>>>>>>>> +                       init_power_library())
>>>>>>>>>                  rte_exit(EXIT_FAILURE, "init_power_library failed\n");
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Rather than just exiting from here can we have a else condition to
>>>>>>>> automatically enter into the "interrupt only" mode.
>>>>>>>> Please correct me if I am missing something.
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
>>>>>>> the user wants interrupt-only mode, they should request it. Silently falling
>>>>>>> back to interrupt-only mode will create an illusion of successful
>>>>>>> initialization and set the wrong expectation for how the application will
>>>>>>> behave.
>>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Thanks for the explanation which even I also believe is logically perfect.
>>>>>>
>>>>>> But since l3fwd-power is an old application and has many users around
>>>>>> which are currently using this app in interrupt only mode without giving
>>>>>> an extra argument. But suddenly they will start getting failure messages with
>>>>>> the new patchset.
>>>>>>
>>>>>> My only intent with else condition was backward compatibility.
>>>>>> Or may be we can have more descriptive failure message, something like
>>>>>> "init_power_library failed, check manual for other modes".
>>>>>>
>>>>>> Thanks
>>>>>> Harman
>>>>>>
>>>>>>
>>>>>
>>>>> I think we can compormise on an informative log message suggesting to use
>>>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
>>>>>
>>>> Hi
>>>>
>>>> I am not insisting to revert to previous behavior, I am just trying to
>>>> highlight some probable issues that many users might face as its an old
>>>> application.
>>>> Since many arm based soc might not be supporting frequency scaling, can
>>>> we add the following check as soon as the application starts, probe the
>>>> platform if it supports frequency scaling, if not automatically set the
>>>> mode to interrupt mode, something like:
>>>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
>>>>                       F_OK))
>>>>       app_mode = APP_MODE_INTERRUPT;
>>>
>>> Sorry, no direct check in application but we can introduce a new API in
>>> power library:
>>>      bool rte_is_freq_scaling() {
>>>           return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
>>>                           F_OK);
>>>      }
>>>
>>> and in the application we can use "rte_is_freq_scaling()" at the start.
>>>
>>
>> What you're suggesting here is effectively what you have already
>> suggested: silently fall back to interrupt-only mode if power lib init
>> failed. I already outlined why i don't think it's a good approach.
> 
> Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
> file presence,
> detects the power lib availability . Right?  Not the failure. Right?
> IMO, it make sense to have following case:
> # first check, Is the system is capable of power lib if so use power lib
> # if the system is not capable of using power lib use interrupt mode.
> 
> I think, there is difference between system capable of using power lib
> vs power lib not available or power lib failure.

I am of the opinion that if a test sets up an unrealistic expectation of 
how an application should behave, it's a problem with the test, not with 
the application.

If the system is not capable of running with power lib - the application 
shouldn't be requested to run in such mode.

"The application behaved that way before" - yes, it did. It was a bug in 
the application, that it allowed users to effectively misuse the 
application and use it despite the fact that it was in a half-working 
state. This problem has been addressed by 1) not allowing the 
application to run in half-working state, and 2) adding a new mode where 
the old "expected" behavior is *actually* expected and is "full working 
state" now.

Therefore, all users who were previously misusing the application to do 
something it was not designed to do because of a bug in the 
implementation, should now fix their usage and use the correct mode - 
and such breakage is IMO necessary to call attention to earlier misuse 
in the tools, and to correct this usage.

What bothers me about your suggestion is that it is impossible to fail 
the test if the wrong mode was requested (as in, if we request the 
power-lib mode on a system that doesn't have freq scaling) - it instead 
silently falls back to a mode that is almost guaranteed to work.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Jerin Jacob]

On Mon, Jun 15, 2020 at 8:35 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 15-Jun-20 12:43 PM, Jerin Jacob wrote:
> > On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
> > <anatoly.burakov@intel.com> wrote:
> >>
> >> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
> >>> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
> >>>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
> >>>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
> >>>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> >>>>>>> External Email
> >>>>>>>
> >>>>>>> ----------------------------------------------------------------------
> >>>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
> >>>>>>>
> >>>>>>>>>          if (ret < 0)
> >>>>>>>>>                  rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> >>>>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> >>>>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
> >>>>>>>>> +               app_mode = APP_MODE_LEGACY;
> >>>>>>>>> +
> >>>>>>>>> +       /* only legacy and empty poll mode rely on power library */
> >>>>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> >>>>>>>>> +                       init_power_library())
> >>>>>>>>>                  rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> Rather than just exiting from here can we have a else condition to
> >>>>>>>> automatically enter into the "interrupt only" mode.
> >>>>>>>> Please correct me if I am missing something.
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
> >>>>>>> the user wants interrupt-only mode, they should request it. Silently falling
> >>>>>>> back to interrupt-only mode will create an illusion of successful
> >>>>>>> initialization and set the wrong expectation for how the application will
> >>>>>>> behave.
> >>>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Thanks for the explanation which even I also believe is logically perfect.
> >>>>>>
> >>>>>> But since l3fwd-power is an old application and has many users around
> >>>>>> which are currently using this app in interrupt only mode without giving
> >>>>>> an extra argument. But suddenly they will start getting failure messages with
> >>>>>> the new patchset.
> >>>>>>
> >>>>>> My only intent with else condition was backward compatibility.
> >>>>>> Or may be we can have more descriptive failure message, something like
> >>>>>> "init_power_library failed, check manual for other modes".
> >>>>>>
> >>>>>> Thanks
> >>>>>> Harman
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> I think we can compormise on an informative log message suggesting to use
> >>>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
> >>>>>
> >>>> Hi
> >>>>
> >>>> I am not insisting to revert to previous behavior, I am just trying to
> >>>> highlight some probable issues that many users might face as its an old
> >>>> application.
> >>>> Since many arm based soc might not be supporting frequency scaling, can
> >>>> we add the following check as soon as the application starts, probe the
> >>>> platform if it supports frequency scaling, if not automatically set the
> >>>> mode to interrupt mode, something like:
> >>>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>>                       F_OK))
> >>>>       app_mode = APP_MODE_INTERRUPT;
> >>>
> >>> Sorry, no direct check in application but we can introduce a new API in
> >>> power library:
> >>>      bool rte_is_freq_scaling() {
> >>>           return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>                           F_OK);
> >>>      }
> >>>
> >>> and in the application we can use "rte_is_freq_scaling()" at the start.
> >>>
> >>
> >> What you're suggesting here is effectively what you have already
> >> suggested: silently fall back to interrupt-only mode if power lib init
> >> failed. I already outlined why i don't think it's a good approach.
> >
> > Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
> > file presence,
> > detects the power lib availability . Right?  Not the failure. Right?
> > IMO, it make sense to have following case:
> > # first check, Is the system is capable of power lib if so use power lib
> > # if the system is not capable of using power lib use interrupt mode.
> >
> > I think, there is difference between system capable of using power lib
> > vs power lib not available or power lib failure.
>
> I am of the opinion that if a test sets up an unrealistic expectation of
> how an application should behave, it's a problem with the test, not with
> the application.
>
> If the system is not capable of running with power lib - the application
> shouldn't be requested to run in such mode.

But with this patch, default proposed mode is power lib without any
explicit request.

>
> "The application behaved that way before" - yes, it did. It was a bug in
> the application, that it allowed users to effectively misuse the
> application and use it despite the fact that it was in a half-working
> state. This problem has been addressed by 1) not allowing the
> application to run in half-working state, and 2) adding a new mode where
> the old "expected" behavior is *actually* expected and is "full working
> state" now.
>
> Therefore, all users who were previously misusing the application to do
> something it was not designed to do because of a bug in the
> implementation, should now fix their usage and use the correct mode -
> and such breakage is IMO necessary to call attention to earlier misuse
> in the tools, and to correct this usage.
>
> What bothers me about your suggestion is that it is impossible to fail
> the test if the wrong mode was requested (as in, if we request the
> power-lib mode on a system that doesn't have freq scaling) - it instead
> silently falls back to a mode that is almost guaranteed to work.

I agree that it should fail, i.e someone explicitly request,
power-lib mode or any mode
and it should fail application if the platform we can not do that.

My suggestion is all about, what is the default, IMO, if no argument
is specified,
the application should _probe_ the capability of the platfrom and
choose the mode. One can override
the probe to select the desired one. In such mode, fail to configure
the mode should result in
an error.


>
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Burakov, Anatoly]

On 15-Jun-20 4:21 PM, Jerin Jacob wrote:
> On Mon, Jun 15, 2020 at 8:35 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>
>> On 15-Jun-20 12:43 PM, Jerin Jacob wrote:
>>> On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
>>> <anatoly.burakov@intel.com> wrote:
>>>>
>>>> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
>>>>> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
>>>>>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
>>>>>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
>>>>>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
>>>>>>>>> External Email
>>>>>>>>>
>>>>>>>>> ----------------------------------------------------------------------
>>>>>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
>>>>>>>>>
>>>>>>>>>>>           if (ret < 0)
>>>>>>>>>>>                   rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>>>>>>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
>>>>>>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
>>>>>>>>>>> +               app_mode = APP_MODE_LEGACY;
>>>>>>>>>>> +
>>>>>>>>>>> +       /* only legacy and empty poll mode rely on power library */
>>>>>>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
>>>>>>>>>>> +                       init_power_library())
>>>>>>>>>>>                   rte_exit(EXIT_FAILURE, "init_power_library failed\n");
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Rather than just exiting from here can we have a else condition to
>>>>>>>>>> automatically enter into the "interrupt only" mode.
>>>>>>>>>> Please correct me if I am missing something.
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
>>>>>>>>> the user wants interrupt-only mode, they should request it. Silently falling
>>>>>>>>> back to interrupt-only mode will create an illusion of successful
>>>>>>>>> initialization and set the wrong expectation for how the application will
>>>>>>>>> behave.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Thanks for the explanation which even I also believe is logically perfect.
>>>>>>>>
>>>>>>>> But since l3fwd-power is an old application and has many users around
>>>>>>>> which are currently using this app in interrupt only mode without giving
>>>>>>>> an extra argument. But suddenly they will start getting failure messages with
>>>>>>>> the new patchset.
>>>>>>>>
>>>>>>>> My only intent with else condition was backward compatibility.
>>>>>>>> Or may be we can have more descriptive failure message, something like
>>>>>>>> "init_power_library failed, check manual for other modes".
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Harman
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> I think we can compormise on an informative log message suggesting to use
>>>>>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
>>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> I am not insisting to revert to previous behavior, I am just trying to
>>>>>> highlight some probable issues that many users might face as its an old
>>>>>> application.
>>>>>> Since many arm based soc might not be supporting frequency scaling, can
>>>>>> we add the following check as soon as the application starts, probe the
>>>>>> platform if it supports frequency scaling, if not automatically set the
>>>>>> mode to interrupt mode, something like:
>>>>>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
>>>>>>                        F_OK))
>>>>>>        app_mode = APP_MODE_INTERRUPT;
>>>>>
>>>>> Sorry, no direct check in application but we can introduce a new API in
>>>>> power library:
>>>>>       bool rte_is_freq_scaling() {
>>>>>            return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
>>>>>                            F_OK);
>>>>>       }
>>>>>
>>>>> and in the application we can use "rte_is_freq_scaling()" at the start.
>>>>>
>>>>
>>>> What you're suggesting here is effectively what you have already
>>>> suggested: silently fall back to interrupt-only mode if power lib init
>>>> failed. I already outlined why i don't think it's a good approach.
>>>
>>> Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
>>> file presence,
>>> detects the power lib availability . Right?  Not the failure. Right?
>>> IMO, it make sense to have following case:
>>> # first check, Is the system is capable of power lib if so use power lib
>>> # if the system is not capable of using power lib use interrupt mode.
>>>
>>> I think, there is difference between system capable of using power lib
>>> vs power lib not available or power lib failure.
>>
>> I am of the opinion that if a test sets up an unrealistic expectation of
>> how an application should behave, it's a problem with the test, not with
>> the application.
>>
>> If the system is not capable of running with power lib - the application
>> shouldn't be requested to run in such mode.
> 
> But with this patch, default proposed mode is power lib without any
> explicit request.

The default has always been "use the power library". That was always the 
expected behavior, i believe since the very first version of this 
application. In other words, even if the "requested" behavior is not 
requested explicitly, it has always been that implicitly, and this patch 
is *keeping existing behavior*.

> 
>>
>> "The application behaved that way before" - yes, it did. It was a bug in
>> the application, that it allowed users to effectively misuse the
>> application and use it despite the fact that it was in a half-working
>> state. This problem has been addressed by 1) not allowing the
>> application to run in half-working state, and 2) adding a new mode where
>> the old "expected" behavior is *actually* expected and is "full working
>> state" now.
>>
>> Therefore, all users who were previously misusing the application to do
>> something it was not designed to do because of a bug in the
>> implementation, should now fix their usage and use the correct mode -
>> and such breakage is IMO necessary to call attention to earlier misuse
>> in the tools, and to correct this usage.
>>
>> What bothers me about your suggestion is that it is impossible to fail
>> the test if the wrong mode was requested (as in, if we request the
>> power-lib mode on a system that doesn't have freq scaling) - it instead
>> silently falls back to a mode that is almost guaranteed to work.
> 
> I agree that it should fail, i.e someone explicitly request,
> power-lib mode or any mode
> and it should fail application if the platform we can not do that.
> 
> My suggestion is all about, what is the default, IMO, if no argument
> is specified,
> the application should _probe_ the capability of the platfrom and
> choose the mode. One can override
> the probe to select the desired one. In such mode, fail to configure
> the mode should result in
> an error.

This would change the default behavior that has always existed with this 
application, and would still be subject to silent failure issue 
*because* older tests may not account for this implied assumption of 
"the application will run no matter what", leading to a possible false 
positive test result.

Now, if the default was "not to run and ask explicitly what mode should 
the user use" - i can see how we could both agree on that. It's not 
unprecedented - l3fwd itself won't run unless you explicitly specify 
core config, so we *could* request additional parameters by default. I 
would've also agreed with the "probe" thing *if it was a new application 
without any pre-existing usages* - but it isn't, and in this case IMO 
this is doubly important because there may be tests out there that *rely 
on a bug* and thus should be explicitly broken and addressed (like the 
internal test we had that uncovered the issue in the first place).

In other words, in the perfect world, i would agree with you :) As it 
stands though, i think that intentionally breaking tests that are 
themselves broken and rely on wrong assumptions is more important than 
keeping them working.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Jerin Jacob]

On Mon, Jun 15, 2020 at 9:15 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 15-Jun-20 4:21 PM, Jerin Jacob wrote:
> > On Mon, Jun 15, 2020 at 8:35 PM Burakov, Anatoly
> > <anatoly.burakov@intel.com> wrote:
> >>
> >> On 15-Jun-20 12:43 PM, Jerin Jacob wrote:
> >>> On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
> >>> <anatoly.burakov@intel.com> wrote:
> >>>>
> >>>> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
> >>>>> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
> >>>>>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
> >>>>>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
> >>>>>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> >>>>>>>>> External Email
> >>>>>>>>>
> >>>>>>>>> ----------------------------------------------------------------------
> >>>>>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
> >>>>>>>>>
> >>>>>>>>>>>           if (ret < 0)
> >>>>>>>>>>>                   rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> >>>>>>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> >>>>>>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
> >>>>>>>>>>> +               app_mode = APP_MODE_LEGACY;
> >>>>>>>>>>> +
> >>>>>>>>>>> +       /* only legacy and empty poll mode rely on power library */
> >>>>>>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> >>>>>>>>>>> +                       init_power_library())
> >>>>>>>>>>>                   rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> Rather than just exiting from here can we have a else condition to
> >>>>>>>>>> automatically enter into the "interrupt only" mode.
> >>>>>>>>>> Please correct me if I am missing something.
> >>>>>>>>>
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
> >>>>>>>>> the user wants interrupt-only mode, they should request it. Silently falling
> >>>>>>>>> back to interrupt-only mode will create an illusion of successful
> >>>>>>>>> initialization and set the wrong expectation for how the application will
> >>>>>>>>> behave.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> Thanks for the explanation which even I also believe is logically perfect.
> >>>>>>>>
> >>>>>>>> But since l3fwd-power is an old application and has many users around
> >>>>>>>> which are currently using this app in interrupt only mode without giving
> >>>>>>>> an extra argument. But suddenly they will start getting failure messages with
> >>>>>>>> the new patchset.
> >>>>>>>>
> >>>>>>>> My only intent with else condition was backward compatibility.
> >>>>>>>> Or may be we can have more descriptive failure message, something like
> >>>>>>>> "init_power_library failed, check manual for other modes".
> >>>>>>>>
> >>>>>>>> Thanks
> >>>>>>>> Harman
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>> I think we can compormise on an informative log message suggesting to use
> >>>>>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
> >>>>>>>
> >>>>>> Hi
> >>>>>>
> >>>>>> I am not insisting to revert to previous behavior, I am just trying to
> >>>>>> highlight some probable issues that many users might face as its an old
> >>>>>> application.
> >>>>>> Since many arm based soc might not be supporting frequency scaling, can
> >>>>>> we add the following check as soon as the application starts, probe the
> >>>>>> platform if it supports frequency scaling, if not automatically set the
> >>>>>> mode to interrupt mode, something like:
> >>>>>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>>>>                        F_OK))
> >>>>>>        app_mode = APP_MODE_INTERRUPT;
> >>>>>
> >>>>> Sorry, no direct check in application but we can introduce a new API in
> >>>>> power library:
> >>>>>       bool rte_is_freq_scaling() {
> >>>>>            return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>>>                            F_OK);
> >>>>>       }
> >>>>>
> >>>>> and in the application we can use "rte_is_freq_scaling()" at the start.
> >>>>>
> >>>>
> >>>> What you're suggesting here is effectively what you have already
> >>>> suggested: silently fall back to interrupt-only mode if power lib init
> >>>> failed. I already outlined why i don't think it's a good approach.
> >>>
> >>> Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
> >>> file presence,
> >>> detects the power lib availability . Right?  Not the failure. Right?
> >>> IMO, it make sense to have following case:
> >>> # first check, Is the system is capable of power lib if so use power lib
> >>> # if the system is not capable of using power lib use interrupt mode.
> >>>
> >>> I think, there is difference between system capable of using power lib
> >>> vs power lib not available or power lib failure.
> >>
> >> I am of the opinion that if a test sets up an unrealistic expectation of
> >> how an application should behave, it's a problem with the test, not with
> >> the application.
> >>
> >> If the system is not capable of running with power lib - the application
> >> shouldn't be requested to run in such mode.
> >
> > But with this patch, default proposed mode is power lib without any
> > explicit request.
>
> The default has always been "use the power library". That was always the
> expected behavior, i believe since the very first version of this
> application. In other words, even if the "requested" behavior is not
> requested explicitly, it has always been that implicitly, and this patch
> is *keeping existing behavior*.

See below,
>
> >
> >>
> >> "The application behaved that way before" - yes, it did. It was a bug in
> >> the application, that it allowed users to effectively misuse the
> >> application and use it despite the fact that it was in a half-working
> >> state. This problem has been addressed by 1) not allowing the
> >> application to run in half-working state, and 2) adding a new mode where
> >> the old "expected" behavior is *actually* expected and is "full working
> >> state" now.
> >>
> >> Therefore, all users who were previously misusing the application to do
> >> something it was not designed to do because of a bug in the
> >> implementation, should now fix their usage and use the correct mode -
> >> and such breakage is IMO necessary to call attention to earlier misuse
> >> in the tools, and to correct this usage.
> >>
> >> What bothers me about your suggestion is that it is impossible to fail
> >> the test if the wrong mode was requested (as in, if we request the
> >> power-lib mode on a system that doesn't have freq scaling) - it instead
> >> silently falls back to a mode that is almost guaranteed to work.
> >
> > I agree that it should fail, i.e someone explicitly request,
> > power-lib mode or any mode
> > and it should fail application if the platform we can not do that.
> >
> > My suggestion is all about, what is the default, IMO, if no argument
> > is specified,
> > the application should _probe_ the capability of the platfrom and
> > choose the mode. One can override
> > the probe to select the desired one. In such mode, fail to configure
> > the mode should result in
> > an error.
>
> This would change the default behavior that has always existed with this
> application, and would still be subject to silent failure issue
> *because* older tests may not account for this implied assumption of
> "the application will run no matter what", leading to a possible false
> positive test result.
>
> Now, if the default was "not to run and ask explicitly what mode should
> the user use" - i can see how we could both agree on that. It's not
> unprecedented - l3fwd itself won't run unless you explicitly specify
> core config, so we *could* request additional parameters by default. I
> would've also agreed with the "probe" thing *if it was a new application
> without any pre-existing usages* - but it isn't, and in this case IMO
> this is doubly important because there may be tests out there that *rely
> on a bug* and thus should be explicitly broken and addressed (like the
> internal test we had that uncovered the issue in the first place).
>
> In other words, in the perfect world, i would agree with you :) As it
> stands though, i think that intentionally breaking tests that are
> themselves broken and rely on wrong assumptions is more important than
> keeping them working.

OK. Let's enumerate the pros and cons of both approaches?
Approach a: Auto probe
Approach b: Current patch

Approach a:
+ Application picks up the mode based on platform capability
+ No change in behavior wrt existing l3fwd-power where the platform
has only interrupt support.
(otherwise, It will fail to boot up, the CI etc we need to patch based
on the DPDK version)

I am not sure approach b has pros wrt approach a.

I.e On the x86 platform where freq scaling is present then SHOULD NOT
have any difference in the approach a vs
approach b. ie. Auto probe finds the system is capable of freq scaling
and picks the powerlib. its is win-win case,
I am not sure, What I am missing?








>
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Burakov, Anatoly]

On 15-Jun-20 5:29 PM, Jerin Jacob wrote:
> On Mon, Jun 15, 2020 at 9:15 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>
>> On 15-Jun-20 4:21 PM, Jerin Jacob wrote:
>>> On Mon, Jun 15, 2020 at 8:35 PM Burakov, Anatoly
>>> <anatoly.burakov@intel.com> wrote:
>>>>
>>>> On 15-Jun-20 12:43 PM, Jerin Jacob wrote:
>>>>> On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
>>>>> <anatoly.burakov@intel.com> wrote:
>>>>>>
>>>>>> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
>>>>>>> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
>>>>>>>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
>>>>>>>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
>>>>>>>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
>>>>>>>>>>> External Email
>>>>>>>>>>>
>>>>>>>>>>> ----------------------------------------------------------------------
>>>>>>>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
>>>>>>>>>>>
>>>>>>>>>>>>>            if (ret < 0)
>>>>>>>>>>>>>                    rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>>>>>>>>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
>>>>>>>>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
>>>>>>>>>>>>> +               app_mode = APP_MODE_LEGACY;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +       /* only legacy and empty poll mode rely on power library */
>>>>>>>>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
>>>>>>>>>>>>> +                       init_power_library())
>>>>>>>>>>>>>                    rte_exit(EXIT_FAILURE, "init_power_library failed\n");
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Rather than just exiting from here can we have a else condition to
>>>>>>>>>>>> automatically enter into the "interrupt only" mode.
>>>>>>>>>>>> Please correct me if I am missing something.
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
>>>>>>>>>>> the user wants interrupt-only mode, they should request it. Silently falling
>>>>>>>>>>> back to interrupt-only mode will create an illusion of successful
>>>>>>>>>>> initialization and set the wrong expectation for how the application will
>>>>>>>>>>> behave.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Thanks for the explanation which even I also believe is logically perfect.
>>>>>>>>>>
>>>>>>>>>> But since l3fwd-power is an old application and has many users around
>>>>>>>>>> which are currently using this app in interrupt only mode without giving
>>>>>>>>>> an extra argument. But suddenly they will start getting failure messages with
>>>>>>>>>> the new patchset.
>>>>>>>>>>
>>>>>>>>>> My only intent with else condition was backward compatibility.
>>>>>>>>>> Or may be we can have more descriptive failure message, something like
>>>>>>>>>> "init_power_library failed, check manual for other modes".
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Harman
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think we can compormise on an informative log message suggesting to use
>>>>>>>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
>>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> I am not insisting to revert to previous behavior, I am just trying to
>>>>>>>> highlight some probable issues that many users might face as its an old
>>>>>>>> application.
>>>>>>>> Since many arm based soc might not be supporting frequency scaling, can
>>>>>>>> we add the following check as soon as the application starts, probe the
>>>>>>>> platform if it supports frequency scaling, if not automatically set the
>>>>>>>> mode to interrupt mode, something like:
>>>>>>>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
>>>>>>>>                         F_OK))
>>>>>>>>         app_mode = APP_MODE_INTERRUPT;
>>>>>>>
>>>>>>> Sorry, no direct check in application but we can introduce a new API in
>>>>>>> power library:
>>>>>>>        bool rte_is_freq_scaling() {
>>>>>>>             return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
>>>>>>>                             F_OK);
>>>>>>>        }
>>>>>>>
>>>>>>> and in the application we can use "rte_is_freq_scaling()" at the start.
>>>>>>>
>>>>>>
>>>>>> What you're suggesting here is effectively what you have already
>>>>>> suggested: silently fall back to interrupt-only mode if power lib init
>>>>>> failed. I already outlined why i don't think it's a good approach.
>>>>>
>>>>> Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
>>>>> file presence,
>>>>> detects the power lib availability . Right?  Not the failure. Right?
>>>>> IMO, it make sense to have following case:
>>>>> # first check, Is the system is capable of power lib if so use power lib
>>>>> # if the system is not capable of using power lib use interrupt mode.
>>>>>
>>>>> I think, there is difference between system capable of using power lib
>>>>> vs power lib not available or power lib failure.
>>>>
>>>> I am of the opinion that if a test sets up an unrealistic expectation of
>>>> how an application should behave, it's a problem with the test, not with
>>>> the application.
>>>>
>>>> If the system is not capable of running with power lib - the application
>>>> shouldn't be requested to run in such mode.
>>>
>>> But with this patch, default proposed mode is power lib without any
>>> explicit request.
>>
>> The default has always been "use the power library". That was always the
>> expected behavior, i believe since the very first version of this
>> application. In other words, even if the "requested" behavior is not
>> requested explicitly, it has always been that implicitly, and this patch
>> is *keeping existing behavior*.
> 
> See below,
>>
>>>
>>>>
>>>> "The application behaved that way before" - yes, it did. It was a bug in
>>>> the application, that it allowed users to effectively misuse the
>>>> application and use it despite the fact that it was in a half-working
>>>> state. This problem has been addressed by 1) not allowing the
>>>> application to run in half-working state, and 2) adding a new mode where
>>>> the old "expected" behavior is *actually* expected and is "full working
>>>> state" now.
>>>>
>>>> Therefore, all users who were previously misusing the application to do
>>>> something it was not designed to do because of a bug in the
>>>> implementation, should now fix their usage and use the correct mode -
>>>> and such breakage is IMO necessary to call attention to earlier misuse
>>>> in the tools, and to correct this usage.
>>>>
>>>> What bothers me about your suggestion is that it is impossible to fail
>>>> the test if the wrong mode was requested (as in, if we request the
>>>> power-lib mode on a system that doesn't have freq scaling) - it instead
>>>> silently falls back to a mode that is almost guaranteed to work.
>>>
>>> I agree that it should fail, i.e someone explicitly request,
>>> power-lib mode or any mode
>>> and it should fail application if the platform we can not do that.
>>>
>>> My suggestion is all about, what is the default, IMO, if no argument
>>> is specified,
>>> the application should _probe_ the capability of the platfrom and
>>> choose the mode. One can override
>>> the probe to select the desired one. In such mode, fail to configure
>>> the mode should result in
>>> an error.
>>
>> This would change the default behavior that has always existed with this
>> application, and would still be subject to silent failure issue
>> *because* older tests may not account for this implied assumption of
>> "the application will run no matter what", leading to a possible false
>> positive test result.
>>
>> Now, if the default was "not to run and ask explicitly what mode should
>> the user use" - i can see how we could both agree on that. It's not
>> unprecedented - l3fwd itself won't run unless you explicitly specify
>> core config, so we *could* request additional parameters by default. I
>> would've also agreed with the "probe" thing *if it was a new application
>> without any pre-existing usages* - but it isn't, and in this case IMO
>> this is doubly important because there may be tests out there that *rely
>> on a bug* and thus should be explicitly broken and addressed (like the
>> internal test we had that uncovered the issue in the first place).
>>
>> In other words, in the perfect world, i would agree with you :) As it
>> stands though, i think that intentionally breaking tests that are
>> themselves broken and rely on wrong assumptions is more important than
>> keeping them working.
> 
> OK. Let's enumerate the pros and cons of both approaches?
> Approach a: Auto probe
> Approach b: Current patch
> 
> Approach a:
> + Application picks up the mode based on platform capability
> + No change in behavior wrt existing l3fwd-power where the platform
> has only interrupt support.
> (otherwise, It will fail to boot up, the CI etc we need to patch based
> on the DPDK version)
> 
> I am not sure approach b has pros wrt approach a.

The power library has other ways of freq scaling, not just through the 
pstate/ACPI. Now, granted, the third way (KVM) isn't supposed to work on 
l3fwd-power, and is in fact explicitly stopped from working in this 
patch, but still - relying on the scaling governor alone is not good enough.

Perhaps we should add a "check available" API that would probe each of 
the supported modes and return 1 for (potentially) supported and 0 for 
(definitely) unsupported? As far as i know, all three are reliant on 
file access at the end of the day, so we can check if they exist before 
trying to access them. That would effectively be "autodetect" without 
being too specific to ACPI/pstate scaling checks.

> 
> I.e On the x86 platform where freq scaling is present then SHOULD NOT
> have any difference in the approach a vs
> approach b. ie. Auto probe finds the system is capable of freq scaling
> and picks the powerlib. its is win-win case,
> I am not sure, What I am missing?
> 
> 

I was worried that an x86 VM would still have this file without being 
capable of frequency scaling, but i just checked and it seems that VM's 
indeed don't expose the cpufreq filesystem.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode

[Jerin Jacob]

On Tue, Jun 16, 2020 at 3:01 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 15-Jun-20 5:29 PM, Jerin Jacob wrote:
> > On Mon, Jun 15, 2020 at 9:15 PM Burakov, Anatoly
> > <anatoly.burakov@intel.com> wrote:
> >>
> >> On 15-Jun-20 4:21 PM, Jerin Jacob wrote:
> >>> On Mon, Jun 15, 2020 at 8:35 PM Burakov, Anatoly
> >>> <anatoly.burakov@intel.com> wrote:
> >>>>
> >>>> On 15-Jun-20 12:43 PM, Jerin Jacob wrote:
> >>>>> On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
> >>>>> <anatoly.burakov@intel.com> wrote:
> >>>>>>
> >>>>>> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
> >>>>>>> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
> >>>>>>>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
> >>>>>>>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
> >>>>>>>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> >>>>>>>>>>> External Email
> >>>>>>>>>>>
> >>>>>>>>>>> ----------------------------------------------------------------------
> >>>>>>>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>>>            if (ret < 0)
> >>>>>>>>>>>>>                    rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> >>>>>>>>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> >>>>>>>>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
> >>>>>>>>>>>>> +               app_mode = APP_MODE_LEGACY;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +       /* only legacy and empty poll mode rely on power library */
> >>>>>>>>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> >>>>>>>>>>>>> +                       init_power_library())
> >>>>>>>>>>>>>                    rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> >>>>>>>>>>>> Hi,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Rather than just exiting from here can we have a else condition to
> >>>>>>>>>>>> automatically enter into the "interrupt only" mode.
> >>>>>>>>>>>> Please correct me if I am missing something.
> >>>>>>>>>>>
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
> >>>>>>>>>>> the user wants interrupt-only mode, they should request it. Silently falling
> >>>>>>>>>>> back to interrupt-only mode will create an illusion of successful
> >>>>>>>>>>> initialization and set the wrong expectation for how the application will
> >>>>>>>>>>> behave.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> Thanks for the explanation which even I also believe is logically perfect.
> >>>>>>>>>>
> >>>>>>>>>> But since l3fwd-power is an old application and has many users around
> >>>>>>>>>> which are currently using this app in interrupt only mode without giving
> >>>>>>>>>> an extra argument. But suddenly they will start getting failure messages with
> >>>>>>>>>> the new patchset.
> >>>>>>>>>>
> >>>>>>>>>> My only intent with else condition was backward compatibility.
> >>>>>>>>>> Or may be we can have more descriptive failure message, something like
> >>>>>>>>>> "init_power_library failed, check manual for other modes".
> >>>>>>>>>>
> >>>>>>>>>> Thanks
> >>>>>>>>>> Harman
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I think we can compormise on an informative log message suggesting to use
> >>>>>>>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
> >>>>>>>>>
> >>>>>>>> Hi
> >>>>>>>>
> >>>>>>>> I am not insisting to revert to previous behavior, I am just trying to
> >>>>>>>> highlight some probable issues that many users might face as its an old
> >>>>>>>> application.
> >>>>>>>> Since many arm based soc might not be supporting frequency scaling, can
> >>>>>>>> we add the following check as soon as the application starts, probe the
> >>>>>>>> platform if it supports frequency scaling, if not automatically set the
> >>>>>>>> mode to interrupt mode, something like:
> >>>>>>>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>>>>>>                         F_OK))
> >>>>>>>>         app_mode = APP_MODE_INTERRUPT;
> >>>>>>>
> >>>>>>> Sorry, no direct check in application but we can introduce a new API in
> >>>>>>> power library:
> >>>>>>>        bool rte_is_freq_scaling() {
> >>>>>>>             return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>>>>>                             F_OK);
> >>>>>>>        }
> >>>>>>>
> >>>>>>> and in the application we can use "rte_is_freq_scaling()" at the start.
> >>>>>>>
> >>>>>>
> >>>>>> What you're suggesting here is effectively what you have already
> >>>>>> suggested: silently fall back to interrupt-only mode if power lib init
> >>>>>> failed. I already outlined why i don't think it's a good approach.
> >>>>>
> >>>>> Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
> >>>>> file presence,
> >>>>> detects the power lib availability . Right?  Not the failure. Right?
> >>>>> IMO, it make sense to have following case:
> >>>>> # first check, Is the system is capable of power lib if so use power lib
> >>>>> # if the system is not capable of using power lib use interrupt mode.
> >>>>>
> >>>>> I think, there is difference between system capable of using power lib
> >>>>> vs power lib not available or power lib failure.
> >>>>
> >>>> I am of the opinion that if a test sets up an unrealistic expectation of
> >>>> how an application should behave, it's a problem with the test, not with
> >>>> the application.
> >>>>
> >>>> If the system is not capable of running with power lib - the application
> >>>> shouldn't be requested to run in such mode.
> >>>
> >>> But with this patch, default proposed mode is power lib without any
> >>> explicit request.
> >>
> >> The default has always been "use the power library". That was always the
> >> expected behavior, i believe since the very first version of this
> >> application. In other words, even if the "requested" behavior is not
> >> requested explicitly, it has always been that implicitly, and this patch
> >> is *keeping existing behavior*.
> >
> > See below,
> >>
> >>>
> >>>>
> >>>> "The application behaved that way before" - yes, it did. It was a bug in
> >>>> the application, that it allowed users to effectively misuse the
> >>>> application and use it despite the fact that it was in a half-working
> >>>> state. This problem has been addressed by 1) not allowing the
> >>>> application to run in half-working state, and 2) adding a new mode where
> >>>> the old "expected" behavior is *actually* expected and is "full working
> >>>> state" now.
> >>>>
> >>>> Therefore, all users who were previously misusing the application to do
> >>>> something it was not designed to do because of a bug in the
> >>>> implementation, should now fix their usage and use the correct mode -
> >>>> and such breakage is IMO necessary to call attention to earlier misuse
> >>>> in the tools, and to correct this usage.
> >>>>
> >>>> What bothers me about your suggestion is that it is impossible to fail
> >>>> the test if the wrong mode was requested (as in, if we request the
> >>>> power-lib mode on a system that doesn't have freq scaling) - it instead
> >>>> silently falls back to a mode that is almost guaranteed to work.
> >>>
> >>> I agree that it should fail, i.e someone explicitly request,
> >>> power-lib mode or any mode
> >>> and it should fail application if the platform we can not do that.
> >>>
> >>> My suggestion is all about, what is the default, IMO, if no argument
> >>> is specified,
> >>> the application should _probe_ the capability of the platfrom and
> >>> choose the mode. One can override
> >>> the probe to select the desired one. In such mode, fail to configure
> >>> the mode should result in
> >>> an error.
> >>
> >> This would change the default behavior that has always existed with this
> >> application, and would still be subject to silent failure issue
> >> *because* older tests may not account for this implied assumption of
> >> "the application will run no matter what", leading to a possible false
> >> positive test result.
> >>
> >> Now, if the default was "not to run and ask explicitly what mode should
> >> the user use" - i can see how we could both agree on that. It's not
> >> unprecedented - l3fwd itself won't run unless you explicitly specify
> >> core config, so we *could* request additional parameters by default. I
> >> would've also agreed with the "probe" thing *if it was a new application
> >> without any pre-existing usages* - but it isn't, and in this case IMO
> >> this is doubly important because there may be tests out there that *rely
> >> on a bug* and thus should be explicitly broken and addressed (like the
> >> internal test we had that uncovered the issue in the first place).
> >>
> >> In other words, in the perfect world, i would agree with you :) As it
> >> stands though, i think that intentionally breaking tests that are
> >> themselves broken and rely on wrong assumptions is more important than
> >> keeping them working.
> >
> > OK. Let's enumerate the pros and cons of both approaches?
> > Approach a: Auto probe
> > Approach b: Current patch
> >
> > Approach a:
> > + Application picks up the mode based on platform capability
> > + No change in behavior wrt existing l3fwd-power where the platform
> > has only interrupt support.
> > (otherwise, It will fail to boot up, the CI etc we need to patch based
> > on the DPDK version)
> >
> > I am not sure approach b has pros wrt approach a.
>
> The power library has other ways of freq scaling, not just through the
> pstate/ACPI. Now, granted, the third way (KVM) isn't supposed to work on
> l3fwd-power, and is in fact explicitly stopped from working in this
> patch, but still - relying on the scaling governor alone is not good enough.
>
> Perhaps we should add a "check available" API that would probe each of
> the supported modes and return 1 for (potentially) supported and 0 for
> (definitely) unsupported?

+1 for adding "check available" API for the specific mode.


>  As far as i know, all three are reliant on
> file access at the end of the day, so we can check if they exist before
> trying to access them. That would effectively be "autodetect" without
> being too specific to ACPI/pstate scaling checks.

+1

>
> >
> > I.e On the x86 platform where freq scaling is present then SHOULD NOT
> > have any difference in the approach a vs
> > approach b. ie. Auto probe finds the system is capable of freq scaling
> > and picks the powerlib. its is win-win case,
> > I am not sure, What I am missing?
> >
> >
>
> I was worried that an x86 VM would still have this file without being
> capable of frequency scaling, but i just checked and it seems that VM's
> indeed don't expose the cpufreq filesystem.

Looks good then!!

>
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH 0/3] Add interrupt-only mode to l3fwd-power

[Wang, Yinan]

Tested-by: Wang, Yinan <yinan.wang@intel.com>

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> Sent: 2020年5月28日 17:14
> To: dev@dpdk.org
> Cc: Hunt, David <david.hunt@intel.com>; Ma, Liang J <liang.j.ma@intel.com>;
> Pattan, Reshma <reshma.pattan@intel.com>
> Subject: [dpdk-dev] [PATCH 0/3] Add interrupt-only mode to l3fwd-power
> 
> Since 20.05, l3fwd-power has become much more stringent about whether it
> allows initialization without initializing the librte_power library with it. This
> means that while previously the app could have been used to test RX interrupts
> functionality even if the app itself was in a half-working state, it is now no
> longer possible to do so.
> 
> To address this use case, we're adding an interrupt-only mode that does not rely
> on librte_power or telemetry. This enables using l3fwd-power in environments
> where librte_power is not expected to work (such as inside a VM or on non-IA
> architectures). The RX/TX path is basically copy paste from legacy RX/TX path
> but with librte_power bits taken out.
> 
> There seem to be two opposing schools of thought on whether we should have
> more or less examples. This patchset goes in the "less" direction where we add a
> new mode to an existing app, rather than creating a new one like it could be
> argued it deserves.
> 
> Anatoly Burakov (3):
>   l3fwd-power: disable interrupts by default
>   l3fwd-power: only allow supported power library envs
>   l3fwd-power: add interrupt-only mode
> 
>  examples/l3fwd-power/main.c | 234 +++++++++++++++++++++++++++++++++--
> -
>  1 file changed, 217 insertions(+), 17 deletions(-)
> 
> --
> 2.17.1

[dpdk-dev] [PATCH v2 0/7] Add interrupt-only mode to l3fwd-power

[Anatoly Burakov]

Since 20.05, l3fwd-power has become much more stringent about
whether it allows initialization without initializing the
librte_power library with it. This means that while previously
the app could have been used to test RX interrupts functionality
even if the app itself was in a half-working state, it is now
no longer possible to do so.

To address this use case, we're adding an interrupt-only mode
that does not rely on librte_power or telemetry. This enables
using l3fwd-power in environments where librte_power is not
expected to work (such as inside a VM or on non-IA
architectures). The RX/TX path is basically copy paste from
legacy RX/TX path but with librte_power bits taken out.

There seem to be two opposing schools of thought on whether we
should have more or less examples. This patchset goes in the
"less" direction where we add a new mode to an existing app,
rather than creating a new one like it could be argued it
deserves.

v2:
- Add API to probe support for a specific power env
- Add autodetection for the default mode
- Add ability to request legacy mode specifically
- Fix some code style issues

Anatoly Burakov (7):
  l3fwd-power: disable interrupts by default
  l3fwd-power: only allow supported power library envs
  l3fwd-power: code style and flow fixes
  l3fwd-power: add support for requesting legacy mode
  l3fwd-power: add interrupt-only mode
  power: add API to probe support for a specific env
  l3fwd-power: add auto-selection of default mode

 examples/l3fwd-power/main.c             | 267 ++++++++++++++++++++++--
 lib/librte_power/Makefile               |   1 +
 lib/librte_power/guest_channel.c        |  26 +++
 lib/librte_power/guest_channel.h        |  12 ++
 lib/librte_power/meson.build            |   3 +-
 lib/librte_power/power_acpi_cpufreq.c   |   7 +
 lib/librte_power/power_acpi_cpufreq.h   |  10 +
 lib/librte_power/power_common.c         |  52 +++++
 lib/librte_power/power_common.h         |   3 +
 lib/librte_power/power_kvm_vm.c         |   5 +
 lib/librte_power/power_kvm_vm.h         |  10 +
 lib/librte_power/power_pstate_cpufreq.c |   7 +
 lib/librte_power/power_pstate_cpufreq.h |  10 +
 lib/librte_power/rte_power.c            |  17 ++
 lib/librte_power/rte_power.h            |  18 ++
 lib/librte_power/rte_power_version.map  |   1 +
 16 files changed, 429 insertions(+), 20 deletions(-)
 create mode 100644 lib/librte_power/power_common.c

-- 
2.17.1

[dpdk-dev] [PATCH v3 0/7] Add interrupt-only mode to l3fwd-power

[Anatoly Burakov]

Since 20.05, l3fwd-power has become much more stringent about
whether it allows initialization without initializing the
librte_power library with it. This means that while previously
the app could have been used to test RX interrupts functionality
even if the app itself was in a half-working state, it is now
no longer possible to do so.

To address this use case, we're adding an interrupt-only mode
that does not rely on librte_power or telemetry. This enables
using l3fwd-power in environments where librte_power is not
expected to work (such as inside a VM or on non-IA
architectures). The RX/TX path is basically copy paste from
legacy RX/TX path but with librte_power bits taken out.

There seem to be two opposing schools of thought on whether we
should have more or less examples. This patchset goes in the
"less" direction where we add a new mode to an existing app,
rather than creating a new one like it could be argued it
deserves.

v3:
- Added log messages for autodetect
- Fixed wrong comment in patch 7

v2:
- Add API to probe support for a specific power env
- Add autodetection for the default mode
- Add ability to request legacy mode specifically
- Fix some code style issues

Anatoly Burakov (7):
  l3fwd-power: disable interrupts by default
  l3fwd-power: only allow supported power library envs
  l3fwd-power: code style and flow fixes
  l3fwd-power: add support for requesting legacy mode
  l3fwd-power: add interrupt-only mode
  power: add API to probe support for a specific env
  l3fwd-power: add auto-selection of default mode

 examples/l3fwd-power/main.c             | 292 ++++++++++++++++++++++--
 lib/librte_power/Makefile               |   1 +
 lib/librte_power/guest_channel.c        |  26 +++
 lib/librte_power/guest_channel.h        |  12 +
 lib/librte_power/meson.build            |   3 +-
 lib/librte_power/power_acpi_cpufreq.c   |   7 +
 lib/librte_power/power_acpi_cpufreq.h   |  10 +
 lib/librte_power/power_common.c         |  52 +++++
 lib/librte_power/power_common.h         |   3 +
 lib/librte_power/power_kvm_vm.c         |   5 +
 lib/librte_power/power_kvm_vm.h         |  10 +
 lib/librte_power/power_pstate_cpufreq.c |   7 +
 lib/librte_power/power_pstate_cpufreq.h |  10 +
 lib/librte_power/rte_power.c            |  17 ++
 lib/librte_power/rte_power.h            |  18 ++
 lib/librte_power/rte_power_version.map  |   1 +
 16 files changed, 454 insertions(+), 20 deletions(-)
 create mode 100644 lib/librte_power/power_common.c

-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 0/7] Add interrupt-only mode to l3fwd-power

[Thomas Monjalon]

19/06/2020 12:53, Anatoly Burakov:
> Since 20.05, l3fwd-power has become much more stringent about
> whether it allows initialization without initializing the
> librte_power library with it. This means that while previously
> the app could have been used to test RX interrupts functionality
> even if the app itself was in a half-working state, it is now
> no longer possible to do so.
> 
> To address this use case, we're adding an interrupt-only mode
> that does not rely on librte_power or telemetry. This enables
> using l3fwd-power in environments where librte_power is not
> expected to work (such as inside a VM or on non-IA
> architectures). The RX/TX path is basically copy paste from
> legacy RX/TX path but with librte_power bits taken out.
> 
> There seem to be two opposing schools of thought on whether we
> should have more or less examples. This patchset goes in the
> "less" direction where we add a new mode to an existing app,
> rather than creating a new one like it could be argued it
> deserves.
> 
> v3:
> - Added log messages for autodetect
> - Fixed wrong comment in patch 7
> 
> v2:
> - Add API to probe support for a specific power env
> - Add autodetection for the default mode
> - Add ability to request legacy mode specifically
> - Fix some code style issues
> 
> Anatoly Burakov (7):
>   l3fwd-power: disable interrupts by default
>   l3fwd-power: only allow supported power library envs
>   l3fwd-power: code style and flow fixes
>   l3fwd-power: add support for requesting legacy mode
>   l3fwd-power: add interrupt-only mode
>   power: add API to probe support for a specific env
>   l3fwd-power: add auto-selection of default mode

Applied with following titles:
	examples/l3fwd-power: disable interrupts by default
	examples/l3fwd-power: allow only supported environments
	examples/l3fwd-power: fix style and control flow
	examples/l3fwd-power: add legacy mode option
	examples/l3fwd-power: add interrupt-only mode
	power: add environment capability probing
	examples/l3fwd-power: select default mode automatically

[dpdk-dev] [PATCH v3 1/7] l3fwd-power: disable interrupts by default

[Anatoly Burakov]

Currently, interrupts are enabled in telemetry and empty poll modes, but
they are not used. Switch to disabling interrupts by default, and only
enable interrupts for modes that require them.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
---
 examples/l3fwd-power/main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 9db94ce044..2cc5d7b121 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -256,10 +256,7 @@ static struct rte_eth_conf port_conf = {
 	},
 	.txmode = {
 		.mq_mode = ETH_MQ_TX_NONE,
-	},
-	.intr_conf = {
-		.rxq = 1,
-	},
+	}
 };
 
 static struct rte_mempool * pktmbuf_pool[NB_SOCKETS];
@@ -2270,6 +2267,8 @@ main(int argc, char **argv)
 	/* initialize all ports */
 	RTE_ETH_FOREACH_DEV(portid) {
 		struct rte_eth_conf local_port_conf = port_conf;
+		/* not all app modes need interrupts */
+		bool need_intr = app_mode == APP_MODE_LEGACY;
 
 		/* skip ports that are not enabled */
 		if ((enabled_port_mask & (1 << portid)) == 0) {
@@ -2303,7 +2302,10 @@ main(int argc, char **argv)
 			nb_rx_queue, (unsigned)n_tx_queue );
 		/* If number of Rx queue is 0, no need to enable Rx interrupt */
 		if (nb_rx_queue == 0)
-			local_port_conf.intr_conf.rxq = 0;
+			need_intr = false;
+
+		if (need_intr)
+			local_port_conf.intr_conf.rxq = 1;
 
 		ret = rte_eth_dev_info_get(portid, &dev_info);
 		if (ret != 0)
-- 
2.17.1

[dpdk-dev] [PATCH v3 2/7] l3fwd-power: only allow supported power library envs

[Anatoly Burakov]

Currently, l3fwd-power will attempt to run even if the power env
is set to KVM, which is not supported. Fix this by preventing the
app from initializing unless the env is set to one of the supported
modes.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
---
 examples/l3fwd-power/main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 2cc5d7b121..5cee9d5387 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -2043,6 +2043,7 @@ static int check_ptype(uint16_t portid)
 static int
 init_power_library(void)
 {
+	enum power_management_env env;
 	unsigned int lcore_id;
 	int ret = 0;
 
@@ -2055,6 +2056,14 @@ init_power_library(void)
 				lcore_id);
 			return ret;
 		}
+		/* we're not supporting the VM channel mode */
+		env = rte_power_get_env();
+		if (env != PM_ENV_ACPI_CPUFREQ &&
+				env != PM_ENV_PSTATE_CPUFREQ) {
+			RTE_LOG(ERR, POWER,
+				"Only ACPI and PSTATE mode are supported\n");
+			return -1;
+		}
 	}
 	return ret;
 }
-- 
2.17.1

[dpdk-dev] [PATCH v3 3/7] l3fwd-power: code style and flow fixes

[Anatoly Burakov]

Make the coding style more consistent, and the init logic control flow
more explicit.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 5cee9d5387..c70611bb7f 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -1126,7 +1126,7 @@ main_empty_poll_loop(__rte_unused void *dummy)
 }
 /* main processing loop */
 static int
-main_loop(__rte_unused void *dummy)
+main_legacy_loop(__rte_unused void *dummy)
 {
 	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
 	unsigned lcore_id;
@@ -1581,6 +1581,7 @@ parse_ep_config(const char *q_arg)
 
 }
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+#define CMD_LINE_OPT_EMPTY_POLL "empty-poll"
 #define CMD_LINE_OPT_TELEMETRY "telemetry"
 
 /* Parse the argument given in the command line of the application */
@@ -1598,7 +1599,7 @@ parse_args(int argc, char **argv)
 		{"high-perf-cores", 1, 0, 0},
 		{"no-numa", 0, 0, 0},
 		{"enable-jumbo", 0, 0, 0},
-		{"empty-poll", 1, 0, 0},
+		{CMD_LINE_OPT_EMPTY_POLL, 1, 0, 0},
 		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
 		{NULL, 0, 0, 0}
@@ -1673,7 +1674,7 @@ parse_args(int argc, char **argv)
 			}
 
 			if (!strncmp(lgopts[option_index].name,
-						"empty-poll", 10)) {
+					CMD_LINE_OPT_EMPTY_POLL, 10)) {
 				if (app_mode == APP_MODE_TELEMETRY) {
 					printf(" empty-poll cannot be enabled as telemetry mode is enabled\n");
 					return -1;
@@ -2253,7 +2254,9 @@ main(int argc, char **argv)
 	if (ret < 0)
 		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
 
-	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
+	/* only legacy and empty poll mode rely on power library */
+	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
+			init_power_library())
 		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
 
 	if (update_lcore_params() < 0)
@@ -2526,12 +2529,12 @@ main(int argc, char **argv)
 
 	/* launch per-lcore init on every lcore */
 	if (app_mode == APP_MODE_LEGACY) {
-		rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
+		rte_eal_mp_remote_launch(main_legacy_loop, NULL, CALL_MASTER);
 	} else if (app_mode == APP_MODE_EMPTY_POLL) {
 		empty_poll_stop = false;
 		rte_eal_mp_remote_launch(main_empty_poll_loop, NULL,
 				SKIP_MASTER);
-	} else {
+	} else if (app_mode == APP_MODE_TELEMETRY) {
 		unsigned int i;
 
 		/* Init metrics library */
@@ -2577,7 +2580,8 @@ main(int argc, char **argv)
 	if (app_mode == APP_MODE_EMPTY_POLL)
 		rte_power_empty_poll_stat_free();
 
-	if (app_mode != APP_MODE_TELEMETRY && deinit_power_library())
+	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
+			deinit_power_library())
 		rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
 
 	if (rte_eal_cleanup() < 0)
-- 
2.17.1

[dpdk-dev] [PATCH v3 4/7] l3fwd-power: add support for requesting legacy mode

[Anatoly Burakov]

Currently, legacy mode is the implicit default, but it is not possible
to directly request using legacy mode. Add the argument to enable
requesting legacy mode, and also make it the default.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index c70611bb7f..c02b0a3574 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -195,7 +195,8 @@ static int parse_ptype; /**< Parse packet type using rx callback, and */
 			/**< disabled by default */
 
 enum appmode {
-	APP_MODE_LEGACY = 0,
+	APP_MODE_DEFAULT = 0,
+	APP_MODE_LEGACY,
 	APP_MODE_EMPTY_POLL,
 	APP_MODE_TELEMETRY
 };
@@ -1435,6 +1436,7 @@ print_usage(const char *prgname)
 		"  --enable-jumbo: enable jumbo frame"
 		" which max packet len is PKTLEN in decimal (64-9600)\n"
 		"  --parse-ptype: parse packet type by software\n"
+		"  --legacy: use legacy interrupt-based scaling\n"
 		"  --empty-poll: enable empty poll detection"
 		" follow (training_flag, high_threshold, med_threshold)\n"
 		" --telemetry: enable telemetry mode, to update"
@@ -1581,6 +1583,7 @@ parse_ep_config(const char *q_arg)
 
 }
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+#define CMD_LINE_OPT_LEGACY "legacy"
 #define CMD_LINE_OPT_EMPTY_POLL "empty-poll"
 #define CMD_LINE_OPT_TELEMETRY "telemetry"
 
@@ -1601,6 +1604,7 @@ parse_args(int argc, char **argv)
 		{"enable-jumbo", 0, 0, 0},
 		{CMD_LINE_OPT_EMPTY_POLL, 1, 0, 0},
 		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
+		{CMD_LINE_OPT_LEGACY, 0, 0, 0},
 		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
@@ -1673,10 +1677,21 @@ parse_args(int argc, char **argv)
 				numa_on = 0;
 			}
 
+			if (!strncmp(lgopts[option_index].name,
+					CMD_LINE_OPT_LEGACY,
+					sizeof(CMD_LINE_OPT_LEGACY))) {
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" legacy mode is mutually exclusive with other modes\n");
+					return -1;
+				}
+				app_mode = APP_MODE_LEGACY;
+				printf("legacy mode is enabled\n");
+			}
+
 			if (!strncmp(lgopts[option_index].name,
 					CMD_LINE_OPT_EMPTY_POLL, 10)) {
-				if (app_mode == APP_MODE_TELEMETRY) {
-					printf(" empty-poll cannot be enabled as telemetry mode is enabled\n");
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" empty-poll mode is mutually exclusive with other modes\n");
 					return -1;
 				}
 				app_mode = APP_MODE_EMPTY_POLL;
@@ -1693,8 +1708,8 @@ parse_args(int argc, char **argv)
 			if (!strncmp(lgopts[option_index].name,
 					CMD_LINE_OPT_TELEMETRY,
 					sizeof(CMD_LINE_OPT_TELEMETRY))) {
-				if (app_mode == APP_MODE_EMPTY_POLL) {
-					printf("telemetry mode cannot be enabled as empty poll mode is enabled\n");
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" telemetry mode is mutually exclusive with other modes\n");
 					return -1;
 				}
 				app_mode = APP_MODE_TELEMETRY;
@@ -2254,6 +2269,9 @@ main(int argc, char **argv)
 	if (ret < 0)
 		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
 
+	if (app_mode == APP_MODE_DEFAULT)
+		app_mode = APP_MODE_LEGACY;
+
 	/* only legacy and empty poll mode rely on power library */
 	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
 			init_power_library())
-- 
2.17.1

[dpdk-dev] [PATCH v3 5/7] l3fwd-power: add interrupt-only mode

[Anatoly Burakov]

In addition to existing modes, add a mode which is very similar to
legacy mode, but does not do frequency scaling, and thus does not
depend on the power library.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
---
 examples/l3fwd-power/main.c | 188 +++++++++++++++++++++++++++++++++++-
 1 file changed, 185 insertions(+), 3 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index c02b0a3574..51acbfd87d 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -198,7 +198,8 @@ enum appmode {
 	APP_MODE_DEFAULT = 0,
 	APP_MODE_LEGACY,
 	APP_MODE_EMPTY_POLL,
-	APP_MODE_TELEMETRY
+	APP_MODE_TELEMETRY,
+	APP_MODE_INTERRUPT
 };
 
 enum appmode app_mode;
@@ -901,6 +902,170 @@ static int event_register(struct lcore_conf *qconf)
 
 	return 0;
 }
+
+/* main processing loop */
+static int main_intr_loop(__rte_unused void *dummy)
+{
+	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
+	unsigned int lcore_id;
+	uint64_t prev_tsc, diff_tsc, cur_tsc;
+	int i, j, nb_rx;
+	uint8_t queueid;
+	uint16_t portid;
+	struct lcore_conf *qconf;
+	struct lcore_rx_queue *rx_queue;
+	uint32_t lcore_rx_idle_count = 0;
+	uint32_t lcore_idle_hint = 0;
+	int intr_en = 0;
+
+	const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
+				   US_PER_S * BURST_TX_DRAIN_US;
+
+	prev_tsc = 0;
+
+	lcore_id = rte_lcore_id();
+	qconf = &lcore_conf[lcore_id];
+
+	if (qconf->n_rx_queue == 0) {
+		RTE_LOG(INFO, L3FWD_POWER, "lcore %u has nothing to do\n",
+				lcore_id);
+		return 0;
+	}
+
+	RTE_LOG(INFO, L3FWD_POWER, "entering main interrupt loop on lcore %u\n",
+			lcore_id);
+
+	for (i = 0; i < qconf->n_rx_queue; i++) {
+		portid = qconf->rx_queue_list[i].port_id;
+		queueid = qconf->rx_queue_list[i].queue_id;
+		RTE_LOG(INFO, L3FWD_POWER,
+				" -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
+				lcore_id, portid, queueid);
+	}
+
+	/* add into event wait list */
+	if (event_register(qconf) == 0)
+		intr_en = 1;
+	else
+		RTE_LOG(INFO, L3FWD_POWER, "RX interrupt won't enable.\n");
+
+	while (!is_done()) {
+		stats[lcore_id].nb_iteration_looped++;
+
+		cur_tsc = rte_rdtsc();
+
+		/*
+		 * TX burst queue drain
+		 */
+		diff_tsc = cur_tsc - prev_tsc;
+		if (unlikely(diff_tsc > drain_tsc)) {
+			for (i = 0; i < qconf->n_tx_port; ++i) {
+				portid = qconf->tx_port_id[i];
+				rte_eth_tx_buffer_flush(portid,
+						qconf->tx_queue_id[portid],
+						qconf->tx_buffer[portid]);
+			}
+			prev_tsc = cur_tsc;
+		}
+
+start_rx:
+		/*
+		 * Read packet from RX queues
+		 */
+		lcore_rx_idle_count = 0;
+		for (i = 0; i < qconf->n_rx_queue; ++i) {
+			rx_queue = &(qconf->rx_queue_list[i]);
+			rx_queue->idle_hint = 0;
+			portid = rx_queue->port_id;
+			queueid = rx_queue->queue_id;
+
+			nb_rx = rte_eth_rx_burst(portid, queueid, pkts_burst,
+					MAX_PKT_BURST);
+
+			stats[lcore_id].nb_rx_processed += nb_rx;
+			if (unlikely(nb_rx == 0)) {
+				/**
+				 * no packet received from rx queue, try to
+				 * sleep for a while forcing CPU enter deeper
+				 * C states.
+				 */
+				rx_queue->zero_rx_packet_count++;
+
+				if (rx_queue->zero_rx_packet_count <=
+						MIN_ZERO_POLL_COUNT)
+					continue;
+
+				rx_queue->idle_hint = power_idle_heuristic(
+						rx_queue->zero_rx_packet_count);
+				lcore_rx_idle_count++;
+			} else {
+				rx_queue->zero_rx_packet_count = 0;
+			}
+
+			/* Prefetch first packets */
+			for (j = 0; j < PREFETCH_OFFSET && j < nb_rx; j++) {
+				rte_prefetch0(rte_pktmbuf_mtod(
+						pkts_burst[j], void *));
+			}
+
+			/* Prefetch and forward already prefetched packets */
+			for (j = 0; j < (nb_rx - PREFETCH_OFFSET); j++) {
+				rte_prefetch0(rte_pktmbuf_mtod(
+						pkts_burst[j + PREFETCH_OFFSET],
+						void *));
+				l3fwd_simple_forward(
+						pkts_burst[j], portid, qconf);
+			}
+
+			/* Forward remaining prefetched packets */
+			for (; j < nb_rx; j++) {
+				l3fwd_simple_forward(
+						pkts_burst[j], portid, qconf);
+			}
+		}
+
+		if (unlikely(lcore_rx_idle_count == qconf->n_rx_queue)) {
+			/**
+			 * All Rx queues empty in recent consecutive polls,
+			 * sleep in a conservative manner, meaning sleep as
+			 * less as possible.
+			 */
+			for (i = 1,
+			    lcore_idle_hint = qconf->rx_queue_list[0].idle_hint;
+					i < qconf->n_rx_queue; ++i) {
+				rx_queue = &(qconf->rx_queue_list[i]);
+				if (rx_queue->idle_hint < lcore_idle_hint)
+					lcore_idle_hint = rx_queue->idle_hint;
+			}
+
+			if (lcore_idle_hint < SUSPEND_THRESHOLD)
+				/**
+				 * execute "pause" instruction to avoid context
+				 * switch which generally take hundred of
+				 * microseconds for short sleep.
+				 */
+				rte_delay_us(lcore_idle_hint);
+			else {
+				/* suspend until rx interrupt triggers */
+				if (intr_en) {
+					turn_on_off_intr(qconf, 1);
+					sleep_until_rx_interrupt(
+							qconf->n_rx_queue);
+					turn_on_off_intr(qconf, 0);
+					/**
+					 * start receiving packets immediately
+					 */
+					if (likely(!is_done()))
+						goto start_rx;
+				}
+			}
+			stats[lcore_id].sleep_time += lcore_idle_hint;
+		}
+	}
+
+	return 0;
+}
+
 /* main processing loop */
 static int
 main_telemetry_loop(__rte_unused void *dummy)
@@ -1440,7 +1605,8 @@ print_usage(const char *prgname)
 		"  --empty-poll: enable empty poll detection"
 		" follow (training_flag, high_threshold, med_threshold)\n"
 		" --telemetry: enable telemetry mode, to update"
-		" empty polls, full polls, and core busyness to telemetry\n",
+		" empty polls, full polls, and core busyness to telemetry\n"
+		" --interrupt-only: enable interrupt-only mode\n",
 		prgname);
 }
 
@@ -1585,6 +1751,7 @@ parse_ep_config(const char *q_arg)
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
 #define CMD_LINE_OPT_LEGACY "legacy"
 #define CMD_LINE_OPT_EMPTY_POLL "empty-poll"
+#define CMD_LINE_OPT_INTERRUPT_ONLY "interrupt-only"
 #define CMD_LINE_OPT_TELEMETRY "telemetry"
 
 /* Parse the argument given in the command line of the application */
@@ -1606,6 +1773,7 @@ parse_args(int argc, char **argv)
 		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{CMD_LINE_OPT_LEGACY, 0, 0, 0},
 		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
+		{CMD_LINE_OPT_INTERRUPT_ONLY, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -1716,6 +1884,17 @@ parse_args(int argc, char **argv)
 				printf("telemetry mode is enabled\n");
 			}
 
+			if (!strncmp(lgopts[option_index].name,
+					CMD_LINE_OPT_INTERRUPT_ONLY,
+					sizeof(CMD_LINE_OPT_INTERRUPT_ONLY))) {
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" interrupt-only mode is mutually exclusive with other modes\n");
+					return -1;
+				}
+				app_mode = APP_MODE_INTERRUPT;
+				printf("interrupt-only mode is enabled\n");
+			}
+
 			if (!strncmp(lgopts[option_index].name,
 					"enable-jumbo", 12)) {
 				struct option lenopts =
@@ -2298,7 +2477,8 @@ main(int argc, char **argv)
 	RTE_ETH_FOREACH_DEV(portid) {
 		struct rte_eth_conf local_port_conf = port_conf;
 		/* not all app modes need interrupts */
-		bool need_intr = app_mode == APP_MODE_LEGACY;
+		bool need_intr = app_mode == APP_MODE_LEGACY ||
+				app_mode == APP_MODE_INTERRUPT;
 
 		/* skip ports that are not enabled */
 		if ((enabled_port_mask & (1 << portid)) == 0) {
@@ -2576,6 +2756,8 @@ main(int argc, char **argv)
 				"Returns global power stats. Parameters: None");
 		rte_eal_mp_remote_launch(main_telemetry_loop, NULL,
 						SKIP_MASTER);
+	} else if (app_mode == APP_MODE_INTERRUPT) {
+		rte_eal_mp_remote_launch(main_intr_loop, NULL, CALL_MASTER);
 	}
 
 	if (app_mode == APP_MODE_EMPTY_POLL || app_mode == APP_MODE_TELEMETRY)
-- 
2.17.1

[dpdk-dev] [PATCH v3 6/7] power: add API to probe support for a specific env

[Anatoly Burakov]

Currently, there is no way to know if the power management env is
supported without trying to initialize it. The init API also does
not distinguish between failure due to some error and failure due to
power management not being available on the platform in the first
place.

Thus, add an API that provides capability of probing support for a
specific power management API.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Suggested-by: Jerin Jacob <jerinjacobk@gmail.com>
---
 lib/librte_power/Makefile               |  1 +
 lib/librte_power/guest_channel.c        | 26 +++++++++++++
 lib/librte_power/guest_channel.h        | 12 ++++++
 lib/librte_power/meson.build            |  3 +-
 lib/librte_power/power_acpi_cpufreq.c   |  7 ++++
 lib/librte_power/power_acpi_cpufreq.h   | 10 +++++
 lib/librte_power/power_common.c         | 52 +++++++++++++++++++++++++
 lib/librte_power/power_common.h         |  3 ++
 lib/librte_power/power_kvm_vm.c         |  5 +++
 lib/librte_power/power_kvm_vm.h         | 10 +++++
 lib/librte_power/power_pstate_cpufreq.c |  7 ++++
 lib/librte_power/power_pstate_cpufreq.h | 10 +++++
 lib/librte_power/rte_power.c            | 17 ++++++++
 lib/librte_power/rte_power.h            | 18 +++++++++
 lib/librte_power/rte_power_version.map  |  1 +
 15 files changed, 181 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_power/power_common.c

diff --git a/lib/librte_power/Makefile b/lib/librte_power/Makefile
index 087d643ee5..3b067b615f 100644
--- a/lib/librte_power/Makefile
+++ b/lib/librte_power/Makefile
@@ -16,6 +16,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_POWER) := rte_power.c power_acpi_cpufreq.c
 SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_kvm_vm.c guest_channel.c
 SRCS-$(CONFIG_RTE_LIBRTE_POWER) += rte_power_empty_poll.c
 SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_pstate_cpufreq.c
+SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_common.c
 
 # install this header file
 SYMLINK-$(CONFIG_RTE_LIBRTE_POWER)-include := rte_power.h  rte_power_empty_poll.h
diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index b984d55bc8..7b5926e5c4 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2010-2014 Intel Corporation
  */
 
+#include <glob.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -25,6 +26,31 @@
 
 static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
+int
+guest_channel_host_check_exists(const char *path)
+{
+	char glob_path[PATH_MAX];
+	glob_t g;
+	int ret;
+
+	/* we cannot know in advance which cores have VM channels, so glob */
+	snprintf(glob_path, PATH_MAX, "%s.*", path);
+
+	ret = glob(glob_path, GLOB_NOSORT, NULL, &g);
+	if (ret != 0) {
+		/* couldn't read anything */
+		ret = 0;
+		goto out;
+	}
+
+	/* do we have at least one match? */
+	ret = g.gl_pathc > 0;
+
+out:
+	globfree(&g);
+	return ret;
+}
+
 int
 guest_channel_host_connect(const char *path, unsigned int lcore_id)
 {
diff --git a/lib/librte_power/guest_channel.h b/lib/librte_power/guest_channel.h
index 025961606c..e15db46fc7 100644
--- a/lib/librte_power/guest_channel.h
+++ b/lib/librte_power/guest_channel.h
@@ -10,6 +10,18 @@ extern "C" {
 
 #include <channel_commands.h>
 
+/**
+ * Check if any Virtio-Serial VM end-points exist in path.
+ *
+ * @param path
+ *  The path to the serial device on the filesystem
+ *
+ * @return
+ *  - 1 if at least one potential end-point found.
+ *  - 0 if no end-points found.
+ */
+int guest_channel_host_check_exists(const char *path);
+
 /**
  * Connect to the Virtio-Serial VM end-point located in path. It is
  * thread safe for unique lcore_ids. This function must be only called once from
diff --git a/lib/librte_power/meson.build b/lib/librte_power/meson.build
index cdf08f6df3..78c031c943 100644
--- a/lib/librte_power/meson.build
+++ b/lib/librte_power/meson.build
@@ -8,6 +8,7 @@ endif
 sources = files('rte_power.c', 'power_acpi_cpufreq.c',
 		'power_kvm_vm.c', 'guest_channel.c',
 		'rte_power_empty_poll.c',
-		'power_pstate_cpufreq.c')
+		'power_pstate_cpufreq.c',
+		'power_common.c')
 headers = files('rte_power.h','rte_power_empty_poll.h')
 deps += ['timer']
diff --git a/lib/librte_power/power_acpi_cpufreq.c b/lib/librte_power/power_acpi_cpufreq.c
index f443fce69f..583815a07e 100644
--- a/lib/librte_power/power_acpi_cpufreq.c
+++ b/lib/librte_power/power_acpi_cpufreq.c
@@ -59,6 +59,7 @@
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_frequencies"
 #define POWER_SYSFILE_SETSPEED   \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_setspeed"
+#define POWER_ACPI_DRIVER "acpi-cpufreq"
 
 /*
  * MSR related
@@ -289,6 +290,12 @@ power_init_for_setting_freq(struct rte_power_info *pi)
 	return -1;
 }
 
+int
+power_acpi_cpufreq_check_supported(void)
+{
+	return cpufreq_check_scaling_driver(POWER_ACPI_DRIVER);
+}
+
 int
 power_acpi_cpufreq_init(unsigned int lcore_id)
 {
diff --git a/lib/librte_power/power_acpi_cpufreq.h b/lib/librte_power/power_acpi_cpufreq.h
index 1af7416073..038857b3a6 100644
--- a/lib/librte_power/power_acpi_cpufreq.h
+++ b/lib/librte_power/power_acpi_cpufreq.h
@@ -20,6 +20,16 @@
 extern "C" {
 #endif
 
+/**
+ * Check if ACPI power management is supported.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+int power_acpi_cpufreq_check_supported(void);
+
 /**
  * Initialize power management for a specific lcore. It will check and set the
  * governor to userspace for the lcore, get the available frequencies, and
diff --git a/lib/librte_power/power_common.c b/lib/librte_power/power_common.c
new file mode 100644
index 0000000000..59023d986b
--- /dev/null
+++ b/lib/librte_power/power_common.c
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+
+#include <limits.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "power_common.h"
+
+#define POWER_SYSFILE_SCALING_DRIVER   \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
+
+int
+cpufreq_check_scaling_driver(const char *driver_name)
+{
+	unsigned int lcore_id = 0; /* always check core 0 */
+	char fullpath[PATH_MAX];
+	char readbuf[PATH_MAX];
+	char *s;
+	FILE *f;
+
+	/*
+	 * Check if scaling driver matches what we expect.
+	 */
+	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SCALING_DRIVER,
+			lcore_id);
+	f = fopen(fullpath, "r");
+
+	/* if there's no driver at all, bail out */
+	if (f == NULL)
+		return 0;
+
+	s = fgets(readbuf, sizeof(readbuf), f);
+	/* don't need it any more */
+	fclose(f);
+
+	/* if we can't read it, consider unsupported */
+	if (s == NULL)
+		return 0;
+
+	/* does the driver name match? */
+	if (strncmp(readbuf, driver_name, sizeof(readbuf)) != 0)
+		return 0;
+
+	/*
+	 * We might have a situation where the driver is supported, but we don't
+	 * have permissions to do frequency scaling. This error should not be
+	 * handled here, so consider the system to support scaling for now.
+	 */
+	return 1;
+}
diff --git a/lib/librte_power/power_common.h b/lib/librte_power/power_common.h
index feeb5777b7..fab3ca995a 100644
--- a/lib/librte_power/power_common.h
+++ b/lib/librte_power/power_common.h
@@ -7,4 +7,7 @@
 
 #define RTE_POWER_INVALID_FREQ_INDEX (~0)
 
+/* check if scaling driver matches one we want */
+int cpufreq_check_scaling_driver(const char *driver);
+
 #endif /* _POWER_COMMON_H_ */
diff --git a/lib/librte_power/power_kvm_vm.c b/lib/librte_power/power_kvm_vm.c
index 2bb17beb17..409c3e03ab 100644
--- a/lib/librte_power/power_kvm_vm.c
+++ b/lib/librte_power/power_kvm_vm.c
@@ -15,6 +15,11 @@
 
 static struct channel_packet pkt[RTE_MAX_LCORE];
 
+int
+power_kvm_vm_check_supported(void)
+{
+	return guest_channel_host_check_exists(FD_PATH);
+}
 
 int
 power_kvm_vm_init(unsigned int lcore_id)
diff --git a/lib/librte_power/power_kvm_vm.h b/lib/librte_power/power_kvm_vm.h
index 94d4aa1213..73b4c82e57 100644
--- a/lib/librte_power/power_kvm_vm.h
+++ b/lib/librte_power/power_kvm_vm.h
@@ -20,6 +20,16 @@
 extern "C" {
 #endif
 
+/**
+ * Check if KVM power management is supported.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+int power_kvm_vm_check_supported(void);
+
 /**
  * Initialize power management for a specific lcore.
  *
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 2d8a9499dc..2526441a4c 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -71,6 +71,7 @@
 		"/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_PSTATE_DRIVER "intel_pstate"
 #define POWER_MSR_PATH  "/dev/cpu/%u/msr"
 
 /*
@@ -531,6 +532,12 @@ power_get_available_freqs(struct pstate_power_info *pi)
 	return ret;
 }
 
+int
+power_pstate_cpufreq_check_supported(void)
+{
+	return cpufreq_check_scaling_driver(POWER_PSTATE_DRIVER);
+}
+
 int
 power_pstate_cpufreq_init(unsigned int lcore_id)
 {
diff --git a/lib/librte_power/power_pstate_cpufreq.h b/lib/librte_power/power_pstate_cpufreq.h
index 6fd801881f..0be0bd6b81 100644
--- a/lib/librte_power/power_pstate_cpufreq.h
+++ b/lib/librte_power/power_pstate_cpufreq.h
@@ -20,6 +20,16 @@
 extern "C" {
 #endif
 
+/**
+ * Check if pstate power management is supported.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+int power_pstate_cpufreq_check_supported(void);
+
 /**
  * Initialize power management for a specific lcore. It will check and set the
  * governor to performance for the lcore, get the available frequencies, and
diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
index 6b77227275..98eaba9154 100644
--- a/lib/librte_power/rte_power.c
+++ b/lib/librte_power/rte_power.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2010-2014 Intel Corporation
  */
 
+#include <rte_errno.h>
 #include <rte_spinlock.h>
 
 #include "rte_power.h"
@@ -43,6 +44,22 @@ reset_power_function_ptrs(void)
 	rte_power_get_capabilities = NULL;
 }
 
+int
+rte_power_check_env_supported(enum power_management_env env)
+{
+	switch (env) {
+	case PM_ENV_ACPI_CPUFREQ:
+		return power_acpi_cpufreq_check_supported();
+	case PM_ENV_PSTATE_CPUFREQ:
+		return power_pstate_cpufreq_check_supported();
+	case PM_ENV_KVM_VM:
+		return power_kvm_vm_check_supported();
+	default:
+		rte_errno = EINVAL;
+		return -1;
+	}
+}
+
 int
 rte_power_set_env(enum power_management_env env)
 {
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index 427058b811..bbbde4dfb4 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -23,6 +23,24 @@ extern "C" {
 enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
 		PM_ENV_PSTATE_CPUFREQ};
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Check if a specific power management environment type is supported on a
+ * currently running system.
+ *
+ * @param env
+ *   The environment type to check support for.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+__rte_experimental
+int rte_power_check_env_supported(enum power_management_env env);
+
 /**
  * Set the default power management implementation. If this is not called prior
  * to rte_power_init(), then auto-detect of the environment will take place.
diff --git a/lib/librte_power/rte_power_version.map b/lib/librte_power/rte_power_version.map
index 55a168f56e..00ee5753e2 100644
--- a/lib/librte_power/rte_power_version.map
+++ b/lib/librte_power/rte_power_version.map
@@ -26,6 +26,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_empty_poll_detection;
+	rte_power_check_env_supported;
 	rte_power_empty_poll_stat_fetch;
 	rte_power_empty_poll_stat_free;
 	rte_power_empty_poll_stat_init;
-- 
2.17.1

[dpdk-dev] [PATCH v3 7/7] l3fwd-power: add auto-selection of default mode

[Anatoly Burakov]

Currently, the application does support running without the power
library being initialized, but it has to be specifically requested. On
platforms without support for frequency scaling using the power library,
we can just enable interrupt-only mode by default.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Suggested-by: Jerin Jacob <jerinjacobk@gmail.com>
Tested-by: Harman Kalra <hkalra@marvell.com>
---
 examples/l3fwd-power/main.c | 41 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 51acbfd87d..6a4a27984b 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -2412,6 +2412,42 @@ launch_timer(unsigned int lcore_id)
 	return 0;
 }
 
+static int
+autodetect_mode(void)
+{
+	RTE_LOG(NOTICE, L3FWD_POWER, "Operating mode not specified, probing frequency scaling support...\n");
+
+	/*
+	 * Empty poll and telemetry modes have to be specifically requested to
+	 * be enabled, but we can auto-detect between interrupt mode with or
+	 * without frequency scaling. Both ACPI and pstate can be used.
+	 */
+	if (rte_power_check_env_supported(PM_ENV_ACPI_CPUFREQ))
+		return APP_MODE_LEGACY;
+	if (rte_power_check_env_supported(PM_ENV_PSTATE_CPUFREQ))
+		return APP_MODE_LEGACY;
+
+	RTE_LOG(NOTICE, L3FWD_POWER, "Frequency scaling not supported, selecting interrupt-only mode\n");
+
+	return APP_MODE_INTERRUPT;
+}
+
+static const char *
+mode_to_str(enum appmode mode)
+{
+	switch (mode) {
+	case APP_MODE_LEGACY:
+		return "legacy";
+	case APP_MODE_EMPTY_POLL:
+		return "empty poll";
+	case APP_MODE_TELEMETRY:
+		return "telemetry";
+	case APP_MODE_INTERRUPT:
+		return "interrupt-only";
+	default:
+		return "invalid";
+	}
+}
 
 int
 main(int argc, char **argv)
@@ -2449,7 +2485,10 @@ main(int argc, char **argv)
 		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
 
 	if (app_mode == APP_MODE_DEFAULT)
-		app_mode = APP_MODE_LEGACY;
+		app_mode = autodetect_mode();
+
+	RTE_LOG(INFO, L3FWD_POWER, "Selected operation mode: %s\n",
+			mode_to_str(app_mode));
 
 	/* only legacy and empty poll mode rely on power library */
 	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 7/7] l3fwd-power: add auto-selection of default mode

[Thomas Monjalon]

19/06/2020 12:53, Anatoly Burakov:
> Currently, the application does support running without the power
> library being initialized, but it has to be specifically requested. On
> platforms without support for frequency scaling using the power library,
> we can just enable interrupt-only mode by default.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Suggested-by: Jerin Jacob <jerinjacobk@gmail.com>
> Tested-by: Harman Kalra <hkalra@marvell.com>

Please keep the chronological order in tags:
Suggested-by should be first.

[dpdk-dev] [PATCH v2 1/7] l3fwd-power: disable interrupts by default

[Anatoly Burakov]

Currently, interrupts are enabled in telemetry and empty poll modes, but
they are not used. Switch to disabling interrupts by default, and only
enable interrupts for modes that require them.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 9db94ce044..2cc5d7b121 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -256,10 +256,7 @@ static struct rte_eth_conf port_conf = {
 	},
 	.txmode = {
 		.mq_mode = ETH_MQ_TX_NONE,
-	},
-	.intr_conf = {
-		.rxq = 1,
-	},
+	}
 };
 
 static struct rte_mempool * pktmbuf_pool[NB_SOCKETS];
@@ -2270,6 +2267,8 @@ main(int argc, char **argv)
 	/* initialize all ports */
 	RTE_ETH_FOREACH_DEV(portid) {
 		struct rte_eth_conf local_port_conf = port_conf;
+		/* not all app modes need interrupts */
+		bool need_intr = app_mode == APP_MODE_LEGACY;
 
 		/* skip ports that are not enabled */
 		if ((enabled_port_mask & (1 << portid)) == 0) {
@@ -2303,7 +2302,10 @@ main(int argc, char **argv)
 			nb_rx_queue, (unsigned)n_tx_queue );
 		/* If number of Rx queue is 0, no need to enable Rx interrupt */
 		if (nb_rx_queue == 0)
-			local_port_conf.intr_conf.rxq = 0;
+			need_intr = false;
+
+		if (need_intr)
+			local_port_conf.intr_conf.rxq = 1;
 
 		ret = rte_eth_dev_info_get(portid, &dev_info);
 		if (ret != 0)
-- 
2.17.1

[dpdk-dev] [PATCH v2 2/7] l3fwd-power: only allow supported power library envs

[Anatoly Burakov]

Currently, l3fwd-power will attempt to run even if the power env
is set to KVM, which is not supported. Fix this by preventing the
app from initializing unless the env is set to one of the supported
modes.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 2cc5d7b121..5cee9d5387 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -2043,6 +2043,7 @@ static int check_ptype(uint16_t portid)
 static int
 init_power_library(void)
 {
+	enum power_management_env env;
 	unsigned int lcore_id;
 	int ret = 0;
 
@@ -2055,6 +2056,14 @@ init_power_library(void)
 				lcore_id);
 			return ret;
 		}
+		/* we're not supporting the VM channel mode */
+		env = rte_power_get_env();
+		if (env != PM_ENV_ACPI_CPUFREQ &&
+				env != PM_ENV_PSTATE_CPUFREQ) {
+			RTE_LOG(ERR, POWER,
+				"Only ACPI and PSTATE mode are supported\n");
+			return -1;
+		}
 	}
 	return ret;
 }
-- 
2.17.1

[dpdk-dev] [PATCH v2 3/7] l3fwd-power: code style and flow fixes

[Anatoly Burakov]

Make the coding style more consistent, and the init logic control flow
more explicit.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 5cee9d5387..c70611bb7f 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -1126,7 +1126,7 @@ main_empty_poll_loop(__rte_unused void *dummy)
 }
 /* main processing loop */
 static int
-main_loop(__rte_unused void *dummy)
+main_legacy_loop(__rte_unused void *dummy)
 {
 	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
 	unsigned lcore_id;
@@ -1581,6 +1581,7 @@ parse_ep_config(const char *q_arg)
 
 }
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+#define CMD_LINE_OPT_EMPTY_POLL "empty-poll"
 #define CMD_LINE_OPT_TELEMETRY "telemetry"
 
 /* Parse the argument given in the command line of the application */
@@ -1598,7 +1599,7 @@ parse_args(int argc, char **argv)
 		{"high-perf-cores", 1, 0, 0},
 		{"no-numa", 0, 0, 0},
 		{"enable-jumbo", 0, 0, 0},
-		{"empty-poll", 1, 0, 0},
+		{CMD_LINE_OPT_EMPTY_POLL, 1, 0, 0},
 		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
 		{NULL, 0, 0, 0}
@@ -1673,7 +1674,7 @@ parse_args(int argc, char **argv)
 			}
 
 			if (!strncmp(lgopts[option_index].name,
-						"empty-poll", 10)) {
+					CMD_LINE_OPT_EMPTY_POLL, 10)) {
 				if (app_mode == APP_MODE_TELEMETRY) {
 					printf(" empty-poll cannot be enabled as telemetry mode is enabled\n");
 					return -1;
@@ -2253,7 +2254,9 @@ main(int argc, char **argv)
 	if (ret < 0)
 		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
 
-	if (app_mode != APP_MODE_TELEMETRY && init_power_library())
+	/* only legacy and empty poll mode rely on power library */
+	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
+			init_power_library())
 		rte_exit(EXIT_FAILURE, "init_power_library failed\n");
 
 	if (update_lcore_params() < 0)
@@ -2526,12 +2529,12 @@ main(int argc, char **argv)
 
 	/* launch per-lcore init on every lcore */
 	if (app_mode == APP_MODE_LEGACY) {
-		rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
+		rte_eal_mp_remote_launch(main_legacy_loop, NULL, CALL_MASTER);
 	} else if (app_mode == APP_MODE_EMPTY_POLL) {
 		empty_poll_stop = false;
 		rte_eal_mp_remote_launch(main_empty_poll_loop, NULL,
 				SKIP_MASTER);
-	} else {
+	} else if (app_mode == APP_MODE_TELEMETRY) {
 		unsigned int i;
 
 		/* Init metrics library */
@@ -2577,7 +2580,8 @@ main(int argc, char **argv)
 	if (app_mode == APP_MODE_EMPTY_POLL)
 		rte_power_empty_poll_stat_free();
 
-	if (app_mode != APP_MODE_TELEMETRY && deinit_power_library())
+	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
+			deinit_power_library())
 		rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
 
 	if (rte_eal_cleanup() < 0)
-- 
2.17.1

[dpdk-dev] [PATCH v2 4/7] l3fwd-power: add support for requesting legacy mode

[Anatoly Burakov]

Currently, legacy mode is the implicit default, but it is not possible
to directly request using legacy mode. Add the argument to enable
requesting legacy mode, and also make it the default.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index c70611bb7f..c02b0a3574 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -195,7 +195,8 @@ static int parse_ptype; /**< Parse packet type using rx callback, and */
 			/**< disabled by default */
 
 enum appmode {
-	APP_MODE_LEGACY = 0,
+	APP_MODE_DEFAULT = 0,
+	APP_MODE_LEGACY,
 	APP_MODE_EMPTY_POLL,
 	APP_MODE_TELEMETRY
 };
@@ -1435,6 +1436,7 @@ print_usage(const char *prgname)
 		"  --enable-jumbo: enable jumbo frame"
 		" which max packet len is PKTLEN in decimal (64-9600)\n"
 		"  --parse-ptype: parse packet type by software\n"
+		"  --legacy: use legacy interrupt-based scaling\n"
 		"  --empty-poll: enable empty poll detection"
 		" follow (training_flag, high_threshold, med_threshold)\n"
 		" --telemetry: enable telemetry mode, to update"
@@ -1581,6 +1583,7 @@ parse_ep_config(const char *q_arg)
 
 }
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+#define CMD_LINE_OPT_LEGACY "legacy"
 #define CMD_LINE_OPT_EMPTY_POLL "empty-poll"
 #define CMD_LINE_OPT_TELEMETRY "telemetry"
 
@@ -1601,6 +1604,7 @@ parse_args(int argc, char **argv)
 		{"enable-jumbo", 0, 0, 0},
 		{CMD_LINE_OPT_EMPTY_POLL, 1, 0, 0},
 		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
+		{CMD_LINE_OPT_LEGACY, 0, 0, 0},
 		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
@@ -1673,10 +1677,21 @@ parse_args(int argc, char **argv)
 				numa_on = 0;
 			}
 
+			if (!strncmp(lgopts[option_index].name,
+					CMD_LINE_OPT_LEGACY,
+					sizeof(CMD_LINE_OPT_LEGACY))) {
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" legacy mode is mutually exclusive with other modes\n");
+					return -1;
+				}
+				app_mode = APP_MODE_LEGACY;
+				printf("legacy mode is enabled\n");
+			}
+
 			if (!strncmp(lgopts[option_index].name,
 					CMD_LINE_OPT_EMPTY_POLL, 10)) {
-				if (app_mode == APP_MODE_TELEMETRY) {
-					printf(" empty-poll cannot be enabled as telemetry mode is enabled\n");
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" empty-poll mode is mutually exclusive with other modes\n");
 					return -1;
 				}
 				app_mode = APP_MODE_EMPTY_POLL;
@@ -1693,8 +1708,8 @@ parse_args(int argc, char **argv)
 			if (!strncmp(lgopts[option_index].name,
 					CMD_LINE_OPT_TELEMETRY,
 					sizeof(CMD_LINE_OPT_TELEMETRY))) {
-				if (app_mode == APP_MODE_EMPTY_POLL) {
-					printf("telemetry mode cannot be enabled as empty poll mode is enabled\n");
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" telemetry mode is mutually exclusive with other modes\n");
 					return -1;
 				}
 				app_mode = APP_MODE_TELEMETRY;
@@ -2254,6 +2269,9 @@ main(int argc, char **argv)
 	if (ret < 0)
 		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
 
+	if (app_mode == APP_MODE_DEFAULT)
+		app_mode = APP_MODE_LEGACY;
+
 	/* only legacy and empty poll mode rely on power library */
 	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
 			init_power_library())
-- 
2.17.1

[dpdk-dev] [PATCH v2 5/7] l3fwd-power: add interrupt-only mode

[Anatoly Burakov]

In addition to existing modes, add a mode which is very similar to
legacy mode, but does not do frequency scaling, and thus does not
depend on the power library.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 examples/l3fwd-power/main.c | 188 +++++++++++++++++++++++++++++++++++-
 1 file changed, 185 insertions(+), 3 deletions(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index c02b0a3574..51acbfd87d 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -198,7 +198,8 @@ enum appmode {
 	APP_MODE_DEFAULT = 0,
 	APP_MODE_LEGACY,
 	APP_MODE_EMPTY_POLL,
-	APP_MODE_TELEMETRY
+	APP_MODE_TELEMETRY,
+	APP_MODE_INTERRUPT
 };
 
 enum appmode app_mode;
@@ -901,6 +902,170 @@ static int event_register(struct lcore_conf *qconf)
 
 	return 0;
 }
+
+/* main processing loop */
+static int main_intr_loop(__rte_unused void *dummy)
+{
+	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
+	unsigned int lcore_id;
+	uint64_t prev_tsc, diff_tsc, cur_tsc;
+	int i, j, nb_rx;
+	uint8_t queueid;
+	uint16_t portid;
+	struct lcore_conf *qconf;
+	struct lcore_rx_queue *rx_queue;
+	uint32_t lcore_rx_idle_count = 0;
+	uint32_t lcore_idle_hint = 0;
+	int intr_en = 0;
+
+	const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
+				   US_PER_S * BURST_TX_DRAIN_US;
+
+	prev_tsc = 0;
+
+	lcore_id = rte_lcore_id();
+	qconf = &lcore_conf[lcore_id];
+
+	if (qconf->n_rx_queue == 0) {
+		RTE_LOG(INFO, L3FWD_POWER, "lcore %u has nothing to do\n",
+				lcore_id);
+		return 0;
+	}
+
+	RTE_LOG(INFO, L3FWD_POWER, "entering main interrupt loop on lcore %u\n",
+			lcore_id);
+
+	for (i = 0; i < qconf->n_rx_queue; i++) {
+		portid = qconf->rx_queue_list[i].port_id;
+		queueid = qconf->rx_queue_list[i].queue_id;
+		RTE_LOG(INFO, L3FWD_POWER,
+				" -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
+				lcore_id, portid, queueid);
+	}
+
+	/* add into event wait list */
+	if (event_register(qconf) == 0)
+		intr_en = 1;
+	else
+		RTE_LOG(INFO, L3FWD_POWER, "RX interrupt won't enable.\n");
+
+	while (!is_done()) {
+		stats[lcore_id].nb_iteration_looped++;
+
+		cur_tsc = rte_rdtsc();
+
+		/*
+		 * TX burst queue drain
+		 */
+		diff_tsc = cur_tsc - prev_tsc;
+		if (unlikely(diff_tsc > drain_tsc)) {
+			for (i = 0; i < qconf->n_tx_port; ++i) {
+				portid = qconf->tx_port_id[i];
+				rte_eth_tx_buffer_flush(portid,
+						qconf->tx_queue_id[portid],
+						qconf->tx_buffer[portid]);
+			}
+			prev_tsc = cur_tsc;
+		}
+
+start_rx:
+		/*
+		 * Read packet from RX queues
+		 */
+		lcore_rx_idle_count = 0;
+		for (i = 0; i < qconf->n_rx_queue; ++i) {
+			rx_queue = &(qconf->rx_queue_list[i]);
+			rx_queue->idle_hint = 0;
+			portid = rx_queue->port_id;
+			queueid = rx_queue->queue_id;
+
+			nb_rx = rte_eth_rx_burst(portid, queueid, pkts_burst,
+					MAX_PKT_BURST);
+
+			stats[lcore_id].nb_rx_processed += nb_rx;
+			if (unlikely(nb_rx == 0)) {
+				/**
+				 * no packet received from rx queue, try to
+				 * sleep for a while forcing CPU enter deeper
+				 * C states.
+				 */
+				rx_queue->zero_rx_packet_count++;
+
+				if (rx_queue->zero_rx_packet_count <=
+						MIN_ZERO_POLL_COUNT)
+					continue;
+
+				rx_queue->idle_hint = power_idle_heuristic(
+						rx_queue->zero_rx_packet_count);
+				lcore_rx_idle_count++;
+			} else {
+				rx_queue->zero_rx_packet_count = 0;
+			}
+
+			/* Prefetch first packets */
+			for (j = 0; j < PREFETCH_OFFSET && j < nb_rx; j++) {
+				rte_prefetch0(rte_pktmbuf_mtod(
+						pkts_burst[j], void *));
+			}
+
+			/* Prefetch and forward already prefetched packets */
+			for (j = 0; j < (nb_rx - PREFETCH_OFFSET); j++) {
+				rte_prefetch0(rte_pktmbuf_mtod(
+						pkts_burst[j + PREFETCH_OFFSET],
+						void *));
+				l3fwd_simple_forward(
+						pkts_burst[j], portid, qconf);
+			}
+
+			/* Forward remaining prefetched packets */
+			for (; j < nb_rx; j++) {
+				l3fwd_simple_forward(
+						pkts_burst[j], portid, qconf);
+			}
+		}
+
+		if (unlikely(lcore_rx_idle_count == qconf->n_rx_queue)) {
+			/**
+			 * All Rx queues empty in recent consecutive polls,
+			 * sleep in a conservative manner, meaning sleep as
+			 * less as possible.
+			 */
+			for (i = 1,
+			    lcore_idle_hint = qconf->rx_queue_list[0].idle_hint;
+					i < qconf->n_rx_queue; ++i) {
+				rx_queue = &(qconf->rx_queue_list[i]);
+				if (rx_queue->idle_hint < lcore_idle_hint)
+					lcore_idle_hint = rx_queue->idle_hint;
+			}
+
+			if (lcore_idle_hint < SUSPEND_THRESHOLD)
+				/**
+				 * execute "pause" instruction to avoid context
+				 * switch which generally take hundred of
+				 * microseconds for short sleep.
+				 */
+				rte_delay_us(lcore_idle_hint);
+			else {
+				/* suspend until rx interrupt triggers */
+				if (intr_en) {
+					turn_on_off_intr(qconf, 1);
+					sleep_until_rx_interrupt(
+							qconf->n_rx_queue);
+					turn_on_off_intr(qconf, 0);
+					/**
+					 * start receiving packets immediately
+					 */
+					if (likely(!is_done()))
+						goto start_rx;
+				}
+			}
+			stats[lcore_id].sleep_time += lcore_idle_hint;
+		}
+	}
+
+	return 0;
+}
+
 /* main processing loop */
 static int
 main_telemetry_loop(__rte_unused void *dummy)
@@ -1440,7 +1605,8 @@ print_usage(const char *prgname)
 		"  --empty-poll: enable empty poll detection"
 		" follow (training_flag, high_threshold, med_threshold)\n"
 		" --telemetry: enable telemetry mode, to update"
-		" empty polls, full polls, and core busyness to telemetry\n",
+		" empty polls, full polls, and core busyness to telemetry\n"
+		" --interrupt-only: enable interrupt-only mode\n",
 		prgname);
 }
 
@@ -1585,6 +1751,7 @@ parse_ep_config(const char *q_arg)
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
 #define CMD_LINE_OPT_LEGACY "legacy"
 #define CMD_LINE_OPT_EMPTY_POLL "empty-poll"
+#define CMD_LINE_OPT_INTERRUPT_ONLY "interrupt-only"
 #define CMD_LINE_OPT_TELEMETRY "telemetry"
 
 /* Parse the argument given in the command line of the application */
@@ -1606,6 +1773,7 @@ parse_args(int argc, char **argv)
 		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{CMD_LINE_OPT_LEGACY, 0, 0, 0},
 		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
+		{CMD_LINE_OPT_INTERRUPT_ONLY, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -1716,6 +1884,17 @@ parse_args(int argc, char **argv)
 				printf("telemetry mode is enabled\n");
 			}
 
+			if (!strncmp(lgopts[option_index].name,
+					CMD_LINE_OPT_INTERRUPT_ONLY,
+					sizeof(CMD_LINE_OPT_INTERRUPT_ONLY))) {
+				if (app_mode != APP_MODE_DEFAULT) {
+					printf(" interrupt-only mode is mutually exclusive with other modes\n");
+					return -1;
+				}
+				app_mode = APP_MODE_INTERRUPT;
+				printf("interrupt-only mode is enabled\n");
+			}
+
 			if (!strncmp(lgopts[option_index].name,
 					"enable-jumbo", 12)) {
 				struct option lenopts =
@@ -2298,7 +2477,8 @@ main(int argc, char **argv)
 	RTE_ETH_FOREACH_DEV(portid) {
 		struct rte_eth_conf local_port_conf = port_conf;
 		/* not all app modes need interrupts */
-		bool need_intr = app_mode == APP_MODE_LEGACY;
+		bool need_intr = app_mode == APP_MODE_LEGACY ||
+				app_mode == APP_MODE_INTERRUPT;
 
 		/* skip ports that are not enabled */
 		if ((enabled_port_mask & (1 << portid)) == 0) {
@@ -2576,6 +2756,8 @@ main(int argc, char **argv)
 				"Returns global power stats. Parameters: None");
 		rte_eal_mp_remote_launch(main_telemetry_loop, NULL,
 						SKIP_MASTER);
+	} else if (app_mode == APP_MODE_INTERRUPT) {
+		rte_eal_mp_remote_launch(main_intr_loop, NULL, CALL_MASTER);
 	}
 
 	if (app_mode == APP_MODE_EMPTY_POLL || app_mode == APP_MODE_TELEMETRY)
-- 
2.17.1

[dpdk-dev] [PATCH v2 6/7] power: add API to probe support for a specific env

[Anatoly Burakov]

Currently, there is no way to know if the power management env is
supported without trying to initialize it. The init API also does
not distinguish between failure due to some error and failure due to
power management not being available on the platform in the first
place.

Thus, add an API that provides capability of probing support for a
specific power management API.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Suggested-by: Jerin Jacob <jerinjacobk@gmail.com>
---
 lib/librte_power/Makefile               |  1 +
 lib/librte_power/guest_channel.c        | 26 +++++++++++++
 lib/librte_power/guest_channel.h        | 12 ++++++
 lib/librte_power/meson.build            |  3 +-
 lib/librte_power/power_acpi_cpufreq.c   |  7 ++++
 lib/librte_power/power_acpi_cpufreq.h   | 10 +++++
 lib/librte_power/power_common.c         | 52 +++++++++++++++++++++++++
 lib/librte_power/power_common.h         |  3 ++
 lib/librte_power/power_kvm_vm.c         |  5 +++
 lib/librte_power/power_kvm_vm.h         | 10 +++++
 lib/librte_power/power_pstate_cpufreq.c |  7 ++++
 lib/librte_power/power_pstate_cpufreq.h | 10 +++++
 lib/librte_power/rte_power.c            | 17 ++++++++
 lib/librte_power/rte_power.h            | 18 +++++++++
 lib/librte_power/rte_power_version.map  |  1 +
 15 files changed, 181 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_power/power_common.c

diff --git a/lib/librte_power/Makefile b/lib/librte_power/Makefile
index 087d643ee5..3b067b615f 100644
--- a/lib/librte_power/Makefile
+++ b/lib/librte_power/Makefile
@@ -16,6 +16,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_POWER) := rte_power.c power_acpi_cpufreq.c
 SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_kvm_vm.c guest_channel.c
 SRCS-$(CONFIG_RTE_LIBRTE_POWER) += rte_power_empty_poll.c
 SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_pstate_cpufreq.c
+SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_common.c
 
 # install this header file
 SYMLINK-$(CONFIG_RTE_LIBRTE_POWER)-include := rte_power.h  rte_power_empty_poll.h
diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
index b984d55bc8..7b5926e5c4 100644
--- a/lib/librte_power/guest_channel.c
+++ b/lib/librte_power/guest_channel.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2010-2014 Intel Corporation
  */
 
+#include <glob.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -25,6 +26,31 @@
 
 static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
 
+int
+guest_channel_host_check_exists(const char *path)
+{
+	char glob_path[PATH_MAX];
+	glob_t g;
+	int ret;
+
+	/* we cannot know in advance which cores have VM channels, so glob */
+	snprintf(glob_path, PATH_MAX, "%s.*", path);
+
+	ret = glob(glob_path, GLOB_NOSORT, NULL, &g);
+	if (ret != 0) {
+		/* couldn't read anything */
+		ret = 0;
+		goto out;
+	}
+
+	/* do we have at least one match? */
+	ret = g.gl_pathc > 0;
+
+out:
+	globfree(&g);
+	return ret;
+}
+
 int
 guest_channel_host_connect(const char *path, unsigned int lcore_id)
 {
diff --git a/lib/librte_power/guest_channel.h b/lib/librte_power/guest_channel.h
index 025961606c..e15db46fc7 100644
--- a/lib/librte_power/guest_channel.h
+++ b/lib/librte_power/guest_channel.h
@@ -10,6 +10,18 @@ extern "C" {
 
 #include <channel_commands.h>
 
+/**
+ * Check if any Virtio-Serial VM end-points exist in path.
+ *
+ * @param path
+ *  The path to the serial device on the filesystem
+ *
+ * @return
+ *  - 1 if at least one potential end-point found.
+ *  - 0 if no end-points found.
+ */
+int guest_channel_host_check_exists(const char *path);
+
 /**
  * Connect to the Virtio-Serial VM end-point located in path. It is
  * thread safe for unique lcore_ids. This function must be only called once from
diff --git a/lib/librte_power/meson.build b/lib/librte_power/meson.build
index cdf08f6df3..78c031c943 100644
--- a/lib/librte_power/meson.build
+++ b/lib/librte_power/meson.build
@@ -8,6 +8,7 @@ endif
 sources = files('rte_power.c', 'power_acpi_cpufreq.c',
 		'power_kvm_vm.c', 'guest_channel.c',
 		'rte_power_empty_poll.c',
-		'power_pstate_cpufreq.c')
+		'power_pstate_cpufreq.c',
+		'power_common.c')
 headers = files('rte_power.h','rte_power_empty_poll.h')
 deps += ['timer']
diff --git a/lib/librte_power/power_acpi_cpufreq.c b/lib/librte_power/power_acpi_cpufreq.c
index f443fce69f..583815a07e 100644
--- a/lib/librte_power/power_acpi_cpufreq.c
+++ b/lib/librte_power/power_acpi_cpufreq.c
@@ -59,6 +59,7 @@
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_frequencies"
 #define POWER_SYSFILE_SETSPEED   \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_setspeed"
+#define POWER_ACPI_DRIVER "acpi-cpufreq"
 
 /*
  * MSR related
@@ -289,6 +290,12 @@ power_init_for_setting_freq(struct rte_power_info *pi)
 	return -1;
 }
 
+int
+power_acpi_cpufreq_check_supported(void)
+{
+	return cpufreq_check_scaling_driver(POWER_ACPI_DRIVER);
+}
+
 int
 power_acpi_cpufreq_init(unsigned int lcore_id)
 {
diff --git a/lib/librte_power/power_acpi_cpufreq.h b/lib/librte_power/power_acpi_cpufreq.h
index 1af7416073..038857b3a6 100644
--- a/lib/librte_power/power_acpi_cpufreq.h
+++ b/lib/librte_power/power_acpi_cpufreq.h
@@ -20,6 +20,16 @@
 extern "C" {
 #endif
 
+/**
+ * Check if ACPI power management is supported.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+int power_acpi_cpufreq_check_supported(void);
+
 /**
  * Initialize power management for a specific lcore. It will check and set the
  * governor to userspace for the lcore, get the available frequencies, and
diff --git a/lib/librte_power/power_common.c b/lib/librte_power/power_common.c
new file mode 100644
index 0000000000..59023d986b
--- /dev/null
+++ b/lib/librte_power/power_common.c
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+
+#include <limits.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "power_common.h"
+
+#define POWER_SYSFILE_SCALING_DRIVER   \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
+
+int
+cpufreq_check_scaling_driver(const char *driver_name)
+{
+	unsigned int lcore_id = 0; /* always check core 0 */
+	char fullpath[PATH_MAX];
+	char readbuf[PATH_MAX];
+	char *s;
+	FILE *f;
+
+	/*
+	 * Check if scaling driver matches what we expect.
+	 */
+	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SCALING_DRIVER,
+			lcore_id);
+	f = fopen(fullpath, "r");
+
+	/* if there's no driver at all, bail out */
+	if (f == NULL)
+		return 0;
+
+	s = fgets(readbuf, sizeof(readbuf), f);
+	/* don't need it any more */
+	fclose(f);
+
+	/* if we can't read it, consider unsupported */
+	if (s == NULL)
+		return 0;
+
+	/* does the driver name match? */
+	if (strncmp(readbuf, driver_name, sizeof(readbuf)) != 0)
+		return 0;
+
+	/*
+	 * We might have a situation where the driver is supported, but we don't
+	 * have permissions to do frequency scaling. This error should not be
+	 * handled here, so consider the system to support scaling for now.
+	 */
+	return 1;
+}
diff --git a/lib/librte_power/power_common.h b/lib/librte_power/power_common.h
index feeb5777b7..fab3ca995a 100644
--- a/lib/librte_power/power_common.h
+++ b/lib/librte_power/power_common.h
@@ -7,4 +7,7 @@
 
 #define RTE_POWER_INVALID_FREQ_INDEX (~0)
 
+/* check if scaling driver matches one we want */
+int cpufreq_check_scaling_driver(const char *driver);
+
 #endif /* _POWER_COMMON_H_ */
diff --git a/lib/librte_power/power_kvm_vm.c b/lib/librte_power/power_kvm_vm.c
index 2bb17beb17..409c3e03ab 100644
--- a/lib/librte_power/power_kvm_vm.c
+++ b/lib/librte_power/power_kvm_vm.c
@@ -15,6 +15,11 @@
 
 static struct channel_packet pkt[RTE_MAX_LCORE];
 
+int
+power_kvm_vm_check_supported(void)
+{
+	return guest_channel_host_check_exists(FD_PATH);
+}
 
 int
 power_kvm_vm_init(unsigned int lcore_id)
diff --git a/lib/librte_power/power_kvm_vm.h b/lib/librte_power/power_kvm_vm.h
index 94d4aa1213..73b4c82e57 100644
--- a/lib/librte_power/power_kvm_vm.h
+++ b/lib/librte_power/power_kvm_vm.h
@@ -20,6 +20,16 @@
 extern "C" {
 #endif
 
+/**
+ * Check if KVM power management is supported.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+int power_kvm_vm_check_supported(void);
+
 /**
  * Initialize power management for a specific lcore.
  *
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 2d8a9499dc..2526441a4c 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -71,6 +71,7 @@
 		"/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_PSTATE_DRIVER "intel_pstate"
 #define POWER_MSR_PATH  "/dev/cpu/%u/msr"
 
 /*
@@ -531,6 +532,12 @@ power_get_available_freqs(struct pstate_power_info *pi)
 	return ret;
 }
 
+int
+power_pstate_cpufreq_check_supported(void)
+{
+	return cpufreq_check_scaling_driver(POWER_PSTATE_DRIVER);
+}
+
 int
 power_pstate_cpufreq_init(unsigned int lcore_id)
 {
diff --git a/lib/librte_power/power_pstate_cpufreq.h b/lib/librte_power/power_pstate_cpufreq.h
index 6fd801881f..0be0bd6b81 100644
--- a/lib/librte_power/power_pstate_cpufreq.h
+++ b/lib/librte_power/power_pstate_cpufreq.h
@@ -20,6 +20,16 @@
 extern "C" {
 #endif
 
+/**
+ * Check if pstate power management is supported.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+int power_pstate_cpufreq_check_supported(void);
+
 /**
  * Initialize power management for a specific lcore. It will check and set the
  * governor to performance for the lcore, get the available frequencies, and
diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
index 6b77227275..98eaba9154 100644
--- a/lib/librte_power/rte_power.c
+++ b/lib/librte_power/rte_power.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2010-2014 Intel Corporation
  */
 
+#include <rte_errno.h>
 #include <rte_spinlock.h>
 
 #include "rte_power.h"
@@ -43,6 +44,22 @@ reset_power_function_ptrs(void)
 	rte_power_get_capabilities = NULL;
 }
 
+int
+rte_power_check_env_supported(enum power_management_env env)
+{
+	switch (env) {
+	case PM_ENV_ACPI_CPUFREQ:
+		return power_acpi_cpufreq_check_supported();
+	case PM_ENV_PSTATE_CPUFREQ:
+		return power_pstate_cpufreq_check_supported();
+	case PM_ENV_KVM_VM:
+		return power_kvm_vm_check_supported();
+	default:
+		rte_errno = EINVAL;
+		return -1;
+	}
+}
+
 int
 rte_power_set_env(enum power_management_env env)
 {
diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
index 427058b811..bbbde4dfb4 100644
--- a/lib/librte_power/rte_power.h
+++ b/lib/librte_power/rte_power.h
@@ -23,6 +23,24 @@ extern "C" {
 enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
 		PM_ENV_PSTATE_CPUFREQ};
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Check if a specific power management environment type is supported on a
+ * currently running system.
+ *
+ * @param env
+ *   The environment type to check support for.
+ *
+ * @return
+ *   - 1 if supported
+ *   - 0 if unsupported
+ *   - -1 if error, with rte_errno indicating reason for error.
+ */
+__rte_experimental
+int rte_power_check_env_supported(enum power_management_env env);
+
 /**
  * Set the default power management implementation. If this is not called prior
  * to rte_power_init(), then auto-detect of the environment will take place.
diff --git a/lib/librte_power/rte_power_version.map b/lib/librte_power/rte_power_version.map
index 55a168f56e..00ee5753e2 100644
--- a/lib/librte_power/rte_power_version.map
+++ b/lib/librte_power/rte_power_version.map
@@ -26,6 +26,7 @@ EXPERIMENTAL {
 	global:
 
 	rte_empty_poll_detection;
+	rte_power_check_env_supported;
 	rte_power_empty_poll_stat_fetch;
 	rte_power_empty_poll_stat_free;
 	rte_power_empty_poll_stat_init;
-- 
2.17.1

[dpdk-dev] [PATCH v2 7/7] l3fwd-power: add auto-selection of default mode

[Anatoly Burakov]

Currently, the application does support running without the power
library being initialized, but it has to be specifically requested. On
platforms without support for frequency scaling using the power library,
we can just enable interrupt-only mode by default.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Suggested-by: Jerin Jacob <jerinjacobk@gmail.com>
---
 examples/l3fwd-power/main.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 51acbfd87d..a66599e734 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -2412,6 +2412,20 @@ launch_timer(unsigned int lcore_id)
 	return 0;
 }
 
+static int
+autodetect_mode(void)
+{
+	/*
+	 * Empty poll and telemetry modes have to be specifically requested to
+	 * be enabled, but we can auto-detect between legacy mode with or
+	 * without interrupts. Both ACPI and pstate can be used.
+	 */
+	if (rte_power_check_env_supported(PM_ENV_ACPI_CPUFREQ))
+		return APP_MODE_LEGACY;
+	if (rte_power_check_env_supported(PM_ENV_PSTATE_CPUFREQ))
+		return APP_MODE_LEGACY;
+	return APP_MODE_INTERRUPT;
+}
 
 int
 main(int argc, char **argv)
@@ -2449,7 +2463,7 @@ main(int argc, char **argv)
 		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
 
 	if (app_mode == APP_MODE_DEFAULT)
-		app_mode = APP_MODE_LEGACY;
+		app_mode = autodetect_mode();
 
 	/* only legacy and empty poll mode rely on power library */
 	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
-- 
2.17.1

Re: [dpdk-dev] [EXT] [PATCH v2 7/7] l3fwd-power: add auto-selection of default mode

[Harman Kalra]

On Thu, Jun 18, 2020 at 06:18:29PM +0100, Anatoly Burakov wrote:
> External Email
> 
> ----------------------------------------------------------------------
> Currently, the application does support running without the power
> library being initialized, but it has to be specifically requested. On
> platforms without support for frequency scaling using the power library,
> we can just enable interrupt-only mode by default.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Suggested-by: Jerin Jacob <jerinjacobk@gmail.com>
> ---

Application probed the platform for frequency scaling support, since
octeontx2 doesnt support it, interrupt-only mode got enabled by default.

Tested-by: Harman Kalra <hkalra@marvell.com>	
	
>  examples/l3fwd-power/main.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index 51acbfd87d..a66599e734 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -2412,6 +2412,20 @@ launch_timer(unsigned int lcore_id)
>  	return 0;
>  }
>  
> +static int
> +autodetect_mode(void)
> +{
> +	/*
> +	 * Empty poll and telemetry modes have to be specifically requested to
> +	 * be enabled, but we can auto-detect between legacy mode with or
> +	 * without interrupts. Both ACPI and pstate can be used.
> +	 */
> +	if (rte_power_check_env_supported(PM_ENV_ACPI_CPUFREQ))
> +		return APP_MODE_LEGACY;
> +	if (rte_power_check_env_supported(PM_ENV_PSTATE_CPUFREQ))
> +		return APP_MODE_LEGACY;
> +	return APP_MODE_INTERRUPT;
> +}
>  
>  int
>  main(int argc, char **argv)
> @@ -2449,7 +2463,7 @@ main(int argc, char **argv)
>  		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>  
>  	if (app_mode == APP_MODE_DEFAULT)
> -		app_mode = APP_MODE_LEGACY;
> +		app_mode = autodetect_mode();
>  
>  	/* only legacy and empty poll mode rely on power library */
>  	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> -- 
> 2.17.1

Re: [dpdk-dev] [EXT] [PATCH v2 7/7] l3fwd-power: add auto-selection of default mode

[Burakov, Anatoly]

On 19-Jun-20 8:37 AM, Harman Kalra wrote:
> On Thu, Jun 18, 2020 at 06:18:29PM +0100, Anatoly Burakov wrote:
>> External Email
>>
>> ----------------------------------------------------------------------
>> Currently, the application does support running without the power
>> library being initialized, but it has to be specifically requested. On
>> platforms without support for frequency scaling using the power library,
>> we can just enable interrupt-only mode by default.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Suggested-by: Jerin Jacob <jerinjacobk@gmail.com>
>> ---
> 
> Application probed the platform for frequency scaling support, since
> octeontx2 doesnt support it, interrupt-only mode got enabled by default.
> 
> Tested-by: Harman Kalra <hkalra@marvell.com>	

Perhaps there should be a log indicating which one was picked. There's a 
wrong comment in this particular patch as well, so i'll fix that while 
i'm at it :)

Thanks!

> 	
>>   examples/l3fwd-power/main.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
>> index 51acbfd87d..a66599e734 100644
>> --- a/examples/l3fwd-power/main.c
>> +++ b/examples/l3fwd-power/main.c
>> @@ -2412,6 +2412,20 @@ launch_timer(unsigned int lcore_id)
>>   	return 0;
>>   }
>>   
>> +static int
>> +autodetect_mode(void)
>> +{
>> +	/*
>> +	 * Empty poll and telemetry modes have to be specifically requested to
>> +	 * be enabled, but we can auto-detect between legacy mode with or
>> +	 * without interrupts. Both ACPI and pstate can be used.
>> +	 */
>> +	if (rte_power_check_env_supported(PM_ENV_ACPI_CPUFREQ))
>> +		return APP_MODE_LEGACY;
>> +	if (rte_power_check_env_supported(PM_ENV_PSTATE_CPUFREQ))
>> +		return APP_MODE_LEGACY;
>> +	return APP_MODE_INTERRUPT;
>> +}
>>   
>>   int
>>   main(int argc, char **argv)
>> @@ -2449,7 +2463,7 @@ main(int argc, char **argv)
>>   		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
>>   
>>   	if (app_mode == APP_MODE_DEFAULT)
>> -		app_mode = APP_MODE_LEGACY;
>> +		app_mode = autodetect_mode();
>>   
>>   	/* only legacy and empty poll mode rely on power library */
>>   	if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
>> -- 
>> 2.17.1


-- 
Thanks,
Anatoly