From: Daniel Borkmann <daniel@iogearbox.net>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Song Liu <songliubraving@fb.com>,
netdev@vger.kernel.org, bpf@vger.kernel.org, kernel-team@fb.com,
ast@kernel.org, john.fastabend@gmail.com, kpsingh@chromium.org
Subject: Re: [PATCH bpf-next 1/2] bpf: introduce BPF_F_SHARE_PE for perf event array
Date: Tue, 29 Sep 2020 21:18:13 +0200 [thread overview]
Message-ID: <7c13d40b-fe79-ddbf-2a37-abae1b44de71@iogearbox.net> (raw)
In-Reply-To: <20200929190054.4a2chcuxuvicndtu@ast-mbp.dhcp.thefacebook.com>
On 9/29/20 9:00 PM, Alexei Starovoitov wrote:
> On Tue, Sep 29, 2020 at 04:02:10PM +0200, Daniel Borkmann wrote:
>>> +
>>> +/* Share perf_event among processes */
>>> + BPF_F_SHARE_PE = (1U << 11),
>>
>> nit but given UAPI: maybe name into something more self-descriptive
>> like BPF_F_SHAREABLE_EVENT ?
>
> I'm not happy with either name.
> It's not about sharing and not really about perf event.
> I think the current behavior of perf_event_array is unusual and surprising.
> Sadly we cannot fix it without breaking user space, so flag is needed.
> How about BPF_F_STICKY_OBJECTS or BPF_F_PRESERVE_OBJECTS
> or the same with s/OBJECTS/FILES/ ?
Sounds good to me, BPF_F_PRESERVE_OBJECTS or _ENTRIES seems reasonable.
>>> +static void perf_event_fd_array_map_free(struct bpf_map *map)
>>> +{
>>> + struct bpf_event_entry *ee;
>>> + struct bpf_array *array;
>>> + int i;
>>> +
>>> + if ((map->map_flags & BPF_F_SHARE_PE) == 0) {
>>> + fd_array_map_free(map);
>>> + return;
>>> + }
>>> +
>>> + array = container_of(map, struct bpf_array, map);
>>> + for (i = 0; i < array->map.max_entries; i++) {
>>> + ee = READ_ONCE(array->ptrs[i]);
>>> + if (ee)
>>> + fd_array_map_delete_elem(map, &i);
>>> + }
>>> + bpf_map_area_free(array);
>>
>> Why not simplify into:
>>
>> if (map->map_flags & BPF_F_SHAREABLE_EVENT)
>> bpf_fd_array_map_clear(map);
>> fd_array_map_free(map);
>
> +1
>
>>> +}
>>> +
>>> static void *prog_fd_array_get_ptr(struct bpf_map *map,
>>> struct file *map_file, int fd)
>>> {
>>> @@ -1134,6 +1158,9 @@ static void perf_event_fd_array_release(struct bpf_map *map,
>>> struct bpf_event_entry *ee;
>>> int i;
>
> add empty line pls.
>
>>> + if (map->map_flags & BPF_F_SHARE_PE)
>>> + return;
>>> +
next prev parent reply other threads:[~2020-09-29 19:18 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-29 8:47 [PATCH bpf-next 0/2] introduce BPF_F_SHARE_PE Song Liu
2020-09-29 8:47 ` [PATCH bpf-next 1/2] bpf: introduce BPF_F_SHARE_PE for perf event array Song Liu
2020-09-29 14:02 ` Daniel Borkmann
2020-09-29 19:00 ` Alexei Starovoitov
2020-09-29 19:18 ` Daniel Borkmann [this message]
2020-09-29 19:28 ` Alexei Starovoitov
2020-09-29 21:08 ` Song Liu
2020-09-29 8:47 ` [PATCH bpf-next 2/2] selftests/bpf: add tests for BPF_F_SHARE_PE Song Liu
2020-09-29 14:08 ` Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7c13d40b-fe79-ddbf-2a37-abae1b44de71@iogearbox.net \
--to=daniel@iogearbox.net \
--cc=alexei.starovoitov@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=john.fastabend@gmail.com \
--cc=kernel-team@fb.com \
--cc=kpsingh@chromium.org \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).