[PATCH] perf stat: Add hint for SMI cost measurement

[PATCH] perf stat: Add hint for SMI cost measurement

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

Cstate may cause drift between aperf and cycles, which impact the
accuracy of SMI cost measurement.

Add smi_env_check() to check if cstate is completely disabled during the
measurement. If not, give a hint to user.

Update the document as well.

Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/Documentation/perf-stat.txt |  4 ++++
 tools/perf/builtin-stat.c              | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index 39c05f8..e0d8763 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -309,6 +309,10 @@ The output is SMI cycles%, equals to (aperf - unhalted core cycles) / aperf
 
 Users who wants to get the actual value can apply --no-metric-only.
 
+Cstate may cause drift between aperf and cycles.
+Please completely disable cstate during the measurement.
+E.g. set idle=poll in grub
+
 EXAMPLES
 --------
 
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c3625ec..ac273b4 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -87,6 +87,7 @@
 
 #define DEFAULT_SEPARATOR	" "
 #define FREEZE_ON_SMI_PATH	"devices/cpu/freeze_on_smi"
+#define CPUIDLE_CUR_DRV		"devices/system/cpu/cpuidle/current_driver"
 
 static void print_counters(struct timespec *ts, int argc, const char **argv);
 
@@ -1024,6 +1025,25 @@ __weak void arch_topdown_group_warn(void)
 {
 }
 
