Add interrupt-only mode to l3fwd-power

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 <[email protected]>

---
 examples/l3fwd-power/main.c | 215 +++++++++++++++++++++++++++++++++---
 1 file changed, 202 insertions(+), 13 deletions(-)

examples/l3fwd-power/main.c +188 -8

index 5cee9d5387..4161e01974 100644
@@ -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");

examples/l3fwd-power/main.c +8 -4

 	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)

Signature · 1 line

2.17.1

examples/l3fwd-power/main.c main(int argc, char **argv) · line 2442

Quoted 12 lines

  	if (ret < 0)
  		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

examples/l3fwd-power/main.c +6 -1

-	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.

External Email

----------------------------------------------------------------------

Quoted 12 lines · Harman Kalra

examples/l3fwd-power/main.c main(int argc, char **argv) · line 2442

Quoted 10 lines

  	if (ret < 0)
  		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

examples/l3fwd-power/main.c +6 -1

-	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,

Quoted 3 lines

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.

Signature · 2 lines

Thanks,
Anatoly

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 12 lines · Harman Kalra

examples/l3fwd-power/main.c main(int argc, char **argv) · line 2442

Quoted 10 lines

   	if (ret < 0)
   		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

examples/l3fwd-power/main.c +6 -1

-	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,

Quoted 3 lines

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.

Quoted 8 lines

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

Quoted 3 lines

Signature · 2 lines

Thanks,
Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 12 lines · Harman Kalra

examples/l3fwd-power/main.c main(int argc, char **argv) · line 2442

Quoted 10 lines

   	if (ret < 0)
   		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

examples/l3fwd-power/main.c +6 -1

-	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,

Quoted 3 lines

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.

Quoted 8 lines

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".

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Signature · 2 lines

Thanks,
Anatoly

Signature · 2 lines

Thanks,
Anatoly

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 12 lines · Harman Kalra

examples/l3fwd-power/main.c main(int argc, char **argv) · line 2442

Quoted 10 lines

   	if (ret < 0)
   		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

examples/l3fwd-power/main.c +6 -1

-	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,

Quoted 3 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Signature · 2 lines

Thanks,
Anatoly

Signature · 2 lines

Thanks,
Anatoly

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 12 lines · Harman Kalra

Review

Quoted 10 lines

    	if (ret < 0)
    		rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

(quoted diff) +6 -1

-	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,

Quoted 3 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

			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.

Quoted 13 lines

Thanks
Harman

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Signature · 2 lines

Thanks,
Anatoly

Signature · 2 lines

Thanks,
Anatoly

Quoted 75 lines · Harman Kalra

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 10 lines · Harman Kalra

Review

Quoted 10 lines

        if (ret < 0)
                rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

(quoted diff) +6 -1

-       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");

Quoted 5 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

                     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);
    }

Quoted 2 lines

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.

Quoted 6 lines

Quoted 6 lines

Thanks
Harman

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

--
Thanks,
Anatoly

--
Thanks,
Anatoly

--
Thanks,
Anatoly

Quoted 77 lines · Anatoly

Quoted 75 lines · Harman Kalra

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 10 lines · Harman Kalra

Review

Quoted 10 lines

         if (ret < 0)
                 rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

(quoted diff) +6 -1

-       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");

Quoted 5 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

                      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);
     }

Quoted 6 lines

Quoted 2 lines

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.

Quoted 90 lines · Jerin Jacob

Quoted 77 lines · Anatoly

Quoted 75 lines · Harman Kalra

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 10 lines · Harman Kalra

Review

Quoted 10 lines

         if (ret < 0)
                 rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

(quoted diff) +6 -1

-       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");

Quoted 5 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

                      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);
     }

Quoted 6 lines

Quoted 2 lines

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:

Quoted 5 lines

# 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

Quoted 102 lines · Anatoly

Quoted 90 lines · Jerin Jacob

Quoted 77 lines · Anatoly

Quoted 75 lines · Harman Kalra

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 10 lines · Harman Kalra

Review

Quoted 10 lines

          if (ret < 0)
                  rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

