Re: [PATCH v2 3/3] perf-stat: introduce config stat.bpf-counter-events

[Song Liu]

> On Apr 8, 2021, at 11:24 AM, Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> 
> On Thu, Apr 08, 2021 at 06:08:20PM +0000, Song Liu wrote:
>> 
>> 
>>> On Apr 8, 2021, at 10:45 AM, Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>>> 
>>> On Thu, Apr 08, 2021 at 05:28:10PM +0000, Song Liu wrote:
>>>> 
>>>> 
>>>>> On Apr 8, 2021, at 10:20 AM, Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>>>>> 
>>>>> On Thu, Apr 08, 2021 at 04:39:33PM +0000, Song Liu wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Apr 8, 2021, at 4:47 AM, Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>>>>>>> 
>>>>>>> On Tue, Apr 06, 2021 at 05:36:01PM -0700, Song Liu wrote:
>>>>>>>> Currently, to use BPF to aggregate perf event counters, the user uses
>>>>>>>> --bpf-counters option. Enable "use bpf by default" events with a config
>>>>>>>> option, stat.bpf-counter-events. This is limited to hardware events in
>>>>>>>> evsel__hw_names.
>>>>>>>> 
>>>>>>>> This also enables mixed BPF event and regular event in the same sesssion.
>>>>>>>> For example:
>>>>>>>> 
>>>>>>>> perf config stat.bpf-counter-events=instructions
>>>>>>>> perf stat -e instructions,cs
>>>>>>>> 
>>>>>>> 
>>>>>>> so if we are mixing events now, how about uing modifier for bpf counters,
>>>>>>> instead of configuring .perfconfig list we could use:
>>>>>>> 
>>>>>>> perf stat -e instructions:b,cs
>>>>>>> 
>>>>>>> thoughts?
>>>>>>> 
>>>>>>> the change below adds 'b' modifier and sets 'evsel::bpf_counter',
>>>>>>> feel free to use it
>>>>>> 
>>>>>> I think we will need both 'b' modifier and .perfconfig configuration. 
>>>>>> For systems with BPF-managed perf events running in the background, 
>>>>> 
>>>>> hum, I'm not sure I understand what that means.. you mean there
>>>>> are tools that run perf stat so you don't want to change them?
>>>> 
>>>> We have tools that do perf_event_open(). I will change them to use 
>>>> BPF managed perf events for "cycles" and "instructions". Since these 
>>>> tools are running 24/7, perf-stat on the system should use BPF managed
>>>> "cycles" and "instructions" by default. 
>>> 
>>> well if you are already changing the tools why not change them to add
>>> modifier.. but I don't mind adding that .perfconfig stuff if you need
>>> that
>> 
>> The tools I mentioned here don't use perf-stat, they just use 
>> perf_event_open() and read the perf events fds. We want a config to make
> 
> just curious, how those tools use perf_event_open?
> 
>> "cycles" to use BPF by default, so that when the user (not these tools)
>> runs perf-stat, it will share PMCs with those events by default. 
> 
> I'm sorry but I still don't see the usecase.. if you need to change both tools,
> you can change them to use bpf-managed event, why bother with the list?
> 
>>> 
>>>> 
>>>>> 
>>>>>> .perfconfig makes sure perf-stat sessions will share PMCs with these 
>>>>>> background monitoring tools. 'b' modifier, on the other hand, is useful
>>>>>> when the user knows there is opportunity to share the PMCs. 
>>>>>> 
>>>>>> Does this make sense? 
>>>>> 
>>>>> if there's reason for that then sure.. but let's not limit that just
>>>>> on HARDWARE events only.. there are RAW events with the same demand
>>>>> for this feature.. why don't we let user define any event for this?
>>>> 
>>>> I haven't found a good way to config RAW events. I guess RAW events 
>>>> could use 'b' modifier? 
>>> any event uing the pmu notation like cpu/instructions/
>> 
>> Can we do something like "perf config stat.bpf-counter-events=cpu/*" means 
>> all "cpu/xx" events use BPF by default?
> 
> I think there's misundestanding, all I'm saying is that IIUC you check
> events stat.bpf-counter-events to be HARDWARE type, which I don't think
> is necessary and we can allow any event in there

>From what I see, most of the opportunity of sharing comes from a few common
events, like cycles, instructions. The second reason is that, the config 
implementation is easy and straightforward. We sure can extend the config 
to other events. Before that, 'b' modifier should be good for these use
cases. 

Thanks,
Song