| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ca64f76f-ccf4-3779-4090-6028b7ee7bef@huawei.com>
Date: Tue, 18 Jul 2023 19:32:15 +0800
From: Yang Jihong <yangjihong1@...wei.com>
To: Adrian Hunter <adrian.hunter@...el.com>, <peterz@...radead.org>,
<mingo@...hat.com>, <acme@...nel.org>, <mark.rutland@....com>,
<alexander.shishkin@...ux.intel.com>, <jolsa@...nel.org>,
<namhyung@...nel.org>, <irogers@...gle.com>,
<kan.liang@...ux.intel.com>, <james.clark@....com>,
<tmricht@...ux.ibm.com>, <ak@...ux.intel.com>,
<anshuman.khandual@....com>, <linux-kernel@...r.kernel.org>,
<linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH v2 5/7] perf evlist: Skip dummy event sample_type check
for evlist_config
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@...wei.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
Powered by blists - more mailing lists