diff mbox series

[v2,5/7] perf evlist: Skip dummy event sample_type check for evlist_config

Message ID 20230715032915.97146-6-yangjihong1@huawei.com
State New
Headers
Series perf record: Track sideband events for all CPUs when tracing selected CPUs |

Commit Message

Yang Jihong July 15, 2023, 3:29 a.m. UTC 
  The dummp event does not contain sampls data. Therefore, sample_type does
not need to be checked.

Currently, the sample id format of the actual sampling event may be changed
after the dummy event is added.

Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
 tools/perf/util/record.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Adrian Hunter July 17, 2023, 2:41 p.m. UTC | #1 
On 15/07/23 06:29, Yang Jihong wrote:
> The dummp event does not contain sampls data. Therefore, sample_type does
> not need to be checked.
> 
> Currently, the sample id format of the actual sampling event may be changed
> after the dummy event is added.
> 
> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> ---
>  tools/perf/util/record.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> index 9eb5c6a08999..0240be3b340f 100644
> --- a/tools/perf/util/record.c
> +++ b/tools/perf/util/record.c
> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>  		evlist__for_each_entry(evlist, evsel) {
>  			if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>  				continue;
> +
> +			/*
> +			 * Skip the sample_type check for the dummy event
> +			 * because it does not have any samples anyway.
> +			 */
> +			if (evsel__is_dummy_event(evsel))
> +				continue;

Sideband event records have "ID samples" so the sample type still matters.


>  			use_sample_identifier = perf_can_sample_identifier();
>  			break;
>  		}
Yang Jihong July 18, 2023, 9:30 a.m. UTC | #2 
Hello,

On 2023/7/17 22:41, Adrian Hunter wrote:
> On 15/07/23 06:29, Yang Jihong wrote:
>> The dummp event does not contain sampls data. Therefore, sample_type does
>> not need to be checked.
>>
>> Currently, the sample id format of the actual sampling event may be changed
>> after the dummy event is added.
>>
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   tools/perf/util/record.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>> index 9eb5c6a08999..0240be3b340f 100644
>> --- a/tools/perf/util/record.c
>> +++ b/tools/perf/util/record.c
>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>   		evlist__for_each_entry(evlist, evsel) {
>>   			if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>   				continue;
>> +
>> +			/*
>> +			 * Skip the sample_type check for the dummy event
>> +			 * because it does not have any samples anyway.
>> +			 */
>> +			if (evsel__is_dummy_event(evsel))
>> +				continue;
> 
> Sideband event records have "ID samples" so the sample type still matters.
> 
Okay, will remove this patch in next version.

Can I ask a little more about this?

Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for 
samples of type PERF_RECORD_SAMPLE, there may be different record 
formats due to different *sample_type* settings, so the fixed SAMPLE_ID 
  location mode PERF_SAMPLE_NAME is required here.

However, for the sideband event, the samples of the PERF_RECORD_SAMPLE 
type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so 
on). Therefore, the "use sample identifier "check can be skipped here.

That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any 
error, please help to correct it.

*Sideband event records have "ID samples" so the sample type still matters.*

Does this mean that sideband will also record samples of type 
PERF_RECORD_SAMPLE? What exactly is the sampling data?

