Re: [PATCH 1/7] intel-speed-select: Add int argument to command functions

From: Srinivas Pandruvada
Date: Mon Sep 30 2019 - 16:43:06 EST

Next message: Andi Kleen: "Re: [PATCH v1 0/2] perf stat: Support --all-kernel and --all-user"
Previous message: Navid Emamdoost: "[PATCH v2] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr"
In reply to: Prarit Bhargava: "Re: [PATCH 1/7] intel-speed-select: Add int argument to command functions"
Next in thread: Prarit Bhargava: "[PATCH 3/7] intel-speed-select: Add check for CascadeLake-N models"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 2019-09-30 at 11:18 -0400, Prarit Bhargava wrote:
>
> On 9/26/19 4:21 PM, Srinivas Pandruvada wrote:
> > On Thu, 2019-09-26 at 08:54 -0400, Prarit Bhargava wrote:
> > > The current code structure has similar but separate command
> > > functions
> > > for
> > > the enable and disable operations. This can be improved by
> > > adding an
> > > int
> > > argument to the command function structure, and interpreting 1 as
> > > enable
> > > and 0 as disable. This change results in the removal of the
> > > disable
> > > command functions.
> > >
> > > Add int argument to the command function structure.
> >
> > Does this brings in any significant reduction in data or code size?
>
> It reduces code size. Right now you have separate functions for
> enable &
> disable. These are unified into single functions.
It reduce by 300 bytes.
My logic is the command processor functions are responsible for doing
what is required. After 5 years someone adding a command, don't need to
understand the meaning of additional argument.

IMO let's first focus on adding CLX-N support, this is I guess the aim
of this series. Then we will work on some cleanup.

I can't apply these patches for test. If you can use

http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/review-andy

as the baseline.

Thanks,
Srinivas

