From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Colascione Subject: Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command Date: Sun, 29 Jul 2018 13:46:44 -0700 Message-ID: References: Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="00000000000064016905722971f3" Cc: Joel Fernandes , LKML , Tim Murray , Network Development , Lorenzo Colitti , Chenbo Feng , Mathieu Desnoyers , Alexei Starovoitov , Daniel Borkmann To: Alexei Starovoitov Return-path: Received: from mail-it0-f66.google.com ([209.85.214.66]:33483 "EHLO mail-it0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726813AbeG2WSb (ORCPT ); Sun, 29 Jul 2018 18:18:31 -0400 Received: by mail-it0-f66.google.com with SMTP id d16-v6so7495943itj.0 for ; Sun, 29 Jul 2018 13:46:45 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: --00000000000064016905722971f3 Content-Type: text/plain; charset="UTF-8" On Sun, Jul 29, 2018 at 8:51 AM, Alexei Starovoitov < alexei.starovoitov@gmail.com> wrote: > On Thu, Jul 26, 2018 at 7:51 PM, Daniel Colascione > wrote: > > BPF_SYNCHRONIZE_MAPS waits for the release of any references to a BPF > > map made by a BPF program that is running at the time the > > BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command is > > to provide a means for userspace to replace a BPF map with another, > > newer version, then ensure that no component is still using the "old" > > map before manipulating the "old" map in some way. > > > > Signed-off-by: Daniel Colascione > > --- > > include/uapi/linux/bpf.h | 9 +++++++++ > > kernel/bpf/syscall.c | 13 +++++++++++++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index b7db3261c62d..5b27e9117d3e 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -75,6 +75,14 @@ struct bpf_lpm_trie_key { > > __u8 data[0]; /* Arbitrary size */ > > }; > > > > +/* BPF_SYNCHRONIZE_MAPS waits for the release of any references to a > > + * BPF map made by a BPF program that is running at the time the > > + * BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command > > that doesn't sound right to me. > such command won't wait for the release of the references. > in case of map-in-map the program does not hold > the references to inner map (only to outer map). > Well, today that's the guarantee it provides. :-) I figured it'd be simpler to explain the guarantee this way. How about "waits for the release of any reference to a map obtained from another map"? > > > + * is to provide a means for userspace to replace a BPF map with > > + * another, newer version, then ensure that no component is still > > + * using the "old" map before manipulating the "old" map in some way. > > + */ > > that's correct, but the whole paragraph still reads too > 'generic' which will lead to confusion, > whereas the use case is map-in-map only. > I think bpf_sync_inner_map or > bpf_sync_map_in_map would be better > choices for the name. I worry that a name like that would be too specific. --00000000000064016905722971f3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On S= un, Jul 29, 2018 at 8:51 AM, Alexei Starovoitov <alexei.starovo= itov@gmail.com> wrote:
On Thu, Jul 26, 2018 at 7:51 PM, Daniel Colascione <dancol@google.com> wrote:
> BPF_SYNCHRONIZE_MAPS waits for the release of any references to a BPF<= br> > map made by a BPF program that is running at the time the
> BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command is=
> to provide a means for userspace to replace a BPF map with another, > newer version, then ensure that no component is still using the "= old"
> map before manipulating the "old" map in some way.
>
> Signed-off-by: Daniel Colascione <dancol@google.com>
> ---
>=C2=A0 include/uapi/linux/bpf.h |=C2=A0 9 +++++++++
>=C2=A0 kernel/bpf/syscall.c=C2=A0 =C2=A0 =C2=A0| 13 +++++++++++++
>=C2=A0 2 files changed, 22 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index b7db3261c62d..5b27e9117d3e 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__u8=C2=A0 =C2=A0 data[0];=C2=A0 =C2= =A0 =C2=A0 =C2=A0 /* Arbitrary size */
>=C2=A0 };
>
> +/* BPF_SYNCHRONIZE_MAPS waits for the release of any references to a<= br> > + * BPF map made by a BPF program that is running at the time the
> + * BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this comman= d

that doesn't sound right to me.
such command won't wait for the release of the references.
in case of map-in-map the program does not hold
the references to inner map (only to outer map).

<= /div>
Well, today that's the guarantee it provides. :-) I figured i= t'd be simpler to explain the guarantee this way.

<= div>How about "waits for the release of any reference to a map obtaine= d from another map"?
=C2=A0

> + * is to provide a means for userspace to replace a BPF map with
> + * another, newer version, then ensure that no component is still
> + * using the "old" map before manipulating the "old&qu= ot; map in some way.
> + */

that's correct, but the whole paragraph still reads too
'generic' which will lead to confusion,
whereas the use case is map-in-map only.
I think bpf_sync_inner_map or
bpf_sync_map_in_map would be better
choices for the name.

I worry that a name l= ike that would be too specific.
--00000000000064016905722971f3--