mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command
@ 2018-07-29 15:51 Alexei Starovoitov
  2018-07-31  2:01 ` Joel Fernandes
  0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2018-07-29 15:51 UTC (permalink / raw)
  To: Daniel Colascione
  Cc: Joel Fernandes, LKML, Tim Murray, Network Development,
	Lorenzo Colitti, Chenbo Feng, Mathieu Desnoyers,
	Alexei Starovoitov, Daniel Borkmann

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

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

btw i don't have reliable internet access at the moment,
so pls excuse the delays.

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
@ 2018-07-16 20:23 Joel Fernandes
  2018-07-26 16:51 ` [PATCH v2] Add BPF_SYNCHRONIZE_MAPS " Daniel Colascione
  0 siblings, 1 reply; 7+ messages in thread
From: Joel Fernandes @ 2018-07-16 20:23 UTC (permalink / raw)
  To: Daniel Colascione
  Cc: Alexei Starovoitov, Lorenzo Colitti, Chenbo Feng,
	Mathieu Desnoyers, Joel Fernandes, Alexei Starovoitov, lkml,
	Tim Murray, Daniel Borkmann, netdev

On Mon, Jul 16, 2018 at 08:29:47AM -0700, Daniel Colascione wrote:
> On Sat, Jul 14, 2018 at 11:18 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Tue, Jul 10, 2018 at 08:40:19PM -0700, Alexei Starovoitov wrote:
> > [..]
> >> > The kernel program might do:
> >> >
> >> > =====
> >> > const int current_map_key = 1;
> >> > void *current_map = bpf_map_lookup_elem(outer_map, &current_map_key);
> >> >
> >> > int stats_key = 42;
> >> > uint64_t *stats_value = bpf_map_lookup_elem(current_map, &stats_key);
> >> > __sync_fetch_and_add(&stats_value, 1);
> >> > =====
> >> >
> >> > If a userspace does:
> >> >
> >> > 1. Write new fd to outer_map[1].
> >> > 2. Call BPF_SYNC_MAP_ACCESS.
> >> > 3. Start deleting everything in the old map.
> >> >
> >> > How can we guarantee that the __sync_fetch_and_add will not add to the
> >> > old map?
> >>
> >> without any changes to the kernel sys_membarrier will work.
> >> And that's what folks use already.
> >> BPF_SYNC_MAP_ACCESS implemented via synchronize_rcu() will work
> >> as well whether in the current implementation where rcu_lock/unlock
> >> is done outside of the program and in the future when
> >> rcu_lock/unlock are called by the program itself.
> >
> > Cool Alexei and Lorenzo, sounds great to me. Daniel want to send a follow up
> > patch with BPF_SYNC_MAP_ACCESS changes then?
> 
> Will do. Mind if I just mine this thread for the doc comment?

Do you mean the changelog? Yes I believe you could use the discussion in this
thread for the rationale as Alexei described.

thanks,

- Joel



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-07-31 22:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-29 15:51 [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command Alexei Starovoitov
2018-07-31  2:01 ` Joel Fernandes
2018-07-31  2:06   ` Joel Fernandes
2018-07-31  4:03     ` Y Song
2018-07-31 21:56       ` Joel Fernandes
2018-07-31 22:30         ` Daniel Borkmann
  -- strict thread matches above, loose matches on Subject: below --
2018-07-16 20:23 [RFC] Add BPF_SYNCHRONIZE " Joel Fernandes
2018-07-26 16:51 ` [PATCH v2] Add BPF_SYNCHRONIZE_MAPS " Daniel Colascione

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®