Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <CAADnVQL4twmGF3d26TgLsQJvnqLnM0F6hqL_42DSs95OuZ1y9A@mail.gmail.com>
References: <CAADnVQL4twmGF3d26TgLsQJvnqLnM0F6hqL_42DSs95OuZ1y9A@mail.gmail.com>
From:   Daniel Colascione <dancol@google.com>
Date:   Sun, 29 Jul 2018 13:46:44 -0700
Message-ID: <CAKOZuesQ6GdNTGDFsNi4o8LYzxLBtYZ=Cz4=aZbqqCNia+QFnQ@mail.gmail.com>
Subject: Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Joel Fernandes <joelaf@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Tim Murray <timmurray@google.com>,
        Network Development <netdev@vger.kernel.org>,
        Lorenzo Colitti <lorenzo@google.com>,
        Chenbo Feng <fengc@google.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Alexei Starovoitov <ast@fb.com>,
        Daniel Borkmann <daniel@iogearbox.net>
Content-Type: multipart/alternative; boundary="00000000000064016905722971f3"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--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 <dancol@google.com>
> 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 <dancol@google.com>
> > ---
> >  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

<div dir=3D"ltr"><div class=3D"gmail_extra"><div class=3D"gmail_quote">On S=
un, Jul 29, 2018 at 8:51 AM, Alexei Starovoitov <span dir=3D"ltr">&lt;<a hr=
ef=3D"mailto:alexei.starovoitov@gmail.com" target=3D"_blank">alexei.starovo=
itov@gmail.com</a>&gt;</span> wrote:<br><blockquote class=3D"gmail_quote" s=
tyle=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><spa=
n class=3D"">On Thu, Jul 26, 2018 at 7:51 PM, Daniel Colascione &lt;<a href=
=3D"mailto:dancol@google.com">dancol@google.com</a>&gt; wrote:<br>
&gt; BPF_SYNCHRONIZE_MAPS waits for the release of any references to a BPF<=
br>
&gt; map made by a BPF program that is running at the time the<br>
&gt; BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command is=
<br>
&gt; to provide a means for userspace to replace a BPF map with another,<br=
>
&gt; newer version, then ensure that no component is still using the &quot;=
old&quot;<br>
&gt; map before manipulating the &quot;old&quot; map in some way.<br>
&gt;<br>
&gt; Signed-off-by: Daniel Colascione &lt;<a href=3D"mailto:dancol@google.c=
om">dancol@google.com</a>&gt;<br>
&gt; ---<br>
&gt;=C2=A0 include/uapi/linux/bpf.h |=C2=A0 9 +++++++++<br>
&gt;=C2=A0 kernel/bpf/syscall.c=C2=A0 =C2=A0 =C2=A0| 13 +++++++++++++<br>
&gt;=C2=A0 2 files changed, 22 insertions(+)<br>
&gt;<br>
&gt; diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h<br>
&gt; index b7db3261c62d..5b27e9117d3e 100644<br>
&gt; --- a/include/uapi/linux/bpf.h<br>
&gt; +++ b/include/uapi/linux/bpf.h<br>
&gt; @@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {<br>
&gt;=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 */<br>
&gt;=C2=A0 };<br>
&gt;<br>
&gt; +/* BPF_SYNCHRONIZE_MAPS waits for the release of any references to a<=
br>
&gt; + * BPF map made by a BPF program that is running at the time the<br>
&gt; + * BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this comman=
d<br>
<br>
</span>that doesn&#39;t sound right to me.<br>
such command won&#39;t wait for the release of the references.<br>
in case of map-in-map the program does not hold<br>
the references to inner map (only to outer map).<br></blockquote><div><br><=
/div><div>Well, today that&#39;s the guarantee it provides. :-) I figured i=
t&#39;d be simpler to explain the guarantee this way.</div><div><br></div><=
div>How about &quot;waits for the release of any reference to a map obtaine=
d from another map&quot;?</div><div>=C2=A0</div><blockquote class=3D"gmail_=
quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1=
ex">
<span class=3D""><br>
&gt; + * is to provide a means for userspace to replace a BPF map with<br>
&gt; + * another, newer version, then ensure that no component is still<br>
&gt; + * using the &quot;old&quot; map before manipulating the &quot;old&qu=
ot; map in some way.<br>
&gt; + */<br>
<br>
</span>that&#39;s correct, but the whole paragraph still reads too<br>
&#39;generic&#39; which will lead to confusion,<br>
whereas the use case is map-in-map only.<br>
I think bpf_sync_inner_map or<br>
bpf_sync_map_in_map would be better<br>
choices for the name.</blockquote><div><br></div><div>I worry that a name l=
ike that would be too specific.</div></div></div></div>

--00000000000064016905722971f3--