Re: [PATCH v2 4/4] examples/l3fwd_power: add cli for configurable options

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 19-Apr-22 12:25 PM, Kevin Laatz wrote:
> Add CLI options to l3fwd_power to utilize the new power APIs introduced in
> this patchset. These CLI options allow the user to configure the
> heuritstics made available through the new API via the l3fwd_power
> application options.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> 
> ---
> v2: add doc update for l3fwd-power
> ---

This is a bit confusing. There are other scaling modes in l3fwd-power, 
but the --scale-freq-min only applies to the PMD power management mode.

>   .../sample_app_ug/l3_forward_power_man.rst    |  8 ++
>   examples/l3fwd-power/main.c                   | 75 ++++++++++++++++++-
>   2 files changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/sample_app_ug/l3_forward_power_man.rst b/doc/guides/sample_app_ug/l3_forward_power_man.rst
> index 2e350c45f1..b26f11a220 100644
> --- a/doc/guides/sample_app_ug/l3_forward_power_man.rst
> +++ b/doc/guides/sample_app_ug/l3_forward_power_man.rst
> @@ -109,6 +109,14 @@ where,
>   
>   *   --pmd-mgmt: PMD power management mode.
>   
> +*   --max-empty-polls : Number of empty polls to wait before entering sleep state.
> +
> +*   --pause-duration: Set the duration of the pause callback (microseconds).
> +
> +*   --scale-freq-min: Set minimum frequency for scaling.
> +
> +*   --scale-freq-max: Set maximum frequency for scaling.
> +

Maybe make a note that these only apply to --pmd-mgmt mode?

>   See :doc:`l3_forward` for details.
>   The L3fwd-power example reuses the L3fwd command line options.
>   
> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index 20e5b59af9..ec2a71f02f 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -265,6 +265,10 @@ static struct rte_eth_conf port_conf = {
>   };
>   
>   static uint32_t max_pkt_len;
> +static uint32_t max_empty_polls;
> +static uint32_t pause_duration;
> +static uint32_t scale_freq_min;
> +static uint32_t scale_freq_max;
>   
>   static struct rte_mempool * pktmbuf_pool[NB_SOCKETS];
>   
> @@ -1626,10 +1630,32 @@ print_usage(const char *prgname)
>   		" empty polls, full polls, and core busyness to telemetry\n"
>   		" --interrupt-only: enable interrupt-only mode\n"
>   		" --pmd-mgmt MODE: enable PMD power management mode. "
> -		"Currently supported modes: baseline, monitor, pause, scale\n",
> +		"Currently supported modes: baseline, monitor, pause, scale\n"
> +		"  --max-empty-polls MAX_EMPTY_POLLS: number of empty polls to"
> +		" wait before entering sleep state\n"
> +		"  --pause_duration DURATION: set the duration, in microseconds,"
> +		" of the pause callback\n"
> +		"  --scale-freq-min FREQ_MIN: set minimum frequency for scaling"
> +		" mode for all application lcores\n"
> +		"  --scale_freq_max FREQ_MAX: set maximum frequency for scaling"
> +		" mode for all application lcores\n",

It would be useful to note which units are used here :) This is 
increments in 100MHz, correct?

