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:     Namhyung Kim <namhyung@kernel.org>
CC:     linux-kernel <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>,
        Jiri Olsa <jolsa@kernel.org>
Subject: Re: [PATCH v2 1/3] perf-stat: introduce bperf, share hardware PMCs
 with BPF
Thread-Topic: [PATCH v2 1/3] perf-stat: introduce bperf, share hardware PMCs
 with BPF
Thread-Index: AQHXGqoOnn6PYztrGkqjWdJT1tFfMKqJQQYAgAAYjYCAAGw9gIAAObOA
Date:   Thu, 18 Mar 2021 17:16:24 +0000
Message-ID: <D8EF49AA-D49D-40B6-B1BD-0182E08CBD2B@fb.com>
References: <20210316211837.910506-1-songliubraving@fb.com>
 <20210316211837.910506-2-songliubraving@fb.com>
 <CAM9d7cgqqj5_Koe_dxBxsEXqLFS6d8VOoSwkiVaEO79V55JcxQ@mail.gmail.com>
 <5816B9E5-1664-4D66-9128-EFC2EEE089B2@fb.com>
 <CAM9d7cg7PyKjdWFmv_0B+sz4TDciGGyNkRTC1p+DoXBOn-xXRg@mail.gmail.com>
In-Reply-To: <CAM9d7cg7PyKjdWFmv_0B+sz4TDciGGyNkRTC1p+DoXBOn-xXRg@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4A4ABE4BA430444D98E885ACCC2A039A@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 2a5392ae-0d86-48be-daab-08d8ea318ed0
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Mar 2021 17:16:24.2175
 (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: hrH3ExKeokJBcMd6X9qQ6c1IK88FysWRpjiSYiPsgLq8MUNB1tq7KdEE79DBn1whfS8O1/AgBFJ6yzIxJbx4CA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR15MB4520
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761
 definitions=2021-03-18_09:2021-03-17,2021-03-18 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 malwarescore=0
 bulkscore=0 lowpriorityscore=0 mlxscore=0 phishscore=0 suspectscore=0
 spamscore=0 priorityscore=1501 mlxlogscore=999 impostorscore=0
 clxscore=1015 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2103180121
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Mar 18, 2021, at 6:49 AM, Namhyung Kim <namhyung@kernel.org> wrote:
>=20
> On Thu, Mar 18, 2021 at 4:22 PM Song Liu <songliubraving@fb.com> wrote:
>>=20
>>=20
>>=20
>>> On Mar 17, 2021, at 10:54 PM, Namhyung Kim <namhyung@kernel.org> wrote:
>>>=20
>>=20
>> [...]
>>=20
>>>> +
>>>> +static int bperf_reload_leader_program(struct evsel *evsel, int attr_=
map_fd,
>>>> +                                      struct perf_event_attr_map_entr=
y *entry)
>>>> +{
>>>> +       struct bperf_leader_bpf *skel =3D bperf_leader_bpf__open();
>>>> +       int link_fd, diff_map_fd, err;
>>>> +       struct bpf_link *link =3D NULL;
>>>> +
>>>> +       if (!skel) {
>>>> +               pr_err("Failed to open leader skeleton\n");
>>>> +               return -1;
>>>> +       }
>>>> +
>>>> +       bpf_map__resize(skel->maps.events, libbpf_num_possible_cpus())=
;
>>>> +       err =3D bperf_leader_bpf__load(skel);
>>>> +       if (err) {
>>>> +               pr_err("Failed to load leader skeleton\n");
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       err =3D -1;
>>>> +       link =3D bpf_program__attach(skel->progs.on_switch);
>>>> +       if (!link) {
>>>> +               pr_err("Failed to attach leader program\n");
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       link_fd =3D bpf_link__fd(link);
>>>> +       diff_map_fd =3D bpf_map__fd(skel->maps.diff_readings);
>>>> +       entry->link_id =3D bpf_link_get_id(link_fd);
>>>> +       entry->diff_map_id =3D bpf_map_get_id(diff_map_fd);
>>>> +       err =3D bpf_map_update_elem(attr_map_fd, &evsel->core.attr, en=
try, BPF_ANY);
>>>> +       assert(err =3D=3D 0);
>>>> +
>>>> +       evsel->bperf_leader_link_fd =3D bpf_link_get_fd_by_id(entry->l=
ink_id);
>>>> +       assert(evsel->bperf_leader_link_fd >=3D 0);
>>>=20
>>> Isn't it the same as link_fd?
>>=20
>> This is a different fd on the same link.
>=20
> Ok
>=20
>>=20
>>>=20
>>>> +
>>>> +       /*
>>>> +        * save leader_skel for install_pe, which is called within
>>>> +        * following evsel__open_per_cpu call
>>>> +        */
>>>> +       evsel->leader_skel =3D skel;
>>>> +       evsel__open_per_cpu(evsel, all_cpu_map, -1);
>>>> +
>>>> +out:
>>>> +       bperf_leader_bpf__destroy(skel);
>>>> +       bpf_link__destroy(link);
>>>=20
>>> Why do we destroy it?  Is it because we get an another reference?
>>=20
>> Yes. We only need evsel->bperf_leader_link_fd to keep the whole
>> skeleton attached.
>>=20
>> When multiple perf-stat sessions are sharing the leader skeleton,
>> only the first one loads the leader skeleton, by calling
>> bperf_reload_leader_program(). Other sessions simply hold a fd to
>> the bpf_link. More explanation in bperf__load() below.
>=20
> Ok.
>=20
>>=20
>>=20
>>>=20
>>>> +       return err;
>>>> +}
>>>> +
>>>> +static int bperf__load(struct evsel *evsel, struct target *target)
>>>> +{
>>>> +       struct perf_event_attr_map_entry entry =3D {0xffffffff, 0xffff=
ffff};
>>>> +       int attr_map_fd, diff_map_fd =3D -1, err;
>>>> +       enum bperf_filter_type filter_type;
>>>> +       __u32 filter_entry_cnt, i;
>>>> +
>>>> +       if (bperf_check_target(evsel, target, &filter_type, &filter_en=
try_cnt))
>>>> +               return -1;
>>>> +
>>>> +       if (!all_cpu_map) {
>>>> +               all_cpu_map =3D perf_cpu_map__new(NULL);
>>>> +               if (!all_cpu_map)
>>>> +                       return -1;
>>>> +       }
>>>> +
>>>> +       evsel->bperf_leader_prog_fd =3D -1;
>>>> +       evsel->bperf_leader_link_fd =3D -1;
>>>> +
>>>> +       /*
>>>> +        * Step 1: hold a fd on the leader program and the bpf_link, i=
f
>>>> +        * the program is not already gone, reload the program.
>>>> +        * Use flock() to ensure exclusive access to the perf_event_at=
tr
>>>> +        * map.
>>>> +        */
>>>> +       attr_map_fd =3D bperf_lock_attr_map(target);
>>>> +       if (attr_map_fd < 0) {
>>>> +               pr_err("Failed to lock perf_event_attr map\n");
>>>> +               return -1;
>>>> +       }
>>>> +
>>>> +       err =3D bpf_map_lookup_elem(attr_map_fd, &evsel->core.attr, &e=
ntry);
>>>> +       if (err) {
>>>> +               err =3D bpf_map_update_elem(attr_map_fd, &evsel->core.=
attr, &entry, BPF_ANY);
>>>> +               if (err)
>>>> +                       goto out;
>>>> +       }
>>>> +
>>>> +       evsel->bperf_leader_link_fd =3D bpf_link_get_fd_by_id(entry.li=
nk_id);
>>>> +       if (evsel->bperf_leader_link_fd < 0 &&
>>>> +           bperf_reload_leader_program(evsel, attr_map_fd, &entry))
>>>> +               goto out;
>>=20
>> Continue with previous explanation. In bperf_reload_leader_program(),
>> we open another reference to the link, and destroy the skeleton. This
>> brings the code to the same state as evsel->bperf_leader_link_fd >=3D
>> condition above.
>=20
> Thanks for the explanation.
>=20
>>=20
>>>> +
>>>> +       /*
>>>> +        * The bpf_link holds reference to the leader program, and the
>>>> +        * leader program holds reference to the maps. Therefore, if
>>>> +        * link_id is valid, diff_map_id should also be valid.
>>>> +        */
>>>> +       evsel->bperf_leader_prog_fd =3D bpf_prog_get_fd_by_id(
>>>> +               bpf_link_get_prog_id(evsel->bperf_leader_link_fd));
>>>> +       assert(evsel->bperf_leader_prog_fd >=3D 0);
>>>> +
>>>> +       diff_map_fd =3D bpf_map_get_fd_by_id(entry.diff_map_id);
>>>> +       assert(diff_map_fd >=3D 0);
>>>> +
>>=20
>> [...]
>>=20
>>>> +static int bperf__read(struct evsel *evsel)
>>>> +{
>>>> +       struct bperf_follower_bpf *skel =3D evsel->follower_skel;
>>>> +       __u32 num_cpu_bpf =3D cpu__max_cpu();
>>>> +       struct bpf_perf_event_value values[num_cpu_bpf];
>>>> +       int reading_map_fd, err =3D 0;
>>>> +       __u32 i, j, num_cpu;
>>>> +
>>>> +       bperf_sync_counters(evsel);
>>>> +       reading_map_fd =3D bpf_map__fd(skel->maps.accum_readings);
>>>> +
>>>> +       for (i =3D 0; i < bpf_map__max_entries(skel->maps.accum_readin=
gs); i++) {
>>>> +               __u32 cpu;
>>>> +
>>>> +               err =3D bpf_map_lookup_elem(reading_map_fd, &i, values=
);
>>>> +               if (err)
>>>> +                       goto out;
>>>> +               switch (evsel->follower_skel->bss->type) {
>>>> +               case BPERF_FILTER_GLOBAL:
>>>> +                       assert(i =3D=3D 0);
>>>> +
>>>> +                       num_cpu =3D all_cpu_map->nr;
>>>> +                       for (j =3D 0; j < num_cpu; j++) {
>>>> +                               cpu =3D all_cpu_map->map[j];
>>>> +                               perf_counts(evsel->counts, cpu, 0)->va=
l =3D values[cpu].counter;
>>>> +                               perf_counts(evsel->counts, cpu, 0)->en=
a =3D values[cpu].enabled;
>>>> +                               perf_counts(evsel->counts, cpu, 0)->ru=
n =3D values[cpu].running;
>>>=20
>>> I'm confused with this.  Does the accum_readings map contain values
>>> for all cpus?  IIUC it has only a single entry but you access it for ea=
ch cpu.
>>> What am I missing?
>>=20
>> accumulated_reading is a percpu array. In this case, each cpu has its ow=
n
>> bpf_perf_event_value with index 0. The BPF program could only access the
>> data on current cpu. When reading from use space, we get #-of-cpus entri=
es
>> for index 0.
>>=20
>> Does this make sense?
>=20
> Yep, I didn't know it returns all values when reading from user space.  T=
hen
> I think per cpu event doesn't have many entries too.  Like the global cas=
e
> it can simply put the value with key 0, no?

Current per cpu event use same logic as per task events, so we do have mult=
iple
entries. I think it is possible to modify the logic to use one entry for pe=
r
cpu events.=20

Thanks,
Song