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 2/2] perf-stat: introduce config stat.bpf-counter-events
Thread-Topic: [PATCH 2/2] perf-stat: introduce config stat.bpf-counter-events
Thread-Index: AQHXKCCIduhTbkO6e0WfWNZaS0HfRqqnkBmAgAB8PYA=
Date:   Tue, 6 Apr 2021 21:46:28 +0000
Message-ID: <8970DB18-42B1-44A0-84E9-5C317245F453@fb.com>
References: <20210403002938.390878-1-song@kernel.org>
 <20210403002938.390878-2-song@kernel.org> <YGxue1aHQUFj+UaG@krava>
In-Reply-To: <YGxue1aHQUFj+UaG@krava>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <19DBD3D4FF3B5B4FAA6408FD03BE03F3@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 01e125e5-77d4-4c8f-a5ca-08d8f9456f46
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Apr 2021 21:46:28.6056
 (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: o1R573vlG4eK9c/zQtwZQAKe3//vVzjggsvpFH4QH+eJ3PQgMg5NrS7yrK5JxOJklyOCRhUv06kWwazea8lAQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB3464
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: uw7D3zb39UyrJtrKLj2AvPoiwkfV1Q0v
X-Proofpoint-ORIG-GUID: uw7D3zb39UyrJtrKLj2AvPoiwkfV1Q0v
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761
 definitions=2021-04-06_07:2021-04-06,2021-04-06 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 mlxscore=0 suspectscore=0
 phishscore=0 lowpriorityscore=0 spamscore=0 adultscore=0 mlxlogscore=999
 clxscore=1015 malwarescore=0 impostorscore=0 priorityscore=1501
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104060000 definitions=main-2104060146
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:38PM -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.
>>=20
>> This also enables mixed BPF event and regular event in the same sesssion=
.
>> For example:
>>=20
>>   perf config stat.bpf-counter-events=3Dinstructions
>>   perf stat -e instructions,cs
>=20
> hum, so this will effectively allow to mix 'bpf-shared' counters
> with normals ones.. I don't think we're ready for that ;-)

I think we are ready. :) all bpf_counter stuff is within evsel, so mixing=20
them doesn't need much work.=20

>=20
>>=20
>> The second command will use BPF for "instructions" but not "cs".
>>=20
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> tools/perf/Documentation/perf-stat.txt |  2 ++
>> tools/perf/builtin-stat.c              | 41 ++++++++++++++++----------
>> tools/perf/util/bpf_counter.c          | 11 +++++++
>> tools/perf/util/config.c               | 32 ++++++++++++++++++++
>> tools/perf/util/evsel.c                |  2 ++
>> tools/perf/util/evsel.h                |  1 +
>> tools/perf/util/target.h               |  5 ----
>> 7 files changed, 74 insertions(+), 20 deletions(-)
>>=20
>> diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documen=
tation/perf-stat.txt
>> index 744211fa8c186..6d4733eaac170 100644
>> --- a/tools/perf/Documentation/perf-stat.txt
>> +++ b/tools/perf/Documentation/perf-stat.txt
>> @@ -97,6 +97,8 @@ report::
>> 	Use BPF programs to aggregate readings from perf_events.  This
>> 	allows multiple perf-stat sessions that are counting the same metric (c=
ycles,
>> 	instructions, etc.) to share hardware counters.
>> +	To use BPF programs on common hardware events by default, use
>> +	"perf config stat.bpf-counter-events=3D<list_of_events>".
>>=20
>> --bpf-attr-map::
>> 	With option "--bpf-counters", different perf-stat sessions share
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 4bb48c6b66980..5adfa708ffe68 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -423,17 +423,28 @@ static int read_affinity_counters(struct timespec =
*rs)
>> 	return 0;
>> }
>>=20
>> +/*
>> + * Returns:
>> + *     0   if all events use BPF;
>> + *     1   if some events do NOT use BPF;
>> + *     < 0 on errors;
>> + */
>> static int read_bpf_map_counters(void)
>> {
>> +	bool has_none_bpf_events =3D false;
>> 	struct evsel *counter;
>> 	int err;
>>=20
>> 	evlist__for_each_entry(evsel_list, counter) {
>> +		if (!counter->bpf_counter_ops) {
>> +			has_none_bpf_events =3D true;
>> +			continue;
>> +		}
>> 		err =3D bpf_counter__read(counter);
>> 		if (err)
>> 			return err;
>> 	}
>> -	return 0;
>> +	return has_none_bpf_events ? 1 : 0;
>> }
>>=20
>> static void read_counters(struct timespec *rs)
>> @@ -442,9 +453,10 @@ static void read_counters(struct timespec *rs)
>> 	int err;
>>=20
>> 	if (!stat_config.stop_read_counter) {
>> -		if (target__has_bpf(&target))
>> -			err =3D read_bpf_map_counters();
>> -		else
>> +		err =3D read_bpf_map_counters();
>> +		if (err < 0)
>> +			return;
>> +		if (err)
>> 			err =3D read_affinity_counters(rs);
>=20
> so read_affinity_counters will read also 'bpf-shared' counters no?
> as long as it was separated, I did not see a problem, now we have
> counters that either have bpf ops set or have not
>=20
> it'd be great to do some generic separation.. I was thinking to move
> bpf_counter_ops into some generic counter ops and we would just fill
> in the proper ops for the counter.. buuut the affinity readings are
> not compatible with what we are doing in bperf_read and the profiler
> bpf read
>=20
> so I think the solution will be just to skip those events in
> read_affinity_counters and all the other code, and have some
> helper like:
>=20
>   bool evsel__is_bpf(evsel)
>=20
> so it's clear why it's skipped

Yes, this will be better! Current version does have the problem of=20
extra read in read_affinity_counters(). Will fix this.=20

Thanks,
Song=