Messages in this thread | | | Subject | Re: [PATCH v3 3/3] perf tools: Enable on a list of CPUs for hybrid | From | "Jin, Yao" <> | Date | Fri, 23 Jul 2021 08:49:01 +0800 |
| |
Hi Jiri,
On 7/22/2021 6:19 PM, Jiri Olsa wrote: > On Wed, Jul 21, 2021 at 12:30:11PM +0800, Jin, Yao wrote: >> Hi Jiri, >> >> On 7/20/2021 5:16 PM, Jiri Olsa wrote: >>> On Tue, Jul 20, 2021 at 03:07:02PM +0800, Jin, Yao wrote: >>> >>> SNIP >>> >>>> >>>> OK, evlist__fix_cpus() is better, use this name in v4. >>>> >>>>>> +{ >>>>>> + struct perf_cpu_map *cpus; >>>>>> + struct evsel *evsel, *tmp; >>>>>> + struct perf_pmu *pmu; >>>>>> + int ret, unmatched_count = 0, events_nr = 0; >>>>>> + >>>>>> + if (!perf_pmu__has_hybrid() || !cpu_list) >>>>>> + return 0; >>>>>> + >>>>>> + cpus = perf_cpu_map__new(cpu_list); >>>>>> + if (!cpus) >>>>>> + return -1; >>>>>> + >>>>>> + evlist__for_each_entry_safe(evlist, tmp, evsel) { >>>>>> + struct perf_cpu_map *matched_cpus, *unmatched_cpus; >>>>>> + char buf1[128], buf2[128]; >>>>>> + >>>>>> + pmu = perf_pmu__find_hybrid_pmu(evsel->pmu_name); >>>>>> + if (!pmu) >>>>>> + continue; >>>>>> + >>>>>> + ret = perf_pmu__cpus_match(pmu, cpus, &matched_cpus, >>>>>> + &unmatched_cpus); >>>>>> + if (ret) >>>>>> + goto out; >>>>>> + >>>>>> + events_nr++; >>>>>> + >>>>>> + if (matched_cpus->nr > 0 && (unmatched_cpus->nr > 0 || >>>>>> + matched_cpus->nr < cpus->nr || >>>>>> + matched_cpus->nr < pmu->cpus->nr)) { >>>>>> + perf_cpu_map__put(evsel->core.cpus); >>>>>> + perf_cpu_map__put(evsel->core.own_cpus); >>>>>> + evsel->core.cpus = perf_cpu_map__get(matched_cpus); >>>>>> + evsel->core.own_cpus = perf_cpu_map__get(matched_cpus); >>>>> >>>>> I'm bit confused in here.. AFAIUI there's 2 evsel objects create >>>>> for hybrid 'cycles' ... should they have already proper cpus set? >>>>> >>>> >>>> For 'cycles', yes two evsels are created automatically. One is for atom CPU >>>> (e.g. 8-11), the other is for core CPU (e.g. 0-7). In this example, these 2 >>>> evsels have already the cpus set. >>> >>> hum, so those evsels are created with pmu's cpus, right? >>> >> >> Yes, that's right. But we also check and adjust the evsel->cpus by using >> user's cpu list on hybrid (what the evlist__use_cpu_list() does). >> >>>> >>>> While the 'cpus' here is just the user specified cpu list. >>>> cpus = perf_cpu_map__new(cpu_list); >>> >>> then I think they will be changed by evlist__create_maps >>> with whatever user wants? >>> >> >> No, it will not be changed by evlist__create_maps. >> >> In evlist__create_maps(), >> evlist->core.has_user_cpus = !!target->cpu_list && !target->hybrid; >> >> It disables has_user_cpus for hybrid. >> >> So in __perf_evlist__propagate_maps, they will not be changed by evlist->cpus. >> >> if (!evsel->own_cpus || evlist->has_user_cpus) { >> perf_cpu_map__put(evsel->cpus); >> evsel->cpus = perf_cpu_map__get(evlist->cpus); >> >>> could we just change __perf_evlist__propagate_maps to follow >>> pmu's cpus? >>> >> >> In __perf_evlist__propagate_maps, it has already followed pmu's cpus because >> the evlist->has_user_cpus is false for hybrid. > > sorry for delay >
Never mind. :)
> ok, so we first fix the cpus on hybrid events and then > propagate maps.. I guess it's ok, because it's in libperf > and that has no notion of hybrid so far >
Yes. If we want the libperf to be hybrid aware, the interface has to be modified but actually we need to avoid modifying the libperf interface. So I finally decide to adjust the evsel->cpus first and then propatate maps.
> could you please rename that function so it's also obvious > it's for hybrid only > > evlist__fix_hybrid_cpus ? not sure ;-) >
Sure, I will rename the funciton in v4.
> and add some comment with example to explain what the > function is doing >
Got it!
Thanks Jin Yao
> thanks, > jirka >
| |