>
> P.
>
> > My focus is to add features first which helps users.
> >
> > Thanks,
> > Srinivas
> >
> >
> > >
> > > Signed-off-by: Prarit Bhargava <prarit@xxxxxxxxxx>
> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > > ---
> > > .../x86/intel-speed-select/isst-config.c | 184 +++++++-----
> > > ----
> > > --
> > > 1 file changed, 69 insertions(+), 115 deletions(-)
> > >
> > > diff --git a/tools/power/x86/intel-speed-select/isst-config.c
> > > b/tools/power/x86/intel-speed-select/isst-config.c
> > > index 2a9890c8395a..9f2798caead9 100644
> > > --- a/tools/power/x86/intel-speed-select/isst-config.c
> > > +++ b/tools/power/x86/intel-speed-select/isst-config.c
> > > @@ -11,7 +11,8 @@
> > > struct process_cmd_struct {
> > > char *feature;
> > > char *command;
> > > - void (*process_fn)(void);
> > > + void (*process_fn)(int arg);
> > > + int arg;
> > > };
> > >
> > > static const char *version_str = "v1.0";
> > > @@ -678,7 +679,7 @@ static void exec_on_get_ctdp_cpu(int cpu,
> > > void
> > > *arg1, void *arg2, void *arg3,
> > > }
> > >
> > > #define _get_tdp_level(desc, suffix, object,
> > > help) \
> > > - static void
> > > get_tdp_##object(void) \
> > > + static void get_tdp_##object(int
> > > arg) \
> > > {
> > > \
> > > struct isst_pkg_ctdp
> > > ctdp; \
> > > \
> > > @@ -724,7 +725,7 @@ static void dump_isst_config_for_cpu(int cpu,
> > > void *arg1, void *arg2,
> > > }
> > > }
> > >
> > > -static void dump_isst_config(void)
> > > +static void dump_isst_config(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr,
> > > @@ -787,7 +788,7 @@ static void set_tdp_level_for_cpu(int cpu,
> > > void
> > > *arg1, void *arg2, void *arg3,
> > > }
> > > }
> > >
> > > -static void set_tdp_level(void)
> > > +static void set_tdp_level(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr, "Set Config TDP level\n");
> > > @@ -827,7 +828,7 @@ static void dump_pbf_config_for_cpu(int cpu,
> > > void
> > > *arg1, void *arg2, void *arg3,
> > > }
> > > }
> > >
> > > -static void dump_pbf_config(void)
> > > +static void dump_pbf_config(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr,
> > > @@ -871,43 +872,27 @@ static void set_pbf_for_cpu(int cpu, void
> > > *arg1, void *arg2, void *arg3,
> > > }
> > > }
> > >
> > > -static void set_pbf_enable(void)
> > > -{
> > > - int status = 1;
> > > -
> > > - if (cmd_help) {
> > > - fprintf(stderr,
> > > - "Enable Intel Speed Select Technology base
> > > frequency feature [No command arguments are required]\n");
> > > - exit(0);
> > > - }
> > > -
> > > - isst_ctdp_display_information_start(outf);
> > > - if (max_target_cpus)
> > > - for_each_online_target_cpu_in_set(set_pbf_for_cpu,
> > > NULL, NULL,
> > > - NULL, &status);
> > > - else
> > > - for_each_online_package_in_set(set_pbf_for_cpu, NULL,
> > > NULL,
> > > - NULL, &status);
> > > - isst_ctdp_display_information_end(outf);
> > > -}
> > > -
> > > -static void set_pbf_disable(void)
> > > +static void set_pbf_enable(int arg)
> > > {
> > > - int status = 0;
> > > + int enable = arg;
> > >
> > > if (cmd_help) {
> > > - fprintf(stderr,
> > > - "Disable Intel Speed Select Technology base
> > > frequency feature [No command arguments are required]\n");
> > > + if (enable)
> > > + fprintf(stderr,
> > > + "Enable Intel Speed Select Technology
> > > base frequency feature [No command arguments are required]\n");
> > > + else
> > > + fprintf(stderr,
> > > + "Disable Intel Speed Select Technology
> > > base frequency feature [No command arguments are required]\n");
> > > exit(0);
> > > }
> > >
> > > isst_ctdp_display_information_start(outf);
> > > if (max_target_cpus)
> > > for_each_online_target_cpu_in_set(set_pbf_for_cpu,
> > > NULL, NULL,
> > > - NULL, &status);
> > > + NULL, &enable);
> > > else
> > > for_each_online_package_in_set(set_pbf_for_cpu, NULL,
> > > NULL,
> > > - NULL, &status);
> > > + NULL, &enable);
> > > isst_ctdp_display_information_end(outf);
> > > }
> > >
> > > @@ -925,7 +910,7 @@ static void dump_fact_config_for_cpu(int cpu,
> > > void *arg1, void *arg2,
> > > fact_avx, &fact_info);
> > > }
> > >
> > > -static void dump_fact_config(void)
> > > +static void dump_fact_config(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr,
> > > @@ -985,35 +970,17 @@ static void set_fact_for_cpu(int cpu, void
> > > *arg1, void *arg2, void *arg3,
> > > }
> > > }
> > >
> > > -static void set_fact_enable(void)
> > > +static void set_fact_enable(int arg)
> > > {
> > > - int status = 1;
> > > + int enable = arg;
> > >
> > > if (cmd_help) {
> > > - fprintf(stderr,
> > > - "Enable Intel Speed Select Technology Turbo
> > > frequency feature\n");
> > > - fprintf(stderr,
> > > - "Optional: -t|--trl : Specify turbo ratio
> > > limit\n");
> > > - exit(0);
> > > - }
> > > -
> > > - isst_ctdp_display_information_start(outf);
> > > - if (max_target_cpus)
> > > - for_each_online_target_cpu_in_set(set_fact_for_cpu,
> > > NULL, NULL,
> > > - NULL, &status);
> > > - else
> > > - for_each_online_package_in_set(set_fact_for_cpu, NULL,
> > > NULL,
> > > - NULL, &status);
> > > - isst_ctdp_display_information_end(outf);
> > > -}
> > > -
> > > -static void set_fact_disable(void)
> > > -{
> > > - int status = 0;
> > > -
> > > - if (cmd_help) {
> > > - fprintf(stderr,
> > > - "Disable Intel Speed Select Technology turbo
> > > frequency feature\n");
> > > + if (enable)
> > > + fprintf(stderr,
> > > + "Enable Intel Speed Select Technology
> > > Turbo frequency feature\n");
> > > + else
> > > + fprintf(stderr,
> > > + "Disable Intel Speed Select Technology
> > > turbo frequency feature\n");
> > > fprintf(stderr,
> > > "Optional: -t|--trl : Specify turbo ratio
> > > limit\n");
> > > exit(0);
> > > @@ -1022,10 +989,10 @@ static void set_fact_disable(void)
> > > isst_ctdp_display_information_start(outf);
> > > if (max_target_cpus)
> > > for_each_online_target_cpu_in_set(set_fact_for_cpu,
> > > NULL, NULL,
> > > - NULL, &status);
> > > + NULL, &enable);
> > > else
> > > for_each_online_package_in_set(set_fact_for_cpu, NULL,
> > > NULL,
> > > - NULL, &status);
> > > + NULL, &enable);
> > > isst_ctdp_display_information_end(outf);
> > > }
> > >
> > > @@ -1048,19 +1015,25 @@ static void enable_clos_qos_config(int
> > > cpu,
> > > void *arg1, void *arg2, void *arg3,
> > > }
> > > }
> > >
> > > -static void set_clos_enable(void)
> > > +static void set_clos_enable(int arg)
> > > {
> > > - int status = 1;
> > > + int enable = arg;
> > >
> > > if (cmd_help) {
> > > - fprintf(stderr, "Enable core-power for a
> > > package/die\n");
> > > - fprintf(stderr,
> > > - "\tClos Enable: Specify priority type with [
> > > --priority|-p]\n");
> > > - fprintf(stderr, "\t\t 0: Proportional, 1: Ordered\n");
> > > + if (enable) {
> > > + fprintf(stderr,
> > > + "Enable core-power for a
> > > package/die\n");
> > > + fprintf(stderr,
> > > + "\tClos Enable: Specify priority type
> > > with [--priority|-p]\n");
> > > + fprintf(stderr, "\t\t 0: Proportional, 1:
> > > Ordered\n");
> > > + } else {
> > > + fprintf(stderr,
> > > + "Disable core-power: [No command
> > > arguments are required]\n");
> > > + }
> > > exit(0);
> > > }
> > >
> > > - if (cpufreq_sysfs_present()) {
> > > + if (enable && cpufreq_sysfs_present()) {
> > > fprintf(stderr,
> > > "cpufreq subsystem and core-power enable will
> > > interfere with each other!\n");
> > > }
> > > @@ -1068,30 +1041,10 @@ static void set_clos_enable(void)
> > > isst_ctdp_display_information_start(outf);
> > > if (max_target_cpus)
> > > for_each_online_target_cpu_in_set(enable_clos_qos_confi
> > > g, NULL,
> > > - NULL, NULL, &status);
> > > - else
> > > - for_each_online_package_in_set(enable_clos_qos_config,
> > > NULL,
> > > - NULL, NULL, &status);
> > > - isst_ctdp_display_information_end(outf);
> > > -}
> > > -
> > > -static void set_clos_disable(void)
> > > -{
> > > - int status = 0;
> > > -
> > > - if (cmd_help) {
> > > - fprintf(stderr,
> > > - "Disable core-power: [No command arguments are
> > > required]\n");
> > > - exit(0);
> > > - }
> > > -
> > > - isst_ctdp_display_information_start(outf);
> > > - if (max_target_cpus)
> > > - for_each_online_target_cpu_in_set(enable_clos_qos_confi
> > > g, NULL,
> > > - NULL, NULL, &status);
> > > + NULL, NULL, &enable);
> > > else
> > > for_each_online_package_in_set(enable_clos_qos_config,
> > > NULL,
> > > - NULL, NULL, &status);
> > > + NULL, NULL, &enable);
> > > isst_ctdp_display_information_end(outf);
> > > }
> > >
> > > @@ -1109,7 +1062,7 @@ static void dump_clos_config_for_cpu(int
> > > cpu,
> > > void *arg1, void *arg2,
> > > &clos_config);
> > > }
> > >
> > > -static void dump_clos_config(void)
> > > +static void dump_clos_config(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr,
> > > @@ -1145,7 +1098,7 @@ static void get_clos_info_for_cpu(int cpu,
> > > void
> > > *arg1, void *arg2, void *arg3,
> > > isst_clos_display_clos_information(cpu, outf, enable,
> > > prio_type);
> > > }
> > >
> > > -static void dump_clos_info(void)
> > > +static void dump_clos_info(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr,
> > > @@ -1188,7 +1141,7 @@ static void set_clos_config_for_cpu(int
> > > cpu,
> > > void *arg1, void *arg2, void *arg3,
> > > isst_display_result(cpu, outf, "core-power", "config",
> > > ret);
> > > }
> > >
> > > -static void set_clos_config(void)
> > > +static void set_clos_config(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr,
> > > @@ -1252,7 +1205,7 @@ static void set_clos_assoc_for_cpu(int cpu,
> > > void *arg1, void *arg2, void *arg3,
> > > isst_display_result(cpu, outf, "core-power", "assoc",
> > > ret);
> > > }
> > >
> > > -static void set_clos_assoc(void)
> > > +static void set_clos_assoc(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr, "Associate a clos id to a CPU\n");
> > > @@ -1286,7 +1239,7 @@ static void get_clos_assoc_for_cpu(int cpu,
> > > void *arg1, void *arg2, void *arg3,
> > > isst_clos_display_assoc_information(cpu, outf, clos);
> > > }
> > >
> > > -static void get_clos_assoc(void)
> > > +static void get_clos_assoc(int arg)
> > > {
> > > if (cmd_help) {
> > > fprintf(stderr, "Get associate clos id to a CPU\n");
> > > @@ -1307,26 +1260,27 @@ static void get_clos_assoc(void)
> > > }
> > >
> > > static struct process_cmd_struct isst_cmds[] = {
> > > - { "perf-profile", "get-lock-status", get_tdp_locked },
> > > - { "perf-profile", "get-config-levels", get_tdp_levels },
> > > - { "perf-profile", "get-config-version", get_tdp_version },
> > > - { "perf-profile", "get-config-enabled", get_tdp_enabled },
> > > - { "perf-profile", "get-config-current-level",
> > > get_tdp_current_level },
> > > - { "perf-profile", "set-config-level", set_tdp_level },
> > > - { "perf-profile", "info", dump_isst_config },
> > > - { "base-freq", "info", dump_pbf_config },
> > > - { "base-freq", "enable", set_pbf_enable },
> > > - { "base-freq", "disable", set_pbf_disable },
> > > - { "turbo-freq", "info", dump_fact_config },
> > > - { "turbo-freq", "enable", set_fact_enable },
> > > - { "turbo-freq", "disable", set_fact_disable },
> > > - { "core-power", "info", dump_clos_info },
> > > - { "core-power", "enable", set_clos_enable },
> > > - { "core-power", "disable", set_clos_disable },
> > > - { "core-power", "config", set_clos_config },
> > > - { "core-power", "get-config", dump_clos_config },
> > > - { "core-power", "assoc", set_clos_assoc },
> > > - { "core-power", "get-assoc", get_clos_assoc },
> > > + { "perf-profile", "get-lock-status", get_tdp_locked, 0 },
> > > + { "perf-profile", "get-config-levels", get_tdp_levels, 0 },
> > > + { "perf-profile", "get-config-version", get_tdp_version, 0 },
> > > + { "perf-profile", "get-config-enabled", get_tdp_enabled, 0 },
> > > + { "perf-profile", "get-config-current-level",
> > > get_tdp_current_level,
> > > + 0 },
> > > + { "perf-profile", "set-config-level", set_tdp_level, 0 },
> > > + { "perf-profile", "info", dump_isst_config, 0 },
> > > + { "base-freq", "info", dump_pbf_config, 0 },
> > > + { "base-freq", "enable", set_pbf_enable, 1 },
> > > + { "base-freq", "disable", set_pbf_enable, 0 },
> > > + { "turbo-freq", "info", dump_fact_config, 0 },
> > > + { "turbo-freq", "enable", set_fact_enable, 1 },
> > > + { "turbo-freq", "disable", set_fact_enable, 0 },
> > > + { "core-power", "info", dump_clos_info, 0 },
> > > + { "core-power", "enable", set_clos_enable, 1 },
> > > + { "core-power", "disable", set_clos_enable, 0 },
> > > + { "core-power", "config", set_clos_config, 0 },
> > > + { "core-power", "get-config", dump_clos_config, 0 },
> > > + { "core-power", "assoc", set_clos_assoc, 0 },
> > > + { "core-power", "get-assoc", get_clos_assoc, 0 },
> > > { NULL, NULL, NULL }
> > > };
> > >
> > > @@ -1571,7 +1525,7 @@ void process_command(int argc, char **argv)
> > > if (!strcmp(isst_cmds[i].feature, feature) &&
> > > !strcmp(isst_cmds[i].command, cmd)) {
> > > parse_cmd_args(argc, optind + 1, argv);
> > > - isst_cmds[i].process_fn();
> > > + isst_cmds[i].process_fn(isst_cmds[i].arg);
> > > matched = 1;
> > > break;
> > > }

Next message: Andi Kleen: "Re: [PATCH v1 0/2] perf stat: Support --all-kernel and --all-user"
Previous message: Navid Emamdoost: "[PATCH v2] virt: vbox: fix memory leak in hgcm_call_preprocess_linaddr"
In reply to: Prarit Bhargava: "Re: [PATCH 1/7] intel-speed-select: Add int argument to command functions"
Next in thread: Prarit Bhargava: "[PATCH 3/7] intel-speed-select: Add check for CascadeLake-N models"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]