problem with 'perf script -F weight'

problem with 'perf script -F weight'

[Jiri Olsa]

hi,
Joe reported broken -F weight in perf script with (on x86):

  # ./perf mem record 
  # ./perf script -F weight
  Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot print 'weight' field.

the problem seems to be introduced with the WEIGHT_STRUCT change:
  ea8d0ed6eae3 perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT

which enables WEIGHT_STRUCT sample_type on x86 instead of WEIGHT,
and leaves 'perf script -F weight' in the dark

I'm not sure what the fix should look like.. should we allow user
to use just 'WEIGHT' sample type? or is there a way to get original
weight from 'WEIGHT_STRUCT' data, so we could fix just perf script?

FYI when I switch back the WEIGHT_STRUCT to WEIGHT again it seems
to work as before.. as expected ;-)

any thoughts? thanks
jirka


---
diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
index 2f733cdc8dbb..6bdd744894ad 100644
--- a/tools/perf/arch/x86/util/evsel.c
+++ b/tools/perf/arch/x86/util/evsel.c
@@ -4,5 +4,5 @@
 
 void arch_evsel__set_sample_weight(struct evsel *evsel)
 {
-	evsel__set_sample_bit(evsel, WEIGHT_STRUCT);
+	evsel__set_sample_bit(evsel, WEIGHT);
 }

Re: problem with 'perf script -F weight'

[Andi Kleen]

On 9/28/2021 12:20 PM, Jiri Olsa wrote:
> hi,
> Joe reported broken -F weight in perf script with (on x86):
>
>    # ./perf mem record
>    # ./perf script -F weight
>    Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot print 'weight' field.
>
> the problem seems to be introduced with the WEIGHT_STRUCT change:
>    ea8d0ed6eae3 perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT
>
> which enables WEIGHT_STRUCT sample_type on x86 instead of WEIGHT,
> and leaves 'perf script -F weight' in the dark
>
> I'm not sure what the fix should look like.. should we allow user
> to use just 'WEIGHT' sample type? or is there a way to get original
> weight from 'WEIGHT_STRUCT' data, so we could fix just perf script?

We would need to support both in perf script, to handle both perf.data 
generated by old and new kernels on both big and little endian

So probably try one first and then the other while parsing

The same is true for all other users, so probably there needs to be a 
library function for this somewhere.

-Andi

Re: problem with 'perf script -F weight'

[Liang, Kan]

On 9/28/2021 3:20 PM, Jiri Olsa wrote:
> Joe reported broken -F weight in perf script with (on x86):
> 
>    # ./perf mem record
>    # ./perf script -F weight
>    Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot print 'weight' field.
> 
> the problem seems to be introduced with the WEIGHT_STRUCT change:
>    ea8d0ed6eae3 perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT
> 
> which enables WEIGHT_STRUCT sample_type on x86 instead of WEIGHT,
> and leaves 'perf script -F weight' in the dark

Right, I missed the support for the perf script. Because the default 
perf script worked. It didn't ring the bell. :(

> 
> I'm not sure what the fix should look like.. should we allow user
> to use just 'WEIGHT' sample type? or is there a way to get original
> weight from 'WEIGHT_STRUCT' data, so we could fix just perf script?

Just fix the perf script is enough.
Could you please try the below patch?

