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

[Adrian Hunter <adrian.hunter@intel.com>]

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
>