Thanks,
Yang
Adrian Hunter July 18, 2023, 9:56 a.m. UTC | #3 
On 18/07/23 12:30, Yang Jihong wrote:
> Hello,
> 
> On 2023/7/17 22:41, Adrian Hunter wrote:
>> On 15/07/23 06:29, Yang Jihong wrote:
>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>> not need to be checked.
>>>
>>> Currently, the sample id format of the actual sampling event may be changed
>>> after the dummy event is added.
>>>
>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>> ---
>>>   tools/perf/util/record.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>> index 9eb5c6a08999..0240be3b340f 100644
>>> --- a/tools/perf/util/record.c
>>> +++ b/tools/perf/util/record.c
>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>           evlist__for_each_entry(evlist, evsel) {
>>>               if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>                   continue;
>>> +
>>> +            /*
>>> +             * Skip the sample_type check for the dummy event
>>> +             * because it does not have any samples anyway.
>>> +             */
>>> +            if (evsel__is_dummy_event(evsel))
>>> +                continue;
>>
>> Sideband event records have "ID samples" so the sample type still matters.
>>
> Okay, will remove this patch in next version.
> 
> Can I ask a little more about this?
> 
> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
> 
> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
> 
> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
> 
> *Sideband event records have "ID samples" so the sample type still matters.*
> 
> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?

No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
Yang Jihong July 18, 2023, 10:17 a.m. UTC | #4 
Hello,

On 2023/7/18 17:56, Adrian Hunter wrote:
> On 18/07/23 12:30, Yang Jihong wrote:
>> Hello,
>>
>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>> not need to be checked.
>>>>
>>>> Currently, the sample id format of the actual sampling event may be changed
>>>> after the dummy event is added.
>>>>
>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>> ---
>>>>    tools/perf/util/record.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>> --- a/tools/perf/util/record.c
>>>> +++ b/tools/perf/util/record.c
>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>            evlist__for_each_entry(evlist, evsel) {
>>>>                if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>                    continue;
>>>> +
>>>> +            /*
>>>> +             * Skip the sample_type check for the dummy event
>>>> +             * because it does not have any samples anyway.
>>>> +             */
>>>> +            if (evsel__is_dummy_event(evsel))
>>>> +                continue;
>>>
>>> Sideband event records have "ID samples" so the sample type still matters.
>>>
>> Okay, will remove this patch in next version.
>>
>> Can I ask a little more about this?
>>
>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>
>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>
>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>
>> *Sideband event records have "ID samples" so the sample type still matters.*
>>
>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
> 
> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
> 
I'm sorry, maybe my comments didn't make it clear.
I mean, can we skip the "use_sample_identifier" check here?

That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of 
*XXX|PERF_SAMPLE_IDENTIFICATION*

Thanks,
Yang
Adrian Hunter July 18, 2023, 10:29 a.m. UTC | #5 
On 18/07/23 13:17, Yang Jihong wrote:
> Hello,
> 
> On 2023/7/18 17:56, Adrian Hunter wrote:
>> On 18/07/23 12:30, Yang Jihong wrote:
>>> Hello,
>>>
>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>> not need to be checked.
>>>>>
>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>> after the dummy event is added.
>>>>>
>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>> ---
>>>>>    tools/perf/util/record.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>> --- a/tools/perf/util/record.c
>>>>> +++ b/tools/perf/util/record.c
>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>            evlist__for_each_entry(evlist, evsel) {
>>>>>                if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>                    continue;
>>>>> +
>>>>> +            /*
>>>>> +             * Skip the sample_type check for the dummy event
>>>>> +             * because it does not have any samples anyway.
>>>>> +             */
>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>> +                continue;
>>>>
>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>
>>> Okay, will remove this patch in next version.
>>>
>>> Can I ask a little more about this?
>>>
>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>
>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>
>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>
>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>
>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>
>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>
> I'm sorry, maybe my comments didn't make it clear.
> I mean, can we skip the "use_sample_identifier" check here?
> 
> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*

In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.

Why do want to change it?
Yang Jihong July 18, 2023, 11:32 a.m. UTC | #6 
Hello,

On 2023/7/18 18:29, Adrian Hunter wrote:
> On 18/07/23 13:17, Yang Jihong wrote:
>> Hello,
>>
>> On 2023/7/18 17:56, Adrian Hunter wrote:
>>> On 18/07/23 12:30, Yang Jihong wrote:
>>>> Hello,
>>>>
>>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>>> not need to be checked.
>>>>>>
>>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>>> after the dummy event is added.
>>>>>>
>>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>>> ---
>>>>>>     tools/perf/util/record.c | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>>> --- a/tools/perf/util/record.c
>>>>>> +++ b/tools/perf/util/record.c
>>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>>             evlist__for_each_entry(evlist, evsel) {
>>>>>>                 if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>>                     continue;
>>>>>> +
>>>>>> +            /*
>>>>>> +             * Skip the sample_type check for the dummy event
>>>>>> +             * because it does not have any samples anyway.
>>>>>> +             */
>>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>>> +                continue;
>>>>>
>>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>>
>>>> Okay, will remove this patch in next version.
>>>>
>>>> Can I ask a little more about this?
>>>>
>>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>>
>>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>>
>>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>>
>>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>>
>>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>>
>>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>>
>> I'm sorry, maybe my comments didn't make it clear.
>> I mean, can we skip the "use_sample_identifier" check here?
>>
>> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*
> 
> In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
> But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.
> 
> Why do want to change it?

Without this patch, we now add a system_wide tracking event and modify 
the sample_type of the  actual sample event.

For example, when we run:
   # perf record -e cycles -C 0

1. The default sample_type of the "cycles" is IP|TID|TIME|CPU|PERIOD.
2. Then add a system_wide sideband event whose sample_type is 
IP|TID|TIME|CPU.
3. The two sample_types are different.
4. Therefore, the evlist__config adds a PERF_SAMPLE_IDENTIFICATION to 
both sample_types instead of PERF_SAMPLE_ID.

evlist__config {
         ...
          } else if (evlist->core.nr_entries > 1) {
          // One is cycles and the other is sideband . 

                  struct evsel *first = evlist__first(evlist); 

 

                  evlist__for_each_entry(evlist, evsel) { 

                          if (evsel->core.attr.sample_type == 
first->core.attr.sample_type)
                                  continue; 

                          use_sample_identifier = 
perf_can_sample_identifier();
                          // the sample_type of cycles is different from 
that of sideband.
						 // Therefore, use_sample_identifier is set to true.
                          break; 

                  } 

                  sample_id = true; 

          } 

 

          if (sample_id) { 

                  evlist__for_each_entry(evlist, evsel) 

                          evsel__set_sample_id(evsel, 
use_sample_identifier);
                          // both cycles and sideband set 
PERF_SAMPLE_IDENTIFICATION  						
          } 

         ...
}

The comparison of the sideband event sample_type is skipped so that the 
sample_type of the original cycles is not changed.

It does not seem necessary to compare the sample_type of sidebband 
event. It is not an actual sample event, so I'd like to confirm that.

If the change is as expected and necessary, then I'll remove the patch.

Thanks,
Yang
Adrian Hunter July 20, 2023, 5:41 a.m. UTC | #7 
On 18/07/23 14:32, Yang Jihong wrote:
> Hello,
> 
> On 2023/7/18 18:29, Adrian Hunter wrote:
>> On 18/07/23 13:17, Yang Jihong wrote:
>>> Hello,
>>>
>>> On 2023/7/18 17:56, Adrian Hunter wrote:
>>>> On 18/07/23 12:30, Yang Jihong wrote:
>>>>> Hello,
>>>>>
>>>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>>>> not need to be checked.
>>>>>>>
>>>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>>>> after the dummy event is added.
>>>>>>>
>>>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>>>> ---
>>>>>>>     tools/perf/util/record.c | 7 +++++++
>>>>>>>     1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>>>> --- a/tools/perf/util/record.c
>>>>>>> +++ b/tools/perf/util/record.c
>>>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>>>             evlist__for_each_entry(evlist, evsel) {
>>>>>>>                 if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>>>                     continue;
>>>>>>> +
>>>>>>> +            /*
>>>>>>> +             * Skip the sample_type check for the dummy event
>>>>>>> +             * because it does not have any samples anyway.
>>>>>>> +             */
>>>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>>>> +                continue;
>>>>>>
>>>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>>>
>>>>> Okay, will remove this patch in next version.
>>>>>
>>>>> Can I ask a little more about this?
>>>>>
>>>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>>>
>>>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>>>
>>>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>>>
>>>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>>>
>>>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>>>
>>>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>>>
>>> I'm sorry, maybe my comments didn't make it clear.
>>> I mean, can we skip the "use_sample_identifier" check here?
>>>
>>> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*
>>
>> In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
>> But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.
>>
>> Why do want to change it?
> 
> Without this patch, we now add a system_wide tracking event and modify the sample_type of the  actual sample event.
> 
> For example, when we run:
>   # perf record -e cycles -C 0
> 
> 1. The default sample_type of the "cycles" is IP|TID|TIME|CPU|PERIOD.
> 2. Then add a system_wide sideband event whose sample_type is IP|TID|TIME|CPU.
> 3. The two sample_types are different.
> 4. Therefore, the evlist__config adds a PERF_SAMPLE_IDENTIFICATION to both sample_types instead of PERF_SAMPLE_ID.
> 
> evlist__config {
>         ...
>          } else if (evlist->core.nr_entries > 1) {
>          // One is cycles and the other is sideband .
>                  struct evsel *first = evlist__first(evlist);
> 
> 
>                  evlist__for_each_entry(evlist, evsel) {
>                          if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>                                  continue;
>                          use_sample_identifier = perf_can_sample_identifier();
>                          // the sample_type of cycles is different from that of sideband.
>                          // Therefore, use_sample_identifier is set to true.
>                          break;
>                  }
>                  sample_id = true;
>          }
> 
> 
>          if (sample_id) {
>                  evlist__for_each_entry(evlist, evsel)
>                          evsel__set_sample_id(evsel, use_sample_identifier);
>                          // both cycles and sideband set PERF_SAMPLE_IDENTIFICATION                         
>          }
>         ...
> }
> 
> The comparison of the sideband event sample_type is skipped so that the sample_type of the original cycles is not changed.
> 
> It does not seem necessary to compare the sample_type of sidebband event. It is not an actual sample event, so I'd like to confirm that.

It is necessary.  The sample type is used to parse ID samples
that are part of e.g. MMAP events - refer perf_evsel__parse_id_sample()

We could teach perf to handle dummy events differently because they
do not use all the sample_type bits (only the ones in perf_evsel__parse_id_sample())
but that is probably not backward compatible.

The only value in that would be to make it work without
PERF_SAMPLE_ID or PERF_SAMPLE_IDENTIFIER because that would
save 8-bytes per event record.

Otherwise there is no benefit to prefer PERF_SAMPLE_ID over
PERF_SAMPLE_IDENTIFIER except backward compatibility with
some other tool that doesn't know about PERF_SAMPLE_IDENTIFIER.

> 
> If the change is as expected and necessary, then I'll remove the patch.
> 
> Thanks,
> Yang
>
Yang Jihong July 20, 2023, 7:25 a.m. UTC | #8 
Hello,

On 2023/7/20 13:41, Adrian Hunter wrote:
> On 18/07/23 14:32, Yang Jihong wrote:
>> Hello,
>>
>> On 2023/7/18 18:29, Adrian Hunter wrote:
>>> On 18/07/23 13:17, Yang Jihong wrote:
>>>> Hello,
>>>>
>>>> On 2023/7/18 17:56, Adrian Hunter wrote:
>>>>> On 18/07/23 12:30, Yang Jihong wrote:
>>>>>> Hello,
>>>>>>
>>>>>> On 2023/7/17 22:41, Adrian Hunter wrote:
>>>>>>> On 15/07/23 06:29, Yang Jihong wrote:
>>>>>>>> The dummp event does not contain sampls data. Therefore, sample_type does
>>>>>>>> not need to be checked.
>>>>>>>>
>>>>>>>> Currently, the sample id format of the actual sampling event may be changed
>>>>>>>> after the dummy event is added.
>>>>>>>>
>>>>>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>>>>>>>> ---
>>>>>>>>      tools/perf/util/record.c | 7 +++++++
>>>>>>>>      1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>>>>>>> index 9eb5c6a08999..0240be3b340f 100644
>>>>>>>> --- a/tools/perf/util/record.c
>>>>>>>> +++ b/tools/perf/util/record.c
>>>>>>>> @@ -128,6 +128,13 @@ void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
>>>>>>>>              evlist__for_each_entry(evlist, evsel) {
>>>>>>>>                  if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>>>>>>>                      continue;
>>>>>>>> +
>>>>>>>> +            /*
>>>>>>>> +             * Skip the sample_type check for the dummy event
>>>>>>>> +             * because it does not have any samples anyway.
>>>>>>>> +             */
>>>>>>>> +            if (evsel__is_dummy_event(evsel))
>>>>>>>> +                continue;
>>>>>>>
>>>>>>> Sideband event records have "ID samples" so the sample type still matters.
>>>>>>>
>>>>>> Okay, will remove this patch in next version.
>>>>>>
>>>>>> Can I ask a little more about this?
>>>>>>
>>>>>> Use PERF_SAMPLE_IDENTIFICATION instead of PERF_SAMPLE_ID because for samples of type PERF_RECORD_SAMPLE, there may be different record formats due to different *sample_type* settings, so the fixed SAMPLE_ID  location mode PERF_SAMPLE_NAME is required here.
>>>>>>
>>>>>> However, for the sideband event, the samples of the PERF_RECORD_SAMPLE type is not recorded (only PERF_RECORD_MMAP, PERF_RECORD_COMM, and so on). Therefore, the "use sample identifier "check can be skipped here.
>>>>>>
>>>>>> That's my understanding of PERF_SAMPLE_IDENTIFICATION . If there is any error, please help to correct it.
>>>>>>
>>>>>> *Sideband event records have "ID samples" so the sample type still matters.*
>>>>>>
>>>>>> Does this mean that sideband will also record samples of type PERF_RECORD_SAMPLE? What exactly is the sampling data?
>>>>>
>>>>> No.  There are additional members as defined by struct sample_id for PERF_RECORD_MMAP:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v6.4#n872
>>>>>
>>>> I'm sorry, maybe my comments didn't make it clear.
>>>> I mean, can we skip the "use_sample_identifier" check here?
>>>>
>>>> That is, set sample_type to *XXX|PERF_SAMPLE_ID* instead of *XXX|PERF_SAMPLE_IDENTIFICATION*
>>>
>>> In general, when there are different values of sample_type, the PERF_SAMPLE_ID is needed to determine which is which.
>>> But PERF_SAMPLE_ID is not at a fixed position, so the sample_type is needed to find it.  That is why PERF_SAMPLE_IDENTIFIER is better.
>>>
>>> Why do want to change it?
>>
>> Without this patch, we now add a system_wide tracking event and modify the sample_type of the  actual sample event.
>>
>> For example, when we run:
>>    # perf record -e cycles -C 0
>>
>> 1. The default sample_type of the "cycles" is IP|TID|TIME|CPU|PERIOD.
>> 2. Then add a system_wide sideband event whose sample_type is IP|TID|TIME|CPU.
>> 3. The two sample_types are different.
>> 4. Therefore, the evlist__config adds a PERF_SAMPLE_IDENTIFICATION to both sample_types instead of PERF_SAMPLE_ID.
>>
>> evlist__config {
>>          ...
>>           } else if (evlist->core.nr_entries > 1) {
>>           // One is cycles and the other is sideband .
>>                   struct evsel *first = evlist__first(evlist);
>>
>>
>>                   evlist__for_each_entry(evlist, evsel) {
>>                           if (evsel->core.attr.sample_type == first->core.attr.sample_type)
>>                                   continue;
>>                           use_sample_identifier = perf_can_sample_identifier();
>>                           // the sample_type of cycles is different from that of sideband.
>>                           // Therefore, use_sample_identifier is set to true.
>>                           break;
>>                   }
>>                   sample_id = true;
>>           }
>>
>>
>>           if (sample_id) {
>>                   evlist__for_each_entry(evlist, evsel)
>>                           evsel__set_sample_id(evsel, use_sample_identifier);
>>                           // both cycles and sideband set PERF_SAMPLE_IDENTIFICATION
>>           }
>>          ...
>> }
>>
>> The comparison of the sideband event sample_type is skipped so that the sample_type of the original cycles is not changed.
>>
>> It does not seem necessary to compare the sample_type of sidebband event. It is not an actual sample event, so I'd like to confirm that.
> 
> It is necessary.  The sample type is used to parse ID samples
> that are part of e.g. MMAP events - refer perf_evsel__parse_id_sample()
> 
> We could teach perf to handle dummy events differently because they
> do not use all the sample_type bits (only the ones in perf_evsel__parse_id_sample())
> but that is probably not backward compatible.
> 
> The only value in that would be to make it work without
> PERF_SAMPLE_ID or PERF_SAMPLE_IDENTIFIER because that would
> save 8-bytes per event record.
> 
> Otherwise there is no benefit to prefer PERF_SAMPLE_ID over
> PERF_SAMPLE_IDENTIFIER except backward compatibility with
> some other tool that doesn't know about PERF_SAMPLE_IDENTIFIER.
> 
Thanks for detailed instructions, I understand, OK, will remove this 
patch in the next version.

Thanks,
Yang
diff mbox series

Patch

diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
index 9eb5c6a08999..0240be3b340f 100644
--- a/tools/perf/util/record.c
+++ b/tools/perf/util/record.c
@@ -128,6 +128,13 @@  void evlist__config(struct evlist *evlist, struct record_opts *opts, struct call
 		evlist__for_each_entry(evlist, evsel) {
 			if (evsel->core.attr.sample_type == first->core.attr.sample_type)
 				continue;
+
+			/*
+			 * Skip the sample_type check for the dummy event
+			 * because it does not have any samples anyway.
+			 */
+			if (evsel__is_dummy_event(evsel))
+				continue;
 			use_sample_identifier = perf_can_sample_identifier();
 			break;
 		}