| 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: <4c670bf6-c0e4-1f9d-455c-a4bf9fd005fc@linux.intel.com>
Date: Wed, 27 May 2020 15:26:38 +0800
From: "Jin, Yao" <yao.jin@...ux.intel.com>
To: Jiri Olsa <jolsa@...hat.com>
Cc: acme@...nel.org, jolsa@...nel.org, peterz@...radead.org,
mingo@...hat.com, alexander.shishkin@...ux.intel.com,
Linux-kernel@...r.kernel.org, ak@...ux.intel.com,
kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu
map
Hi Jiri,
On 5/27/2020 2:31 PM, Jin, Yao wrote:
> Hi Jiri,
>
> On 5/27/2020 11:20 AM, Jin, Yao wrote:
>> Hi Jiri,
>>
>> On 5/26/2020 7:51 PM, Jiri Olsa wrote:
>>> On Mon, May 25, 2020 at 02:55:58PM +0800, Jin Yao wrote:
>>>
>>> SNIP
>>>
>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>> index 2a9de6491700..1161cffc0688 100644
>>>> --- a/tools/perf/util/evlist.c
>>>> +++ b/tools/perf/util/evlist.c
>>>> @@ -1704,3 +1704,52 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
>>>> }
>>>> return leader;
>>>> }
>>>> +
>>>> +static bool cpus_map_matched(struct evsel *prev, struct evsel *evsel)
>>>> +{
>>>> + if (evsel->core.cpus->nr != prev->core.cpus->nr)
>>>> + return false;
>>>> +
>>>> + for (int i = 0; i < evsel->core.cpus->nr; i++) {
>>>> + if (evsel->core.cpus->map[i] != prev->core.cpus->map[i])
>>>> + return false;
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +bool evlist__cpus_map_matched(struct evlist *evlist)
>>>> +{
>>>> + struct evsel *prev = evlist__first(evlist), *evsel = prev;
>>>> + int nr_members = prev->core.nr_members;
>>>> +
>>>> + evlist__for_each_entry_continue(evlist, evsel) {
>>>> + if (nr_members <= 1) {
>>>> + prev = evsel;
>>>> + nr_members = evsel->core.nr_members;
>>>> + continue;
>>>> + }
>>>> +
>>>> + nr_members--;
>>>> +
>>>> + if (!cpus_map_matched(prev, evsel))
>>>> + return false;
>>>> +
>>>> + prev = evsel;
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +void evlist__force_disable_group(struct evlist *evlist)
>>>> +{
>>>> + struct evsel *evsel;
>>>> +
>>>> + pr_warning("WARNING: event cpu maps are not fully matched, "
>>>> + "stop event grouping\n");
>>>> +
>>>> + evlist__for_each_entry(evlist, evsel) {
>>>> + evsel->leader = evsel;
>>>> + evsel->core.nr_members = 0;
>>>> + }
>>>> +}
>>>
>>> I think this is too much, we need to disable only groups with not
>>> matching cpus, not all of them, how about something like this
>>>
>>
>> Yes, that's too much.
>>
>>>
>>> struct evsel *pos;
>>>
>>> evlist__for_each_entry(evlist, evsel) {
>>> if (evsel->leader == evsel)
>>> continue;
>>> if (!cpus_map_matched(evsel->leader, evsel))
>>> continue;
>>> pr_warn("Disabling group...
>>>
>>> for_each_group_member(pos, evsel->leader) {
>>> pos->leader = pos;
>>> evsel->core.nr_members = 0;
>>> }
>>> }
>>>
>>> jirka
>>>
>>
>> Hmm, change "!cpus_map_matched()" to "cpus_map_matched()"? and use for_each_group_evsel() to
>> replace for_each_group_member()?
>>
>> How about something like following?
>>
>> void evlist__check_cpu_maps(struct evlist *evlist)
>> {
>> struct evsel *evsel, *pos;
>>
>> evlist__for_each_entry(evlist, evsel) {
>> if (evsel->leader == evsel)
>> continue;
>>
>> if (cpu_maps_matched(evsel->leader, evsel))
>> continue;
>>
>> pr_warning("WARNING: event cpu maps are not fully matched, "
>> "disable group\n");
>>
>> for_each_group_evsel(pos, evsel->leader) {
>> pos->leader = pos;
>> pos->core.nr_members = 0;
>> }
>>
>> /*
>> * For core & uncore mixed event group, for example,
>> * '{cycles,unc_cbo_cache_lookup.any_i}',
>> * In evlist:
>> * cycles,
>> * unc_cbo_cache_lookup.any_i,
>> * unc_cbo_cache_lookup.any_i,
>> * unc_cbo_cache_lookup.any_i,
>> * unc_cbo_cache_lookup.any_i,
>> *
>> * cycles is leader and all unc_cbo_cache_lookup.any_i
>> * point to this leader. But for_each_group_evsel can't
>> * iterate all members from cycles. It only iterates
>> * cycles and one unc_cbo_cache_lookup.any_i. So we
>> * set extra evsel here.
>> */
>> evsel->leader = evsel;
>> evsel->core.nr_members = 0;
>> }
>> }
>>
>> Thanks
>> Jin Yao
>
> Issue is found!
>
> It looks we can't set "pos->leader = pos" in either for_each_group_member() or in
> for_each_group_evsel() because it may exit the iteration immediately.
>
> evlist__for_each_entry(evlist, evsel) {
> if (evsel->leader == evsel)
> continue;
>
> if (cpu_maps_matched(evsel->leader, evsel))
> continue;
>
> pr_warning("WARNING: event cpu maps are not fully matched, "
> "disable group\n");
>
> for_each_group_member(pos, evsel->leader) {
> pos->leader = pos;
> pos->core.nr_members = 0;
> }
>
> Let me use the example of '{cycles,unc_cbo_cache_lookup.any_i}' again.
>
> In evlist:
> cycles,
> unc_cbo_cache_lookup.any_i,
> unc_cbo_cache_lookup.any_i,
> unc_cbo_cache_lookup.any_i,
> unc_cbo_cache_lookup.any_i,
>
> When we reach the for_each_group_member at first time, evsel is the first unc_cbo_cache_lookup.any_i
> and evsel->leader is cycles. pos is same as the evsel (the first unc_cbo_cache_lookup.any_i).
>
> Once we execute "pos->leader = pos;", it's actually "evsel->leader = evsel". So now evsel->leader is
> changed to the first unc_cbo_cache_lookup.any_i.
>
> In next iteration, pos is the second unc_cbo_cache_lookup.any_i. pos->leader is cycles but
> unfortunately evsel->leader has been changed to the first unc_cbo_cache_lookup.any_i. So iteration
> stops immediately.
>
> I'm now thinking if we can solve this issue by an easy way.
>
> Thanks
> Jin Yao
How about this fix?
void evlist__check_cpu_maps(struct evlist *evlist)
{
struct evsel *evsel, *pos, *tmp;
evlist__for_each_entry(evlist, evsel) {
if (evsel->leader == evsel)
continue;
if (cpu_maps_matched(evsel->leader, evsel))
continue;
pr_warning("WARNING: event cpu maps are not fully matched, "
"disable group\n");
for_each_group_member(pos, evsel->leader) {
if (pos != evsel) {
pos->leader = pos;
pos->core.nr_members = 0;
}
}
tmp = evsel->leader;
tmp->leader = tmp;
tmp->core.nr_members = 0;
evsel->leader = evsel;
evsel->core.nr_members = 0;
}
}
Thanks
Jin Yao
Powered by blists - more mailing lists