| Message ID | 20190911095424.49605-1-huntbag@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Shuah Khan |
| Headers | show |
| Series | cpupower : Handle set and info subcommands for powerpc | expand |
On 9/11/19 3:54 AM, Abhishek Goel wrote: > Cpupower tool has set and info options which are not being used by > POWER machines. For powerpc, we will return directly for these two > subcommands. This removes the ambiguous error message while using set > option in case of power systems. What is the error message you see? Please include it in the commit log. > > Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com> > --- > tools/power/cpupower/utils/cpupower-info.c | 5 +++++ > tools/power/cpupower/utils/cpupower-set.c | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/tools/power/cpupower/utils/cpupower-info.c b/tools/power/cpupower/utils/cpupower-info.c > index 4c9d342b70ff..674b707a76af 100644 > --- a/tools/power/cpupower/utils/cpupower-info.c > +++ b/tools/power/cpupower/utils/cpupower-info.c > @@ -39,6 +39,11 @@ int cmd_info(int argc, char **argv) > } params = {}; > int ret = 0; > > + #ifdef __powerpc__ > + printf(_("Cannot read info as system does not support performance bias setting\n")); > + return 0; > + #endif > + I am not in favor of bailing out this early with this ifdef switch. I would rather see this checked somehow(?) when the ambiguous error happens. > setlocale(LC_ALL, ""); > textdomain(PACKAGE); > > diff --git a/tools/power/cpupower/utils/cpupower-set.c b/tools/power/cpupower/utils/cpupower-set.c > index 3cd95c6cb974..c95b29278780 100644 > --- a/tools/power/cpupower/utils/cpupower-set.c > +++ b/tools/power/cpupower/utils/cpupower-set.c > @@ -41,6 +41,11 @@ int cmd_set(int argc, char **argv) > int perf_bias = 0; > int ret = 0; > > + #ifdef __powerpc__ > + printf(_("System does not support performance bias setting\n")); > + return 0; > + #endif > + Same here. > setlocale(LC_ALL, ""); > textdomain(PACKAGE); > > thanks, -- Shuah
Hi Shuah, Thanks for the review. Few comments below. On 09/11/2019 03:41 PM, shuah wrote: > On 9/11/19 3:54 AM, Abhishek Goel wrote: >> Cpupower tool has set and info options which are not being used by >> POWER machines. For powerpc, we will return directly for these two >> subcommands. This removes the ambiguous error message while using set >> option in case of power systems. > > What is the error message you see? Please include it in the commit log. > Sure. Will include it in next version. >> >> Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com> >> --- >> tools/power/cpupower/utils/cpupower-info.c | 5 +++++ >> tools/power/cpupower/utils/cpupower-set.c | 5 +++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/tools/power/cpupower/utils/cpupower-info.c >> b/tools/power/cpupower/utils/cpupower-info.c >> index 4c9d342b70ff..674b707a76af 100644 >> --- a/tools/power/cpupower/utils/cpupower-info.c >> +++ b/tools/power/cpupower/utils/cpupower-info.c >> @@ -39,6 +39,11 @@ int cmd_info(int argc, char **argv) >> } params = {}; >> int ret = 0; >> + #ifdef __powerpc__ >> + printf(_("Cannot read info as system does not support >> performance bias setting\n")); >> + return 0; >> + #endif >> + > We see something like this for "cpupower info" ---> "System does not support Intel's performance bias setting" > I am not in favor of bailing out this early with this ifdef switch. > I would rather see this checked somehow(?) when the ambiguous error > happens. Since these two options are not being used by any other architecture except x86, I suggest these options should not even be shown for other architecture. So we can do something like this in cpupower.c : static struct cmd_struct commands[] = { ............. +#if defined (__x86_64__) || defined (__i386__) { "set", cmd_set, 1 }, { "info", cmd_info, 0 }, +#endif .............. Is this Okay? > >> setlocale(LC_ALL, ""); >> textdomain(PACKAGE); >> diff --git a/tools/power/cpupower/utils/cpupower-set.c >> b/tools/power/cpupower/utils/cpupower-set.c >> index 3cd95c6cb974..c95b29278780 100644 >> --- a/tools/power/cpupower/utils/cpupower-set.c >> +++ b/tools/power/cpupower/utils/cpupower-set.c >> @@ -41,6 +41,11 @@ int cmd_set(int argc, char **argv) >> int perf_bias = 0; >> int ret = 0; >> + #ifdef __powerpc__ >> + printf(_("System does not support performance bias setting\n")); >> + return 0; >> + #endif >> + > > Same here. > For "cpupower set -b 10", we get something like : "Error setting perf-bias value on CPU" >> setlocale(LC_ALL, ""); >> textdomain(PACKAGE); >> > > thanks, > -- Shuah Thanks, -- Abhishek
Hi Abishek, On Wednesday, September 11, 2019 11:54:24 AM CEST Abhishek Goel wrote: > Cpupower tool has set and info options which are not being used by > POWER machines. For powerpc, we will return directly for these two > subcommands. This removes the ambiguous error message while using set > option in case of power systems. > > Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com> > --- > tools/power/cpupower/utils/cpupower-info.c | 5 +++++ > tools/power/cpupower/utils/cpupower-set.c | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/tools/power/cpupower/utils/cpupower-info.c > b/tools/power/cpupower/utils/cpupower-info.c index > 4c9d342b70ff..674b707a76af 100644 > --- a/tools/power/cpupower/utils/cpupower-info.c > +++ b/tools/power/cpupower/utils/cpupower-info.c > @@ -39,6 +39,11 @@ int cmd_info(int argc, char **argv) > } params = {}; > int ret = 0; > > + #ifdef __powerpc__ > + printf(_("Cannot read info as system does not support performance bias > setting\n")); + return 0; > + #endif > + Please do no do this. cpupower info is designed to show general information related to powersaving features of your CPU. For examle there has been (see changelog): cpupower: Remove mc and smt power aware scheduler info/settings These kernel interfaces got removed by: Unfortunately only -b (perf bias on Intel only) is left right now. So if you cut this out for Power you do not see anything and the cmd is useless. Which is a pity, but for now makes sense. Ideally you provide some tag/option which makes sense on power (e.g. whether run in OPAL mode and if provide some figures otherwise tell running in VM mode). But if this is cut out something like this should do the same and is more flexible: - Still allows additional cpupower info features for other CPUs later easily - Should also cover AMD or other non-perf bias supporting CPUs to exclude perf_bias setting/info If this one works for you, can you please re-submit with also handling the set cmd similar. If it works or you only slightly adjust, feel free to already add: Acked-by: Thomas Renninger <trenn@suse.de> Thanks! Thomas --- tools/power/cpupower/utils/cpupower-info.c.orig 2019-09-12 11:45:02.578568335 +0200 +++ tools/power/cpupower/utils/cpupower-info.c 2019-09-12 11:46:09.618571947 +0200 @@ -55,8 +55,11 @@ } }; - if (!params.params) + if (!params.params) { params.params = 0x7; + if !(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS) + params.perf_bias = 0; + } /* Default is: show output of CPU 0 only */ if (bitmask_isallclear(cpus_chosen))
On Thursday, September 12, 2019 11:43:40 AM CEST Abhishek wrote: > Hi Shuah, > > Thanks for the review. Few comments below. ... > Since these two options are not being used by any other architecture > except x86, I suggest these options should not even be shown for > other architecture. So we can do something like this in cpupower.c : > > static struct cmd_struct commands[] = { > ............. > +#if defined (__x86_64__) || defined (__i386__) > { "set", cmd_set, 1 }, > { "info", cmd_info, 0 }, > +#endif > .............. > > Is this Okay? No, I expected you to add something meaningful for Power case... Just kidding. If this works without any side-effects in not x86 case, this approach seem to be the best solution for now. Thanks. Thomas
Hi Thomas, Thanks for the review. On 09/12/2019 03:24 PM, Thomas Renninger wrote: > Hi Abishek, > > On Wednesday, September 11, 2019 11:54:24 AM CEST Abhishek Goel wrote: >> Cpupower tool has set and info options which are not being used by >> POWER machines. For powerpc, we will return directly for these two >> subcommands. This removes the ambiguous error message while using set >> option in case of power systems. >> >> Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com> >> --- >> tools/power/cpupower/utils/cpupower-info.c | 5 +++++ >> tools/power/cpupower/utils/cpupower-set.c | 5 +++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/tools/power/cpupower/utils/cpupower-info.c >> b/tools/power/cpupower/utils/cpupower-info.c index >> 4c9d342b70ff..674b707a76af 100644 >> --- a/tools/power/cpupower/utils/cpupower-info.c >> +++ b/tools/power/cpupower/utils/cpupower-info.c >> @@ -39,6 +39,11 @@ int cmd_info(int argc, char **argv) >> } params = {}; >> int ret = 0; >> >> + #ifdef __powerpc__ >> + printf(_("Cannot read info as system does not support performance bias >> setting\n")); + return 0; >> + #endif >> + > Please do no do this. > > cpupower info > is designed to show general information related to powersaving features of your CPU. > > For examle there has been (see changelog): > cpupower: Remove mc and smt power aware scheduler info/settings > These kernel interfaces got removed by: > > Unfortunately only -b (perf bias on Intel only) is left right now. > > So if you cut this out for Power you do not see anything and the cmd is useless. > Which is a pity, but for now makes sense. > Ideally you provide some tag/option which makes sense on power (e.g. whether run > in OPAL mode and if provide some figures otherwise tell running in VM mode). > But if this is cut out something like this should do the same and is more flexible: > - Still allows additional cpupower info features for other CPUs later easily > - Should also cover AMD or other non-perf bias supporting CPUs to exclude perf_bias > setting/info As I have suggested in here : https://lkml.org/lkml/2019/9/12/159 We should cut out these two options as of now for other architecture except x86, since the current implementation of both these subcommands are very intel specific. As you have already suggested, we can later on maybe change the documentation of info to be architecture specific and use info based on arch requirement. > > If this one works for you, can you please re-submit with also handling the set cmd > similar. If it works or you only slightly adjust, feel free to already add: > Acked-by: Thomas Renninger <trenn@suse.de> Yeah. Sure. > Thanks! > > Thomas > > --- tools/power/cpupower/utils/cpupower-info.c.orig 2019-09-12 11:45:02.578568335 +0200 > +++ tools/power/cpupower/utils/cpupower-info.c 2019-09-12 11:46:09.618571947 +0200 > @@ -55,8 +55,11 @@ > } > }; > > - if (!params.params) > + if (!params.params) { > params.params = 0x7; > + if !(cpupower_cpu_info.caps & CPUPOWER_CAP_PERF_BIAS) > + params.perf_bias = 0; > + } > > /* Default is: show output of CPU 0 only */ > if (bitmask_isallclear(cpus_chosen)) > > > -- Abhishek
Hi, On 09/12/2019 03:46 PM, Thomas Renninger wrote: > On Thursday, September 12, 2019 11:43:40 AM CEST Abhishek wrote: >> Hi Shuah, >> >> Thanks for the review. Few comments below. > ... > >> Since these two options are not being used by any other architecture >> except x86, I suggest these options should not even be shown for >> other architecture. So we can do something like this in cpupower.c : >> >> static struct cmd_struct commands[] = { >> ............. >> +#if defined (__x86_64__) || defined (__i386__) >> { "set", cmd_set, 1 }, >> { "info", cmd_info, 0 }, >> +#endif >> .............. >> >> Is this Okay? > No, I expected you to add something meaningful for Power case... Haha. Will add something meaningful later... (One of the suggestion has already been given by you) > > Just kidding. If this works without any side-effects in not x86 case, this > approach seem to be the best solution for now. > > Thanks. > > Thomas > > -- Abhishek
diff --git a/tools/power/cpupower/utils/cpupower-info.c b/tools/power/cpupower/utils/cpupower-info.c index 4c9d342b70ff..674b707a76af 100644 --- a/tools/power/cpupower/utils/cpupower-info.c +++ b/tools/power/cpupower/utils/cpupower-info.c @@ -39,6 +39,11 @@ int cmd_info(int argc, char **argv) } params = {}; int ret = 0; + #ifdef __powerpc__ + printf(_("Cannot read info as system does not support performance bias setting\n")); + return 0; + #endif + setlocale(LC_ALL, ""); textdomain(PACKAGE); diff --git a/tools/power/cpupower/utils/cpupower-set.c b/tools/power/cpupower/utils/cpupower-set.c index 3cd95c6cb974..c95b29278780 100644 --- a/tools/power/cpupower/utils/cpupower-set.c +++ b/tools/power/cpupower/utils/cpupower-set.c @@ -41,6 +41,11 @@ int cmd_set(int argc, char **argv) int perf_bias = 0; int ret = 0; + #ifdef __powerpc__ + printf(_("System does not support performance bias setting\n")); + return 0; + #endif + setlocale(LC_ALL, ""); textdomain(PACKAGE);
Cpupower tool has set and info options which are not being used by POWER machines. For powerpc, we will return directly for these two subcommands. This removes the ambiguous error message while using set option in case of power systems. Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com> --- tools/power/cpupower/utils/cpupower-info.c | 5 +++++ tools/power/cpupower/utils/cpupower-set.c | 5 +++++ 2 files changed, 10 insertions(+)