bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	David Miller <davem@davemloft.net>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map
Date: Tue, 22 Oct 2019 11:23:20 -0700	[thread overview]
Message-ID: <CAEf4BzapbVb=u_GPLSv-KEctwZz=K3FUb_B6p2HmWnqz06n4Rg@mail.gmail.com> (raw)
In-Reply-To: <87blu8odhf.fsf@toke.dk>

On Tue, Oct 22, 2019 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
> >>
> >> When pinning a map, store the pin path in struct bpf_map so it can be
> >> re-used later for un-pinning. This simplifies the later addition of per-map
> >> pin paths.
> >>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >>  tools/lib/bpf/libbpf.c |   19 ++++++++++---------
> >>  1 file changed, 10 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index cccfd9355134..b4fdd8ee3bbd 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -226,6 +226,7 @@ struct bpf_map {
> >>         void *priv;
> >>         bpf_map_clear_priv_t clear_priv;
> >>         enum libbpf_map_type libbpf_type;
> >> +       char *pin_path;
> >>  };
> >>
> >>  struct bpf_secdata {
> >> @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
> >>         if (err)
> >>                 goto err_close_new_fd;
> >>         free(map->name);
> >> +       zfree(&map->pin_path);
> >>
> >
> > While you are touching this function, can you please also fix error
> > handling in it? We should store -errno locally on error, before we
> > call close() which might change errno.
>
> Didn't actually look much at the surrounding function, TBH. I do expect
> that I will need to go poke into this for the follow-on "automatic reuse
> of pinned maps" series anyway. But sure, I can do a bit of cleanup in a
> standalone patch first :)
>
> >>         map->fd = new_fd;
> >>         map->name = new_name;
> >> @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
> >>                 return -errno;
> >>         }
> >>
> >> +       map->pin_path = strdup(path);
> >
> > if (!map->pin_path) {
> >     err = -errno;
> >     goto err_close_new_fd;
> > }
>
> Right.
>
> >>         pr_debug("pinned map '%s'\n", path);
> >>
> >>         return 0;
> >> @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path)
> >>  {
> >>         int err;
> >>
> >> +       if (!path)
> >> +               path = map->pin_path;
> >
> > This semantics is kind of weird. Given we now remember pin_path,
> > should we instead check that user-provided path is actually correct
> > and matches what we stored? Alternatively, bpf_map__unpin() w/o path
> > argument looks like a cleaner API.
>
> Yeah, I guess the function without a path argument would make the most
> sense. However, we can't really change the API of bpf_map__unpin()
> (unless you're proposing a symbol-versioned new version?). Dunno if it's
> worth it to include a new, somewhat oddly-named, function to achieve
> this? For the internal libbpf uses at least it's easy enough for the
> caller to just go bpf_map__unpin(map, map->pin_path), so I could also
> just drop this change? WDYT?

I'd probably do strcmp(map->pin_path, path), if path is specified.
This will support existing use cases, will allow NULL if we don't want
to bother remembering pin_path, will prevent weird use case of pinning
to one path, but unpinning another one.

Ideally, all this pinning will just be done declaratively and will
happen automatically, so users won't even have to know about this API
:)

>
> -Toke

  reply	other threads:[~2019-10-22 18:23 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-22 15:04 [PATCH bpf-next 0/3] libbpf: Support pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen
2019-10-22 15:04 ` [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map Toke Høiland-Jørgensen
2019-10-22 17:37   ` Andrii Nakryiko
2019-10-22 18:13     ` Toke Høiland-Jørgensen
2019-10-22 18:23       ` Andrii Nakryiko [this message]
2019-10-22 18:45         ` Toke Høiland-Jørgensen
2019-10-22 18:49           ` Andrii Nakryiko
2019-10-22 19:06             ` Toke Høiland-Jørgensen
2019-10-23  4:43               ` Andrii Nakryiko
2019-10-22 15:04 ` [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations Toke Høiland-Jørgensen
2019-10-22 18:20   ` Andrii Nakryiko
2019-10-22 18:57     ` Toke Høiland-Jørgensen
2019-10-23  4:40       ` Andrii Nakryiko
2019-10-23  8:53         ` Toke Høiland-Jørgensen
2019-10-23 12:30           ` Daniel Borkmann
2019-10-23 13:05             ` Toke Høiland-Jørgensen
2019-10-22 15:04 ` [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning Toke Høiland-Jørgensen
2019-10-22 18:28   ` Andrii Nakryiko
2019-10-22 19:04     ` Toke Høiland-Jørgensen
2019-10-23  4:58       ` Andrii Nakryiko
2019-10-23  8:58         ` Toke Høiland-Jørgensen

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='CAEf4BzapbVb=u_GPLSv-KEctwZz=K3FUb_B6p2HmWnqz06n4Rg@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=kafai@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=toke@redhat.com \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map' \
    /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

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