Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Song Liu <songliubraving@fb.com>
To:     Jiri Olsa <jolsa@redhat.com>
CC:     Song Liu <song@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        "jolsa@kernel.org" <jolsa@kernel.org>
Subject: Re: [PATCH v2 3/3] perf-stat: introduce config
 stat.bpf-counter-events
Thread-Topic: [PATCH v2 3/3] perf-stat: introduce config
 stat.bpf-counter-events
Thread-Index: AQHXK0YG4LdD7IbPc0ehmtAjm2+fxaqqg16AgABRlYCAAAtnAIAAAjCAgAAEzwCAAAZpgIAABJqAgAAWyoA=
Date:   Thu, 8 Apr 2021 19:46:23 +0000
Message-ID: <9F632420-4349-4BB0-B156-9739C27B5B2B@fb.com>
References: <20210407003601.619158-1-song@kernel.org>
 <20210407003601.619158-4-song@kernel.org> <YG7tU/mVxQZLNCYL@krava>
 <FA667178-09F3-48C5-891C-9E44744276C0@fb.com> <YG87VLdSrhG0xSDj@krava>
 <0106D99A-5CBB-4CC4-B6F5-1B795E9B8274@fb.com> <YG9BMss16qFOjhos@krava>
 <FDBEC7CA-7F74-4494-93B3-7E5758E5DED9@fb.com> <YG9Kb9izJhQlth3X@krava>
In-Reply-To: <YG9Kb9izJhQlth3X@krava>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <060379E2587731479701DF67B4843CEF@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: cef88d6a-6042-40e9-746d-08d8fac6fd4d
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2021 19:46:23.1626
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: YZmXHGVTcze8orza5g2Te+63Z6Di/fpxByziikQaeDw1JsQ3/ZkWIkKfdzdjp0xo2N0tx8aY1Hs6f5X2y8aJBg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB3512
X-OriginatorOrg: fb.com
X-Proofpoint-ORIG-GUID: Fo8OBSp_2c9qCHLlYKYPMGo3Ymu4Tdsk
X-Proofpoint-GUID: Fo8OBSp_2c9qCHLlYKYPMGo3Ymu4Tdsk
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761
 definitions=2021-04-08_04:2021-04-08,2021-04-08 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 clxscore=1015
 priorityscore=1501 mlxlogscore=999 phishscore=0 malwarescore=0
 suspectscore=0 lowpriorityscore=0 mlxscore=0 adultscore=0 bulkscore=0
 spamscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2104060000 definitions=main-2104080131
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Apr 8, 2021, at 11:24 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>=20
> On Thu, Apr 08, 2021 at 06:08:20PM +0000, Song Liu wrote:
>>=20
>>=20
>>> On Apr 8, 2021, at 10:45 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>>>=20
>>> On Thu, Apr 08, 2021 at 05:28:10PM +0000, Song Liu wrote:
>>>>=20
>>>>=20
>>>>> On Apr 8, 2021, at 10:20 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>>>>>=20
>>>>> On Thu, Apr 08, 2021 at 04:39:33PM +0000, Song Liu wrote:
>>>>>>=20
>>>>>>=20
>>>>>>> On Apr 8, 2021, at 4:47 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>>>>>>>=20
>>>>>>> On Tue, Apr 06, 2021 at 05:36:01PM -0700, Song Liu wrote:
>>>>>>>> Currently, to use BPF to aggregate perf event counters, the user u=
ses
>>>>>>>> --bpf-counters option. Enable "use bpf by default" events with a c=
onfig
>>>>>>>> option, stat.bpf-counter-events. This is limited to hardware event=
s in
>>>>>>>> evsel__hw_names.
>>>>>>>>=20
>>>>>>>> This also enables mixed BPF event and regular event in the same se=
sssion.
>>>>>>>> For example:
>>>>>>>>=20
>>>>>>>> perf config stat.bpf-counter-events=3Dinstructions
>>>>>>>> perf stat -e instructions,cs
>>>>>>>>=20
>>>>>>>=20
>>>>>>> so if we are mixing events now, how about uing modifier for bpf cou=
nters,
>>>>>>> instead of configuring .perfconfig list we could use:
>>>>>>>=20
>>>>>>> perf stat -e instructions:b,cs
>>>>>>>=20
>>>>>>> thoughts?
>>>>>>>=20
>>>>>>> the change below adds 'b' modifier and sets 'evsel::bpf_counter',
>>>>>>> feel free to use it
>>>>>>=20
>>>>>> I think we will need both 'b' modifier and .perfconfig configuration=
.=20
>>>>>> For systems with BPF-managed perf events running in the background,=
=20
>>>>>=20
>>>>> 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?
>>>>=20
>>>> We have tools that do perf_event_open(). I will change them to use=20
>>>> BPF managed perf events for "cycles" and "instructions". Since these=20
>>>> tools are running 24/7, perf-stat on the system should use BPF managed
>>>> "cycles" and "instructions" by default.=20
>>>=20
>>> 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
>>=20
>> The tools I mentioned here don't use perf-stat, they just use=20
>> perf_event_open() and read the perf events fds. We want a config to make
>=20
> just curious, how those tools use perf_event_open?
>=20
>> "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.=20
>=20
> 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?
>=20
>>>=20
>>>>=20
>>>>>=20
>>>>>> .perfconfig makes sure perf-stat sessions will share PMCs with these=
=20
>>>>>> background monitoring tools. 'b' modifier, on the other hand, is use=
ful
>>>>>> when the user knows there is opportunity to share the PMCs.=20
>>>>>>=20
>>>>>> Does this make sense?=20
>>>>>=20
>>>>> 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?
>>>>=20
>>>> I haven't found a good way to config RAW events. I guess RAW events=20
>>>> could use 'b' modifier?=20
>>> any event uing the pmu notation like cpu/instructions/
>>=20
>> Can we do something like "perf config stat.bpf-counter-events=3Dcpu/*" m=
eans=20
>> all "cpu/xx" events use BPF by default?
>=20
> 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=20
implementation is easy and straightforward. We sure can extend the config=20
to other events. Before that, 'b' modifier should be good for these use
cases.=20

Thanks,
Song