(I probably need to split the patch into two patches, since it fixed two 
issues, one is this error, the other is to print the instruction 
latency. Anyway, let's try it first.)

 From 99443f49a6236ef6e3bea3930792967a7863f753 Mon Sep 17 00:00:00 2001
From: Kan Liang <kan.liang@linux.intel.com>
Date: Tue, 28 Sep 2021 13:57:18 -0700
Subject: [PATCH] perf script: Fix PERF_SAMPLE_WEIGHT_STRUCT support

-F weight in perf script is broken.

   # ./perf mem record
   # ./perf script -F weight
   Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot
print 'weight' field.

The sample type, PERF_SAMPLE_WEIGHT_STRUCT, is an alternative of the
PERF_SAMPLE_WEIGHT sample type. They share the same space, weight. The
lower 32 bits are exactly the same for both sample type. The higher 32
bits may be different for different architecture. For a new kernel on
x86, the PERF_SAMPLE_WEIGHT_STRUCT is used. For an old kernel or other
ARCHs, the PERF_SAMPLE_WEIGHT is used.

With -F weight, current perf script will check the input string "weight"
with the corresponding sample type. However, the commit ID ea8d0ed6eae3
("perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT") didn't update the
sample type for perf script. The check fails with a new kernel on x86.

Use PERF_SAMPLE_WEIGHT_TYPE, which supports both sample types, to
replace PERF_SAMPLE_WEIGHT.

Also, print the instruction latency for PERF_SAMPLE_WEIGHT_STRUCT type.

Reported-by: Joe Mario <jmario@redhat.com>
Fixes: ea8d0ed6eae3 ("perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT")
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
  tools/perf/builtin-script.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 6211d0b..fd73bbc 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -459,7 +459,7 @@ static int evsel__check_attr(struct evsel *evsel, 
struct perf_session *session)
  		return -EINVAL;

  	if (PRINT_FIELD(WEIGHT) &&
-	    evsel__check_stype(evsel, PERF_SAMPLE_WEIGHT, "WEIGHT", 
PERF_OUTPUT_WEIGHT))
+	    evsel__check_stype(evsel, PERF_SAMPLE_WEIGHT_TYPE, "WEIGHT", 
PERF_OUTPUT_WEIGHT))
  		return -EINVAL;

  	if (PRINT_FIELD(SYM) &&
@@ -2036,8 +2036,11 @@ static void process_event(struct perf_script *script,
  	if (PRINT_FIELD(DATA_SRC))
  		data_src__fprintf(sample->data_src, fp);

-	if (PRINT_FIELD(WEIGHT))
+	if (PRINT_FIELD(WEIGHT)) {
  		fprintf(fp, "%16" PRIu64, sample->weight);
+		if (attr->sample_type & PERF_SAMPLE_WEIGHT_STRUCT)
+			fprintf(fp, ",0x%" PRIx16, sample->ins_lat);
+	}

  	if (PRINT_FIELD(IP)) {
  		struct callchain_cursor *cursor = NULL;
-- 
2.7.4

Thanks,
Kan

Re: problem with 'perf script -F weight'

[Jiri Olsa]

On Tue, Sep 28, 2021 at 05:35:51PM -0400, Liang, Kan wrote:
> 
> 
> On 9/28/2021 3:20 PM, Jiri Olsa wrote:
> > Joe reported broken -F weight in perf script with (on x86):
> > 
> >    # ./perf mem record
> >    # ./perf script -F weight
> >    Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot print 'weight' field.
> > 
> > the problem seems to be introduced with the WEIGHT_STRUCT change:
> >    ea8d0ed6eae3 perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT
> > 
> > which enables WEIGHT_STRUCT sample_type on x86 instead of WEIGHT,
> > and leaves 'perf script -F weight' in the dark
> 
> Right, I missed the support for the perf script. Because the default perf
> script worked. It didn't ring the bell. :(
> 
> > 
> > I'm not sure what the fix should look like.. should we allow user
> > to use just 'WEIGHT' sample type? or is there a way to get original
> > weight from 'WEIGHT_STRUCT' data, so we could fix just perf script?
> 
> Just fix the perf script is enough.
> Could you please try the below patch?
> 
> (I probably need to split the patch into two patches, since it fixed two
> issues, one is this error, the other is to print the instruction latency.
> Anyway, let's try it first.)
> 
> From 99443f49a6236ef6e3bea3930792967a7863f753 Mon Sep 17 00:00:00 2001
> From: Kan Liang <kan.liang@linux.intel.com>
> Date: Tue, 28 Sep 2021 13:57:18 -0700
> Subject: [PATCH] perf script: Fix PERF_SAMPLE_WEIGHT_STRUCT support
> 
> -F weight in perf script is broken.
> 
>   # ./perf mem record
>   # ./perf script -F weight
>   Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot
> print 'weight' field.
> 
> The sample type, PERF_SAMPLE_WEIGHT_STRUCT, is an alternative of the
> PERF_SAMPLE_WEIGHT sample type. They share the same space, weight. The
> lower 32 bits are exactly the same for both sample type. The higher 32
> bits may be different for different architecture. For a new kernel on
> x86, the PERF_SAMPLE_WEIGHT_STRUCT is used. For an old kernel or other
> ARCHs, the PERF_SAMPLE_WEIGHT is used.
> 
> With -F weight, current perf script will check the input string "weight"
> with the corresponding sample type. However, the commit ID ea8d0ed6eae3
> ("perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT") didn't update the
> sample type for perf script. The check fails with a new kernel on x86.
> 
> Use PERF_SAMPLE_WEIGHT_TYPE, which supports both sample types, to
> replace PERF_SAMPLE_WEIGHT.
> 
> Also, print the instruction latency for PERF_SAMPLE_WEIGHT_STRUCT type.
> 
> Reported-by: Joe Mario <jmario@redhat.com>
> Fixes: ea8d0ed6eae3 ("perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT")
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  tools/perf/builtin-script.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 6211d0b..fd73bbc 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -459,7 +459,7 @@ static int evsel__check_attr(struct evsel *evsel, struct
> perf_session *session)
>  		return -EINVAL;
> 
>  	if (PRINT_FIELD(WEIGHT) &&
> -	    evsel__check_stype(evsel, PERF_SAMPLE_WEIGHT, "WEIGHT",
> PERF_OUTPUT_WEIGHT))
> +	    evsel__check_stype(evsel, PERF_SAMPLE_WEIGHT_TYPE, "WEIGHT",
> PERF_OUTPUT_WEIGHT))
>  		return -EINVAL;
> 
>  	if (PRINT_FIELD(SYM) &&
> @@ -2036,8 +2036,11 @@ static void process_event(struct perf_script *script,
>  	if (PRINT_FIELD(DATA_SRC))
>  		data_src__fprintf(sample->data_src, fp);
> 
> -	if (PRINT_FIELD(WEIGHT))
> +	if (PRINT_FIELD(WEIGHT)) {
>  		fprintf(fp, "%16" PRIu64, sample->weight);
> +		if (attr->sample_type & PERF_SAMPLE_WEIGHT_STRUCT)
> +			fprintf(fp, ",0x%" PRIx16, sample->ins_lat);

this will add extra number to weight.. should we add separate column
for that -F 'insn-lat' ? users expect just single value for -F weight

thanks,
jirka

> +	}
> 
>  	if (PRINT_FIELD(IP)) {
>  		struct callchain_cursor *cursor = NULL;
> -- 
> 2.7.4
> 
> Thanks,
> Kan
> 

Re: problem with 'perf script -F weight'

[Liang, Kan]

On 9/29/2021 2:08 AM, Jiri Olsa wrote:
> On Tue, Sep 28, 2021 at 05:35:51PM -0400, Liang, Kan wrote:
>>
>>
>> On 9/28/2021 3:20 PM, Jiri Olsa wrote:
>>> Joe reported broken -F weight in perf script with (on x86):
>>>
>>>     # ./perf mem record
>>>     # ./perf script -F weight
>>>     Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot print 'weight' field.
>>>
>>> the problem seems to be introduced with the WEIGHT_STRUCT change:
>>>     ea8d0ed6eae3 perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT
>>>
>>> which enables WEIGHT_STRUCT sample_type on x86 instead of WEIGHT,
>>> and leaves 'perf script -F weight' in the dark
>>
>> Right, I missed the support for the perf script. Because the default perf
>> script worked. It didn't ring the bell. :(
>>
>>>
>>> I'm not sure what the fix should look like.. should we allow user
>>> to use just 'WEIGHT' sample type? or is there a way to get original
>>> weight from 'WEIGHT_STRUCT' data, so we could fix just perf script?
>>
>> Just fix the perf script is enough.
>> Could you please try the below patch?
>>
>> (I probably need to split the patch into two patches, since it fixed two
>> issues, one is this error, the other is to print the instruction latency.
>> Anyway, let's try it first.)
>>
>>  From 99443f49a6236ef6e3bea3930792967a7863f753 Mon Sep 17 00:00:00 2001
>> From: Kan Liang <kan.liang@linux.intel.com>
>> Date: Tue, 28 Sep 2021 13:57:18 -0700
>> Subject: [PATCH] perf script: Fix PERF_SAMPLE_WEIGHT_STRUCT support
>>
>> -F weight in perf script is broken.
>>
>>    # ./perf mem record
>>    # ./perf script -F weight
>>    Samples for 'dummy:HG' event do not have WEIGHT attribute set. Cannot
>> print 'weight' field.
>>
>> The sample type, PERF_SAMPLE_WEIGHT_STRUCT, is an alternative of the
>> PERF_SAMPLE_WEIGHT sample type. They share the same space, weight. The
>> lower 32 bits are exactly the same for both sample type. The higher 32
>> bits may be different for different architecture. For a new kernel on
>> x86, the PERF_SAMPLE_WEIGHT_STRUCT is used. For an old kernel or other
>> ARCHs, the PERF_SAMPLE_WEIGHT is used.
>>
>> With -F weight, current perf script will check the input string "weight"
>> with the corresponding sample type. However, the commit ID ea8d0ed6eae3
>> ("perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT") didn't update the
>> sample type for perf script. The check fails with a new kernel on x86.
>>
>> Use PERF_SAMPLE_WEIGHT_TYPE, which supports both sample types, to
>> replace PERF_SAMPLE_WEIGHT.
>>
>> Also, print the instruction latency for PERF_SAMPLE_WEIGHT_STRUCT type.
>>
>> Reported-by: Joe Mario <jmario@redhat.com>
>> Fixes: ea8d0ed6eae3 ("perf tools: Support PERF_SAMPLE_WEIGHT_STRUCT")
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>   tools/perf/builtin-script.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>> index 6211d0b..fd73bbc 100644
>> --- a/tools/perf/builtin-script.c
>> +++ b/tools/perf/builtin-script.c
>> @@ -459,7 +459,7 @@ static int evsel__check_attr(struct evsel *evsel, struct
>> perf_session *session)
>>   		return -EINVAL;
>>
>>   	if (PRINT_FIELD(WEIGHT) &&
>> -	    evsel__check_stype(evsel, PERF_SAMPLE_WEIGHT, "WEIGHT",
>> PERF_OUTPUT_WEIGHT))
>> +	    evsel__check_stype(evsel, PERF_SAMPLE_WEIGHT_TYPE, "WEIGHT",
>> PERF_OUTPUT_WEIGHT))
>>   		return -EINVAL;
>>
>>   	if (PRINT_FIELD(SYM) &&
>> @@ -2036,8 +2036,11 @@ static void process_event(struct perf_script *script,
>>   	if (PRINT_FIELD(DATA_SRC))
>>   		data_src__fprintf(sample->data_src, fp);
>>
>> -	if (PRINT_FIELD(WEIGHT))
>> +	if (PRINT_FIELD(WEIGHT)) {
>>   		fprintf(fp, "%16" PRIu64, sample->weight);
>> +		if (attr->sample_type & PERF_SAMPLE_WEIGHT_STRUCT)
>> +			fprintf(fp, ",0x%" PRIx16, sample->ins_lat);
> 
> this will add extra number to weight.. should we add separate column
> for that -F 'insn-lat' ? users expect just single value for -F weight
> 

Sure, I will send out a patch set to LKML for review shortly.

Thanks,
Kan