[v2,bpf-next,1/2] libbpf: add helpers for mmapping maps

Commit Message

Barret Rhoden Jan. 3, 2024, 6:53 p.m. UTC

bpf_map__mmap_size() was internal to bpftool.  Use that to make wrappers
for mmap and munmap.

Signed-off-by: Barret Rhoden <[e-mail]>
---
 tools/bpf/bpftool/gen.c  | 16 +++-------------
 tools/lib/bpf/libbpf.c   | 18 ++++++++++++++++++
 tools/lib/bpf/libbpf.h   |  6 ++++++
 tools/lib/bpf/libbpf.map |  4 ++++
 4 files changed, 31 insertions(+), 13 deletions(-)

Comments

Andrii Nakryiko Jan. 3, 2024, 7:42 p.m. UTC | #1

On Wed, Jan 3, 2024 at 10:54 AM Barret Rhoden <[e-mail]> wrote:
>
> bpf_map__mmap_size() was internal to bpftool.  Use that to make wrappers
> for mmap and munmap.
>
> Signed-off-by: Barret Rhoden <[e-mail]>
> ---
>  tools/bpf/bpftool/gen.c  | 16 +++-------------
>  tools/lib/bpf/libbpf.c   | 18 ++++++++++++++++++
>  tools/lib/bpf/libbpf.h   |  6 ++++++
>  tools/lib/bpf/libbpf.map |  4 ++++
>  4 files changed, 31 insertions(+), 13 deletions(-)
>

There is very little added value provided by these APIs, while adding
API obligations. bpf_map__mmap() assumes READ/WRITE access, why? What
if the user needs only read-only? And so on.

Please drop this patch, we don't need to expose these functions as
stable APIs. Definitely not bpf_map__mmap/munmap. bpf_map__mmap_size()
might be useful to hide various per-map type details, but we'll need
to discuss all this separately.

> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index ee3ce2b8000d..a328e960c141 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -453,16 +453,6 @@ static void print_hex(const char *data, int data_sz)
>         }
>  }
>
> -static size_t bpf_map_mmap_sz(const struct bpf_map *map)
> -{
> -       long page_sz = sysconf(_SC_PAGE_SIZE);
> -       size_t map_sz;
> -
> -       map_sz = (size_t)roundup(bpf_map__value_size(map), 8) * bpf_map__max_entries(map);

this is specifically ARRAY map's rules, it might differ for other map
types (e.g., RINGBUF has different logic)

> -       map_sz = roundup(map_sz, page_sz);
> -       return map_sz;
> -}
> -

[...]

Barret Rhoden Jan. 3, 2024, 7:45 p.m. UTC | #2

On 1/3/24 14:42, Andrii Nakryiko wrote:
> this is specifically ARRAY map's rules, it might differ for other map
> types (e.g., RINGBUF has different logic)

good to know.  will drop it.

Andrii Nakryiko Jan. 3, 2024, 8 p.m. UTC | #3

On Wed, Jan 3, 2024 at 11:45 AM Barret Rhoden <[e-mail]> wrote:
>
> On 1/3/24 14:42, Andrii Nakryiko wrote:
> > this is specifically ARRAY map's rules, it might differ for other map
> > types (e.g., RINGBUF has different logic)
>
> good to know.  will drop it.


please give other people a bit more time to review your current
revision, don't rapid-fire new revisions immediately after getting
feedback

Patch

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index ee3ce2b8000d..a328e960c141 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -453,16 +453,6 @@  static void print_hex(const char *data, int data_sz)
 	}
 }
 
-static size_t bpf_map_mmap_sz(const struct bpf_map *map)
-{
-	long page_sz = sysconf(_SC_PAGE_SIZE);
-	size_t map_sz;
-
-	map_sz = (size_t)roundup(bpf_map__value_size(map), 8) * bpf_map__max_entries(map);
-	map_sz = roundup(map_sz, page_sz);
-	return map_sz;
-}
-
 /* Emit type size asserts for all top-level fields in memory-mapped internal maps. */
 static void codegen_asserts(struct bpf_object *obj, const char *obj_name)
 {
@@ -641,7 +631,7 @@  static void codegen_destroy(struct bpf_object *obj, const char *obj_name)
 		if (bpf_map__is_internal(map) &&
 		    (bpf_map__map_flags(map) & BPF_F_MMAPABLE))
 			printf("\tskel_free_map_data(skel->%1$s, skel->maps.%1$s.initial_value, %2$zd);\n",
-			       ident, bpf_map_mmap_sz(map));
+			       ident, bpf_map__mmap_size(map));
 		codegen("\
 			\n\
 				skel_closenz(skel->maps.%1$s.map_fd);	    \n\
@@ -723,7 +713,7 @@  static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
 					goto cleanup;			    \n\
 				skel->maps.%1$s.initial_value = (__u64) (long) skel->%1$s;\n\
 			}						    \n\
-			", ident, bpf_map_mmap_sz(map));
+			", ident, bpf_map__mmap_size(map));
 	}
 	codegen("\
 		\n\
@@ -780,7 +770,7 @@  static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
 			if (!skel->%1$s)				    \n\
 				return -ENOMEM;				    \n\
 			",
-		       ident, bpf_map_mmap_sz(map), mmap_flags);
+		       ident, bpf_map__mmap_size(map), mmap_flags);
 	}
 	codegen("\
 		\n\
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ebcfb2147fbd..2dae6fabc0d1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9830,6 +9830,24 @@  void *bpf_map__initial_value(struct bpf_map *map, size_t *psize)
 	return map->mmaped;
 }
 
+size_t bpf_map__mmap_size(const struct bpf_map *map)
+{
+	return bpf_map_mmap_sz(bpf_map__value_size(map),
+			       bpf_map__max_entries(map));
+}
+
+void *bpf_map__mmap(const struct bpf_map *map)
+{
+	return mmap(NULL, bpf_map__mmap_size(map),
+		    PROT_READ | PROT_WRITE, MAP_SHARED,
+		    bpf_map__fd(map), 0);
+}
+
+int bpf_map__munmap(const struct bpf_map *map, void *addr)
+{
+	return munmap(addr, bpf_map__mmap_size(map));
+}
+
 bool bpf_map__is_internal(const struct bpf_map *map)
 {
 	return map->libbpf_type != LIBBPF_MAP_UNSPEC;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6cd9c501624f..148f4c783ca7 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -996,6 +996,12 @@  LIBBPF_API int bpf_map__set_map_extra(struct bpf_map *map, __u64 map_extra);
 LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map,
 					  const void *data, size_t size);
 LIBBPF_API void *bpf_map__initial_value(struct bpf_map *map, size_t *psize);
+/* get the mmappable size of the map */
+LIBBPF_API size_t bpf_map__mmap_size(const struct bpf_map *map);
+/* mmap the map */
+LIBBPF_API void *bpf_map__mmap(const struct bpf_map *map);
+/* munmap the map at addr */
+LIBBPF_API int bpf_map__munmap(const struct bpf_map *map, void *addr);
 
 /**
  * @brief **bpf_map__is_internal()** tells the caller whether or not the
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 91c5aef7dae7..9e44de4fbf39 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -411,4 +411,8 @@  LIBBPF_1.3.0 {
 } LIBBPF_1.2.0;
 
 LIBBPF_1.4.0 {
+	global:
+		bpf_map__mmap_size;
+		bpf_map__mmap;
+		bpf_map__munmap;
 } LIBBPF_1.3.0;