(quoted diff) +6 -1

-       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");

Quoted 5 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

                       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);
      }

Quoted 6 lines

Quoted 2 lines

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:

Quoted 5 lines

# 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.

Quoted 2 lines

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.

Quoted 16 lines

"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.

Quoted 4 lines

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.

Quoted 107 lines · Jerin Jacob

Quoted 104 lines · Anatoly

Quoted 90 lines · Jerin Jacob

Quoted 77 lines · Anatoly

Quoted 75 lines · Harman Kalra

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 10 lines · Harman Kalra

Review

Quoted 10 lines

          if (ret < 0)
                  rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");

(quoted diff) +6 -1

-       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");

Quoted 5 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

                       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);
      }

Quoted 6 lines

Quoted 2 lines

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:

Quoted 5 lines

# 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.

Quoted 2 lines

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*.

Quoted 26 lines

Quoted 17 lines

"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.

Quoted 4 lines

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.

Quoted 7 lines

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

Quoted 109 lines · Anatoly

Quoted 107 lines · Jerin Jacob

Quoted 104 lines · Anatoly

Quoted 90 lines · Jerin Jacob

Quoted 77 lines · Anatoly

Quoted 75 lines · Harman Kalra

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 10 lines · Harman Kalra

Quoted 10 lines

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");

Quoted 5 lines

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

                        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);
       }

Quoted 6 lines

Quoted 2 lines

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:

Quoted 5 lines

# 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.

Quoted 8 lines

Quoted 2 lines

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,

Quoted 50 lines

Quoted 26 lines

Quoted 17 lines

"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.

Quoted 4 lines

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.

Quoted 7 lines

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).

Quoted 4 lines

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?

Quoted 180 lines · Jerin Jacob

Quoted 109 lines · Anatoly

Quoted 107 lines · Jerin Jacob

Quoted 104 lines · Anatoly

Quoted 90 lines · Jerin Jacob

Quoted 77 lines · Anatoly

Quoted 75 lines · Harman Kalra

Quoted 64 lines · Harman Kalra

Quoted 45 lines · Anatoly

Quoted 44 lines · Harman Kalra

Quoted 21 lines · Anatoly

External Email

----------------------------------------------------------------------

Quoted 15 lines · Harman Kalra

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.

Quoted 8 lines

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".

Quoted 8 lines

Quoted 4 lines

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",

Quoted 2 lines

                        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);
       }

Quoted 6 lines

Quoted 2 lines

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:

Quoted 5 lines

# 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.

Quoted 8 lines

Quoted 2 lines

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,

Quoted 50 lines

Quoted 26 lines

Quoted 17 lines

"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.

Quoted 4 lines

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.

Quoted 7 lines

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).

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 9 lines

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

-----Original Message-----
From: dev <[email protected]> On Behalf Of Anatoly Burakov
Sent: 2020年5月28日 17:14
To: [email protected]
Cc: Hunt, David <[email protected]>; Ma, Liang J <[email protected]>; Pattan, Reshma <[email protected]>
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

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 <[email protected]>

Suggested-by Jerin Jacob <[email protected]>

---

 examples/l3fwd-power/main.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

examples/l3fwd-power/main.c +15 -1

index 51acbfd87d..a66599e734 100644
@@ -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) &&

Signature · 1 line

2.17.1

Quoted 8 lines · Anatoly Burakov

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.

Quoted 3 lines

Signed-off-by Anatoly Burakov <[email protected]>

Suggested-by Jerin Jacob <[email protected]>

---

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

Tested-by Harman Kalra <[email protected]>

Quoted 40 lines

	
  examples/l3fwd-power/main.c | 16 +++++++++++++++-
  1 file changed, 15 insertions(+), 1 deletion(-)

examples/l3fwd-power/main.c +15 -1

index 51acbfd87d..a66599e734 100644
@@ -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) &&

Signature · 1 line

2.17.1

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 <[email protected]>

Suggested-by Jerin Jacob <[email protected]>

Tested-by Harman Kalra <[email protected]>

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