>   		prgname);
>   }
>   
> +static int
> +parse_int(const char *opt)
> +{
> +	char *end = NULL;
> +	unsigned long val;
> +
> +	/* parse integer string */
> +	val = strtoul(opt, &end, 10);
> +	if ((opt[0] == '\0') || (end == NULL) || (*end != '\0'))
> +		return -1;
> +
> +	return val;
> +}
> +
>   static int parse_max_pkt_len(const char *pktlen)
>   {
>   	char *end = NULL;
> @@ -1803,6 +1829,10 @@ parse_ep_config(const char *q_arg)
>   #define CMD_LINE_OPT_TELEMETRY "telemetry"
>   #define CMD_LINE_OPT_PMD_MGMT "pmd-mgmt"
>   #define CMD_LINE_OPT_MAX_PKT_LEN "max-pkt-len"
> +#define CMD_LINE_OPT_MAX_EMPTY_POLLS "max-empty-poll"
> +#define CMD_LINE_OPT_PAUSE_DURATION "pause-duration"
> +#define CMD_LINE_OPT_SCALE_FREQ_MIN "scale-freq-min"
> +#define CMD_LINE_OPT_SCALE_FREQ_MAX "scale-freq-max"
>   
>   /* Parse the argument given in the command line of the application */
>   static int
> @@ -1812,6 +1842,7 @@ parse_args(int argc, char **argv)
>   	char **argvopt;
>   	int option_index;
>   	uint32_t limit;
> +	int i;
>   	char *prgname = argv[0];
>   	static struct option lgopts[] = {
>   		{"config", 1, 0, 0},
> @@ -1825,6 +1856,10 @@ parse_args(int argc, char **argv)
>   		{CMD_LINE_OPT_TELEMETRY, 0, 0, 0},
>   		{CMD_LINE_OPT_INTERRUPT_ONLY, 0, 0, 0},
>   		{CMD_LINE_OPT_PMD_MGMT, 1, 0, 0},
> +		{CMD_LINE_OPT_MAX_EMPTY_POLLS, 1, 0, 0},
> +		{CMD_LINE_OPT_PAUSE_DURATION, 1, 0, 0},
> +		{CMD_LINE_OPT_SCALE_FREQ_MIN, 1, 0, 0},
> +		{CMD_LINE_OPT_SCALE_FREQ_MAX, 1, 0, 0},
>   		{NULL, 0, 0, 0}
>   	};
>   
> @@ -1975,6 +2010,44 @@ parse_args(int argc, char **argv)
>   				parse_ptype = 1;
>   			}
>   
> +			if (!strncmp(lgopts[option_index].name,
> +					CMD_LINE_OPT_MAX_EMPTY_POLLS,
> +					sizeof(CMD_LINE_OPT_MAX_EMPTY_POLLS))) {
> +				printf("Maximum empty polls configured\n");
> +				max_empty_polls = parse_int(optarg);
> +				rte_power_pmd_mgmt_set_emptypoll_max(max_empty_polls);

If you have global variables anyway, why are you setting this here? :) 
You could call these API's under PMD mgmt mode init only, and this would 
also give you a chance of checking return values of any of these API's - 
for example, AFAIR set_pause_duration() can return an error if the value 
supplied is invalid (0).

Plus, I still feel uneasy about having DPDK API calls right inside arg 
parsing code.

> +			}
> +
> +			if (!strncmp(lgopts[option_index].name,
> +					CMD_LINE_OPT_PAUSE_DURATION,
> +					sizeof(CMD_LINE_OPT_PAUSE_DURATION))) {
> +				printf("Pause duration configured\n");
> +				pause_duration = parse_int(optarg);
> +				rte_power_pmd_mgmt_set_pause_duration(pause_duration);
> +			}
> +
> +			if (!strncmp(lgopts[option_index].name,
> +					CMD_LINE_OPT_SCALE_FREQ_MIN,
> +					sizeof(CMD_LINE_OPT_SCALE_FREQ_MIN))) {
> +				printf("Scaling frequency minimum configured\n");
> +				scale_freq_min = parse_int(optarg);
> +				for (i = 0; i < RTE_MAX_LCORE; i++)
> +					if (rte_lcore_is_enabled(i))
> +						rte_power_pmd_mgmt_set_scaling_freq_min(i,
> +								scale_freq_min);
> +			}
> +
> +			if (!strncmp(lgopts[option_index].name,
> +					CMD_LINE_OPT_SCALE_FREQ_MAX,
> +					sizeof(CMD_LINE_OPT_SCALE_FREQ_MAX))) {
> +				printf("Scaling frequency maximum configured\n");
> +				scale_freq_max = parse_int(optarg);
> +				for (i = 0; i < RTE_MAX_LCORE; i++)
> +					if (rte_lcore_is_enabled(i))
> +						rte_power_pmd_mgmt_set_scaling_freq_max(i,
> +								scale_freq_max);
> +			}
> +
>   			break;
>   
>   		default:


-- 
Thanks,
Anatoly