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

[Kevin Laatz <kevin.laatz@intel.com>]

On 18/05/2022 10:11, Burakov, Anatoly wrote:
> 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?

Fair point, I'll add a note.


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

Ack


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

I'll look into this and move the API calls out of the parsing for v3.

Thanks for reviewing!


>
>> +            }
>> +
>> +            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:
>
>