bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>, Rik van Riel <riel@surriel.com>,
	Johannes Weiner <hannes@cmpxchg.org>
Subject: Re: [PATCH v2 bpf-next 1/3] bpf: add mmap() support for BPF_MAP_TYPE_ARRAY
Date: Tue, 12 Nov 2019 14:38:17 -0800	[thread overview]
Message-ID: <20191112143817.0c0eb768@cakuba> (raw)
In-Reply-To: <CAEf4Bzbx0WvgX9uGF4U1HM41m6kfdvWHCeYBSBRnQhR3egGy5w@mail.gmail.com>

On Tue, 12 Nov 2019 14:03:50 -0800, Andrii Nakryiko wrote:
> On Tue, Nov 12, 2019 at 11:17 AM Jakub Kicinski wrote:
> > On Mon, 11 Nov 2019 18:06:42 -0800, Andrii Nakryiko wrote:  
> > > So let's say if sizeof(struct bpf_array) is 300, then I'd have to either:
> > >
> > > - somehow make sure that I allocate 4k (for data) + 300 (for struct
> > > bpf_array) in such a way that those 4k of data are 4k-aligned. Is
> > > there any way to do that?
> > > - assuming there isn't, then another way would be to allocate entire
> > > 4k page for struct bpf_array itself, but put it at the end of that
> > > page, so that 4k of data is 4k-aligned. While wasteful, the bigger
> > > problem is that pointer to bpf_array is not a pointer to allocated
> > > memory anymore, so we'd need to remember that and adjust address
> > > before calling vfree().
> > >
> > > Were you suggesting #2 as a solution? Or am I missing some other way to do this?  
> >
> > I am suggesting #2, that's the way to do it in the kernel.  
> So I'm concerned about this approach, because it feels like a bunch of
> unnecessarily wasted memory. While there is no way around doing
> round_up(PAGE_SIZE) for data itself, it certainly is not necessary to
> waste almost entire page for struct bpf_array. And given this is going
> to be used for BPF maps backing global variables, there most probably
> will be at least 3 (.data, .bss, .rodata) per each program, and could
> be more. Also, while on x86_64 page is 4k, on other architectures it
> can be up to 64KB, so this seems wasteful.

With the extra mutex and int you grew struct bpf_map from 192B to 256B,
that's for every map on the system, unconditionally, and array map has
an extra pointer even if it doesn't need it.

Increasing "wasted" space when an opt-in feature is selected doesn't
seem all that terrible to me, especially that the overhead of aligning
up map size to page size is already necessary.

> What's your concern exactly with the way it's implemented in this patch?

Judging by other threads we seem to care about performance of BPF
(rightly so). Doing an extra pointer deref for every static data access
seems like an obvious waste.

But then again, it's just an obvious suggestion, take it or leave it..

  reply	other threads:[~2019-11-12 22:38 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-09  8:06 [PATCH v2 bpf-next 0/3] Add support for memory-mapping BPF array maps Andrii Nakryiko
2019-11-09  8:06 ` [PATCH v2 bpf-next 1/3] bpf: add mmap() support for BPF_MAP_TYPE_ARRAY Andrii Nakryiko
2019-11-11 16:40   ` Song Liu
2019-11-11 18:37   ` Jakub Kicinski
2019-11-12  2:06     ` Andrii Nakryiko
2019-11-12 19:17       ` Jakub Kicinski
2019-11-12 22:03         ` Andrii Nakryiko
2019-11-12 22:38           ` Jakub Kicinski [this message]
2019-11-13  3:19             ` Andrii Nakryiko
2019-11-11 18:39   ` Jakub Kicinski
2019-11-12  2:01     ` Andrii Nakryiko
2019-11-09  8:06 ` [PATCH v2 bpf-next 2/3] libbpf: make global data internal arrays mmap()-able, if possible Andrii Nakryiko
2019-11-11 18:40   ` Jakub Kicinski
2019-11-12  2:11     ` Andrii Nakryiko
2019-11-09  8:06 ` [PATCH v2 bpf-next 3/3] selftests/bpf: add BPF_TYPE_MAP_ARRAY mmap() tests Andrii Nakryiko

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191112143817.0c0eb768@cakuba \
    --to=jakub.kicinski@netronome.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=hannes@cmpxchg.org \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=riel@surriel.com \


* 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).