From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joel Fernandes Subject: Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command Date: Mon, 30 Jul 2018 19:01:22 -0700 Message-ID: <20180731020122.GA22311@joelaf.mtv.corp.google.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Daniel Colascione , 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-pf1-f193.google.com ([209.85.210.193]:39087 "EHLO mail-pf1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727045AbeGaDjO (ORCPT ); Mon, 30 Jul 2018 23:39:14 -0400 Received: by mail-pf1-f193.google.com with SMTP id j8-v6so5353988pff.6 for ; Mon, 30 Jul 2018 19:01:24 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Jul 29, 2018 at 06:51:18PM +0300, Alexei Starovoitov 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). I didn't follow this completely. The userspace program is using the inner map per your description of the algorithm for using map-in-map to solve the race conditions that this patch is trying to address: If you don't mind, I copy-pasted it below from your netdev post: if you use map-in-map you don't need extra boolean map. 0. bpf prog can do inner_map = lookup(map_in_map, key=0); lookup(inner_map, your_real_key); 1. user space writes into map_in_map[0] <- FD of new map 2. some cpus are using old inner map and some a new 3. user space does sys_membarrier(CMD_GLOBAL) which will do synchronize_sched() which in CONFIG_PREEMPT_NONE=y servers is the same as synchronize_rcu() which will guarantee that progs finished. 4. scan old inner map In step 2, as you mentioned there are CPUs using different inner maps. So could you clarify how the synchronize_rcu mechanism will even work if you're now saying "program does not hold references to the inner maps"? Thanks! - Joel