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>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "acme@kernel.org" <acme@kernel.org>,
        "acme@redhat.com" <acme@redhat.com>,
        "namhyung@kernel.org" <namhyung@kernel.org>,
        "jolsa@kernel.org" <jolsa@kernel.org>
Subject: Re: [PATCH 1/2] perf util: move bperf definitions to a libperf header
Thread-Topic: [PATCH 1/2] perf util: move bperf definitions to a libperf
 header
Thread-Index: AQHXKCCISAhD2jKzCkmPS3MX7a1o1qqnkAwAgAAixAA=
Date:   Tue, 6 Apr 2021 16:26:03 +0000
Message-ID: <FAF2CD39-4FDB-4E6D-96AB-E246030DF8CC@fb.com>
References: <20210403002938.390878-1-song@kernel.org> <YGxucIgiq+ogX687@krava>
In-Reply-To: <YGxucIgiq+ogX687@krava>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <FC734C981216094986F246948E423FDC@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 5c286294-8a01-4dc3-fe37-08d8f918ac62
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Apr 2021 16:26:03.8106
 (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: aGpFX+NpfVRif8dFNmc5jJaKpIpAr7Kya4MC0pptNIWM2HinCUgjMf8tmbJjMTAh15tbPYXUpOTqo4RZYlQdEw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB3094
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: vDln0rgkCpqgrrdUFBRLSs_cnUHruum8
X-Proofpoint-ORIG-GUID: vDln0rgkCpqgrrdUFBRLSs_cnUHruum8
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761
 definitions=2021-04-06_04:2021-04-06,2021-04-06 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 lowpriorityscore=0
 mlxscore=0 phishscore=0 malwarescore=0 spamscore=0 clxscore=1015
 priorityscore=1501 suspectscore=0 impostorscore=0 mlxlogscore=999
 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104030000 definitions=main-2104060109
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Apr 6, 2021, at 7:21 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>=20
> On Fri, Apr 02, 2021 at 05:29:37PM -0700, Song Liu wrote:
>> By following the same protocol, other tools can share hardware PMCs with
>> perf. Move perf_event_attr_map_entry and BPERF_DEFAULT_ATTR_MAP_PATH to
>> bperf.h for other tools to use.
>=20
> hi,
> so is this necessary for some other tool now?

We have monitoring tools do perf_event_open(). I would like to migrate thes=
e
to bperf.=20

>=20
>>=20
>> Also add bperf_attr_map_compatible() to check whether existing attr_map
>> is compatible with current perf binary.
>=20
> please separate this change

Will do.=20

>=20
>>=20
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> tools/lib/perf/include/perf/bperf.h | 29 +++++++++++++++++++
>> tools/perf/util/bpf_counter.c       | 44 ++++++++++++++---------------
>> 2 files changed, 50 insertions(+), 23 deletions(-)
>> create mode 100644 tools/lib/perf/include/perf/bperf.h
>>=20
>> diff --git a/tools/lib/perf/include/perf/bperf.h b/tools/lib/perf/includ=
e/perf/bperf.h
>> new file mode 100644
>> index 0000000000000..02b2fd5e50c75
>> --- /dev/null
>> +++ b/tools/lib/perf/include/perf/bperf.h
>=20
> I wonder we want to call it bpf_perf.h to be more generic?
> or best just bpf.h ... but that might give us some conflict
> headache in future ;-)

I would rather avoid bpf.h... I am ok with bpf_perf.h or bperf.h

>=20
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
>> +#ifndef __LIBPERF_BPERF_H
>> +#define __LIBPERF_BPERF_H
>> +
>> +/*
>> + * bperf uses a hashmap, the attr_map, to track all the leader programs=
.
>> + * The hashmap is pinned in bpffs. flock() on this file is used to ensu=
re
>> + * no concurrent access to the attr_map.  The key of attr_map is struct
>> + * perf_event_attr, and the value is struct perf_event_attr_map_entry.
>> + *
>> + * struct perf_event_attr_map_entry contains two __u32 IDs, bpf_link of=
 the
>> + * leader prog, and the diff_map. Each perf-stat session holds a refere=
nce
>> + * to the bpf_link to make sure the leader prog is attached to sched_sw=
itch
>> + * tracepoint.
>> + *
>> + * Since the hashmap only contains IDs of the bpf_link and diff_map, it
>> + * does not hold any references to the leader program. Once all perf-st=
at
>> + * sessions of these events exit, the leader prog, its maps, and the
>> + * perf_events will be freed.
>> + */
>> +struct perf_event_attr_map_entry {
>> +	__u32 link_id;
>> +	__u32 diff_map_id;
>> +};
>=20
> this header file should be self contained,
> so you need __u32 definitions

Will add.=20

>=20
>=20
>> +
>> +/* pin the map at sysfs__mountpoint()/BPERF_DEFAULT_ATTR_MAP_PATH */
>> +#define BPERF_DEFAULT_ATTR_MAP_PATH "fs/bpf/perf_attr_map"
>=20
> if we are going to expose this, I think we should expose just
> "perf_attr_map" ... without the 'fs/bpf' part, because that
> could be mounted anywhere

Will fix this.=20

Thanks,
Song