From: Joe Burton <jevburton@google.com> To: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Joe Burton <jevburton.kernel@gmail.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>, John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, Petar Penkov <ppenkov@google.com>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, netdev@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [RFC PATCH v2 00/13] Introduce BPF map tracing capability Date: Wed, 6 Oct 2021 14:05:55 -0700 [thread overview] Message-ID: <CAL0ypaCwmGkQ0VK3nvfimHsO+OhBZb8cew-5c1gjZoZVZb1bBg@mail.gmail.com> (raw) In-Reply-To: <20211006164143.fuvbzxjca7cxe5ur@ast-mbp.dhcp.thefacebook.com> > Just to make sure we're on the same patch I'm proposing something like > the patch below... The proposed patch seems reasonable overall: + eliminates a lot of boilerplate + enables map update filtering + minimal perf cost when not tracing maps + avoids adding complexity to verifier - requires touching every map type's implementation - tracing one map implies tracing all maps I can rev this RFC with hooks inside the common map types' update() and delete() methods. > Especially for local storage... doing tracing from bpf program itself > seems to make the most sense. I'm a little unclear on how this should work. There's no off-the-shelf solution that can do this for us, right? In particular I think we're looking for an interface like this: /* This is a BPF program */ int my_prog(struct bpf_sock *sk) { struct MyValue *v = bpf_sk_storage_get(&my_map, sk, ...); ... bpf_sk_storage_trace(&my_map, sk, v); return 0; } I.e. we need some way of triggering a tracing hook from a BPF program. For non-local storage maps we can achieve this with a bpf_map_update_elem(). For local storage I suspect we need something new. Assuming there's no off-the-shelf hook that I'm missing, we can do some brainstorming internally and come back with a proposal or two. On Wed, Oct 6, 2021 at 9:41 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Oct 05, 2021 at 02:47:34PM -0700, Joe Burton wrote: > > > It's a neat idea to user verifier powers for this job, > > > but I wonder why simple tracepoint in map ops was not used instead? > > > > My concern with tracepoints is that they execute for all map updates, > > not for a particular map. Ideally performing an upgrade of program X > > should not affect the performance characteristics of program Y. > > Right, but single 'if (map == map_ptr_being_traced)' > won't really affect update() speed of maps. > For hash maps the update/delete are heavy operations with a bunch of > checks and spinlocks. > Just to make sure we're on the same patch I'm proposing something like > the patch below... > > > If n programs are opted into this model, then upgrading any of them > > affects the performance characteristics of every other. There's also > > the (very remote) possibility of multiple simultaneous upgrades tracing > > map updates at the same time, causing a greater performance hit. > > Also consider that the verifier fixup of update/delete in the code > is permanent whereas attaching fentry or fmod_ret to a nop function is temporary. > Once tracing of the map is no longer necessary that fentry program > will be detached and overhead will go back to zero. > Which is not the case for 'fixup' approach. > > With fmod_ret the tracing program might be the enforcing program. > It could be used to disallow certain map access in a generic way. > > > > I don't think the "solution" for lookup operation is worth pursuing. > > > The bpf prog that needs this map tracing is completely in your control. > > > So just don't do writes after lookup. > > > > I eventually want to support apps that use local storage. Those APIs > > generally only allow updates via a pointer. E.g. bpf_sk_storage_get() > > only allows updates via the returned pointer and via > > bpf_sk_storage_delete(). > > > > Since I eventually have to solve this problem to handle local storage, > > then it seems worth solving it for normal maps as well. They seem > > like isomorphic problems. > > Especially for local storage... doing tracing from bpf program itself > seems to make the most sense. > > From c7b6ec4488ee50ebbca61c22c6837fd6fe7007bf Mon Sep 17 00:00:00 2001 > From: Alexei Starovoitov <ast@kernel.org> > Date: Wed, 6 Oct 2021 09:30:21 -0700 > Subject: [PATCH] bpf: trace array map update > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > kernel/bpf/arraymap.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > index 5e1ccfae916b..89f853b1a217 100644 > --- a/kernel/bpf/arraymap.c > +++ b/kernel/bpf/arraymap.c > @@ -293,6 +293,13 @@ static void check_and_free_timer_in_array(struct bpf_array *arr, void *val) > bpf_timer_cancel_and_free(val + arr->map.timer_off); > } > > +noinline int bpf_array_map_trace_update(struct bpf_map *map, void *key, > + void *value, u64 map_flags) > +{ > + return 0; > +} > +ALLOW_ERROR_INJECTION(bpf_array_map_trace_update, ERRNO); > + > /* Called from syscall or from eBPF program */ > static int array_map_update_elem(struct bpf_map *map, void *key, void *value, > u64 map_flags) > @@ -300,6 +307,7 @@ static int array_map_update_elem(struct bpf_map *map, void *key, void *value, > struct bpf_array *array = container_of(map, struct bpf_array, map); > u32 index = *(u32 *)key; > char *val; > + int err; > > if (unlikely((map_flags & ~BPF_F_LOCK) > BPF_EXIST)) > /* unknown flags */ > @@ -317,6 +325,9 @@ static int array_map_update_elem(struct bpf_map *map, void *key, void *value, > !map_value_has_spin_lock(map))) > return -EINVAL; > > + if (unlikely(err = bpf_array_map_trace_update(map, key, value, map_flags))) > + return err; > + > if (array->map.map_type == BPF_MAP_TYPE_PERCPU_ARRAY) { > memcpy(this_cpu_ptr(array->pptrs[index & array->index_mask]), > value, map->value_size); > -- > 2.30.2 >
next prev parent reply other threads:[~2021-10-06 21:06 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-29 23:58 Joe Burton 2021-09-29 23:58 ` [RFC PATCH v2 01/13] bpf: Add machinery to register map tracing hooks Joe Burton 2021-09-29 23:58 ` [RFC PATCH v2 02/13] bpf: Allow loading BPF_TRACE_MAP programs Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 03/13] bpf: Add list of tracing programs to struct bpf_map Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 04/13] bpf: Define a few bpf_link_ops for BPF_TRACE_MAP Joe Burton 2021-09-30 0:26 ` Eric Dumazet 2021-09-30 1:09 ` Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 05/13] bpf: Enable creation of BPF_LINK_TYPE_MAP_TRACE Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 06/13] bpf: Add APIs to invoke tracing programs Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 07/13] bpf: Register BPF_MAP_TRACE_{UPDATE,DELETE}_ELEM hooks Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 08/13] libbpf: Support BPF_TRACE_MAP Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 09/13] bpf: Add infinite loop check on map tracers Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 10/13] Add bpf_map_trace_{update,delete}_elem() helper functions Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 11/13] bpf: verifier inserts map tracing helper call Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 12/13] bpf: Add selftests for map tracing Joe Burton 2021-09-29 23:59 ` [RFC PATCH v2 13/13] bpf: Add real world example " Joe Burton 2021-10-05 5:13 ` [RFC PATCH v2 00/13] Introduce BPF map tracing capability Alexei Starovoitov 2021-10-05 21:47 ` Joe Burton 2021-10-06 16:41 ` Alexei Starovoitov 2021-10-06 21:05 ` Joe Burton [this message] 2021-10-18 23:15 ` Alexei Starovoitov
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=CAL0ypaCwmGkQ0VK3nvfimHsO+OhBZb8cew-5c1gjZoZVZb1bBg@mail.gmail.com \ --to=jevburton@google.com \ --cc=alexei.starovoitov@gmail.com \ --cc=andrii@kernel.org \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=haoluo@google.com \ --cc=jevburton.kernel@gmail.com \ --cc=john.fastabend@gmail.com \ --cc=kafai@fb.com \ --cc=kpsingh@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=ppenkov@google.com \ --cc=sdf@google.com \ --cc=songliubraving@fb.com \ --cc=yhs@fb.com \ --subject='Re: [RFC PATCH v2 00/13] Introduce BPF map tracing capability' \ /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).