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

[Yang Jihong]

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@xxxxxxxxxx>
> > > > > > ---
> > > > > >     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