+static void smi_env_check(void)
+{
+	char *name;
+	size_t len;
+
+	if (sysfs__read_str(CPUIDLE_CUR_DRV, &name, &len)) {
+		pr_warning("Failed to check cstate status.\n");
+		return;
+	}
+
+	if (strncmp(name, "none", 4)) {
+		pr_warning("Cstate may cause drift between aperf and cycles. "
+			   "Please completely disable cstate, "
+			   "E.g. set idle=poll in grub\n");
+	}
+
+	free(name);
+}
+
 /*
  * Add default attributes, if there were no attributes specified or
  * if -d/--detailed, -d -d or -d -d -d is used:
@@ -1196,6 +1216,9 @@ static int add_default_attributes(void)
 
 		if (pmu_have_event("msr", "aperf") &&
 		    pmu_have_event("msr", "smi")) {
+
+			smi_env_check();
+
 			if (!force_metric_only)
 				stat_config.metric_only = true;
 			err = parse_events(evsel_list, smi_cost_attrs, &errinfo);
-- 
2.7.4

Re: [PATCH] perf stat: Add hint for SMI cost measurement

[Ingo Molnar]

* kan.liang@linux.intel.com <kan.liang@linux.intel.com> wrote:

> +static void smi_env_check(void)
> +{
> +	char *name;
> +	size_t len;
> +
> +	if (sysfs__read_str(CPUIDLE_CUR_DRV, &name, &len)) {
> +		pr_warning("Failed to check cstate status.\n");

What a meaningless message. What did we want to do, what happened, and 
why did it fail?

> +		return;
> +	}
> +
> +	if (strncmp(name, "none", 4)) {
> +		pr_warning("Cstate may cause drift between aperf and cycles. "
> +			   "Please completely disable cstate, "
> +			   "E.g. set idle=poll in grub\n");

Please keep user-visible strings in the same form that the user sees 
them, i.e. in a single line.

By doing that you'll also note a capitalization error.

Also what does 'Cstate may cause drift' mean? What aspect of cstates 
causes the drift - entering/exiting deeper cstates that are not C0? If so 
then say so.

Thanks,

	Ingo

Re: [PATCH] perf stat: Add hint for SMI cost measurement

[Liang, Kan]

On 4/25/2019 2:39 AM, Ingo Molnar wrote:
> 
> * kan.liang@linux.intel.com <kan.liang@linux.intel.com> wrote:
> 
>> +static void smi_env_check(void)
>> +{
>> +	char *name;
>> +	size_t len;
>> +
>> +	if (sysfs__read_str(CPUIDLE_CUR_DRV, &name, &len)) {
>> +		pr_warning("Failed to check cstate status.\n");
> 
> What a meaningless message. What did we want to do, what happened, and
> why did it fail?
> 
>> +		return;
>> +	}
>> +
>> +	if (strncmp(name, "none", 4)) {
>> +		pr_warning("Cstate may cause drift between aperf and cycles. "
>> +			   "Please completely disable cstate, "
>> +			   "E.g. set idle=poll in grub\n");
> 
> Please keep user-visible strings in the same form that the user sees
> them, i.e. in a single line.

To avoid the line over 80 characters, the quoted string was split across 
lines in the code. But the string is shown as a single line when it is 
output for user.

Are you suggesting to ignore the 80 characters rule when printing 
user-visible strings?
Could you please confirm?

Thanks,
Kan

> 
> By doing that you'll also note a capitalization error.
> 
> Also what does 'Cstate may cause drift' mean? What aspect of cstates
> causes the drift - entering/exiting deeper cstates that are not C0? If so
> then say so.
> 
> Thanks,
> 
> 	Ingo
> 

Re: [PATCH] perf stat: Add hint for SMI cost measurement

[Ingo Molnar]

* Liang, Kan <kan.liang@linux.intel.com> wrote:

> 
> 
> On 4/25/2019 2:39 AM, Ingo Molnar wrote:
> > 
> > * kan.liang@linux.intel.com <kan.liang@linux.intel.com> wrote:
> > 
> > > +static void smi_env_check(void)
> > > +{
> > > +	char *name;
> > > +	size_t len;
> > > +
> > > +	if (sysfs__read_str(CPUIDLE_CUR_DRV, &name, &len)) {
> > > +		pr_warning("Failed to check cstate status.\n");
> > 
> > What a meaningless message. What did we want to do, what happened, and
> > why did it fail?
> > 
> > > +		return;
> > > +	}
> > > +
> > > +	if (strncmp(name, "none", 4)) {
> > > +		pr_warning("Cstate may cause drift between aperf and cycles. "
> > > +			   "Please completely disable cstate, "
> > > +			   "E.g. set idle=poll in grub\n");
> > 
> > Please keep user-visible strings in the same form that the user sees
> > them, i.e. in a single line.
> 
> To avoid the line over 80 characters, the quoted string was split across
> lines in the code. But the string is shown as a single line when it is
> output for user.
> 
> Are you suggesting to ignore the 80 characters rule when printing
> user-visible strings?
> Could you please confirm?

What I'd suggest is split it up into multiple messages, or shorten the 
message. Small violations of col80 for user visible strings is fine (up 
to 100-120 columns I guess), but if the string is longer it's a clear 
sign that the syslog message it too long to begin ...

Thanks,

	Ingo

Re: [PATCH] perf stat: Add hint for SMI cost measurement

[Liang, Kan]

On 4/25/2019 1:47 PM, Ingo Molnar wrote:
> 
> * Liang, Kan <kan.liang@linux.intel.com> wrote:
> 
>>
>>
>> On 4/25/2019 2:39 AM, Ingo Molnar wrote:
>>>
>>> * kan.liang@linux.intel.com <kan.liang@linux.intel.com> wrote:
>>>
>>>> +static void smi_env_check(void)
>>>> +{
>>>> +	char *name;
>>>> +	size_t len;
>>>> +
>>>> +	if (sysfs__read_str(CPUIDLE_CUR_DRV, &name, &len)) {
>>>> +		pr_warning("Failed to check cstate status.\n");
>>>
>>> What a meaningless message. What did we want to do, what happened, and
>>> why did it fail?
>>>
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (strncmp(name, "none", 4)) {
>>>> +		pr_warning("Cstate may cause drift between aperf and cycles. "
>>>> +			   "Please completely disable cstate, "
>>>> +			   "E.g. set idle=poll in grub\n");
>>>
>>> Please keep user-visible strings in the same form that the user sees
>>> them, i.e. in a single line.
>>
>> To avoid the line over 80 characters, the quoted string was split across
>> lines in the code. But the string is shown as a single line when it is
>> output for user.
>>
>> Are you suggesting to ignore the 80 characters rule when printing
>> user-visible strings?
>> Could you please confirm?
> 
> What I'd suggest is split it up into multiple messages, or shorten the
> message. Small violations of col80 for user visible strings is fine (up
> to 100-120 columns I guess), but if the string is longer it's a clear
> sign that the syslog message it too long to begin ...
> 

Sure, I will update the messages and send out V2.

Thanks,
Kan