From: Brian Vazquez <email@example.com>
To: Willem de Bruijn <firstname.lastname@example.org>
Cc: Brian Vazquez <email@example.com>,
Alexei Starovoitov <firstname.lastname@example.org>,
Daniel Borkmann <email@example.com>,
"David S . Miller" <firstname.lastname@example.org>,
Stanislav Fomichev <email@example.com>,
Willem de Bruijn <firstname.lastname@example.org>,
Petar Penkov <email@example.com>,
Network Development <firstname.lastname@example.org>,
Subject: Re: [PATCH bpf-next 2/6] bpf: add BPF_MAP_DUMP command to dump more than one entry per call
Date: Wed, 24 Jul 2019 15:26:05 -0700 [thread overview]
Message-ID: <CABCgpaWCLJtDx8kHNiQZneqYZkZ3fzRGnipT5__kmwMhu01gemail@example.com> (raw)
On Wed, Jul 24, 2019 at 12:55 PM Willem de Bruijn
> On Wed, Jul 24, 2019 at 1:10 PM Brian Vazquez <firstname.lastname@example.org> wrote:
> > This introduces a new command to retrieve multiple number of entries
> > from a bpf map, wrapping the existing bpf methods:
> > map_get_next_key and map_lookup_elem
> > To start dumping the map from the beginning you must specify NULL as
> > the prev_key.
> > The new API returns 0 when it successfully copied all the elements
> > requested or it copied less because there weren't more elements to
> > retrieved (i.e err == -ENOENT). In last scenario err will be masked to 0.
> I think I understand this, but perhaps it can be explained a bit more
> concisely without reference to ENOENT and error masking. The function
> returns the min of the number of requested elements and the number of
> remaining elements in the map from the given starting point
That sounds better to me. Thanks!
> > On a successful call buf and buf_len will contain correct data and in
> > case prev_key was provided (not for the first walk, since prev_key is
> > NULL) it will contain the last_key copied into the prev_key which will
> > simplify next call.
> > Only when it can't find a single element it will return -ENOENT meaning
> > that the map has been entirely walked. When an error is return buf,
> > buf_len and prev_key shouldn't be read nor used.
> That's common for error handling. No need to state explicitly.
> > Because maps can be called from userspace and kernel code, this function
> > can have a scenario where the next_key was found but by the time we
> > try to retrieve the value the element is not there, in this case the
> > function continues and tries to get a new next_key value, skipping the
> > deleted key. If at some point the function find itself trap in a loop,
> > it will return -EINTR.
> Good to point this out! I don't think that unbounded continue;
> statements until an interrupt happens is sufficient. Please bound the
> number of retries to a low number.
And what would it be a good number? Maybe 3 attempts? And in that case
what error should be reported?
> > The function will try to fit as much as possible in the buf provided and
> > will return -EINVAL if buf_len is smaller than elem_size.
> > QUEUE and STACK maps are not supported.
> > Note that map_dump doesn't guarantee that reading the entire table is
> > consistent since this function is always racing with kernel and user code
> > but the same behaviour is found when the entire table is walked using
> > the current interfaces: map_get_next_key + map_lookup_elem.
> > It is also important to note that with a locked map, the lock is grabbed
> > for 1 entry at the time, meaning that the returned buf might or might not
> > be consistent.
> Would it be informative to signal to the caller if the read was
> complete and consistent (because the entire table was read while the
> lock was held)?
Mmm.. not sure how we could signal that to the caller. But I don't
think there's a way to know it was consistent (i.e. one element was
removed in bucket 20 and you are copying the keys in bucket 15, when
you get to bucket 20 there's no way to know that some entries were
removed when you traversed them). The lock is held for just 1 single
entry not the entire table.
Maybe clarify more that in the commit message?
Thanks for reviewing!
next prev parent reply other threads:[~2019-07-24 22:26 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-24 16:57 [PATCH bpf-next 0/6] bpf: add BPF_MAP_DUMP command to dump more than one entry per call Brian Vazquez
2019-07-24 16:57 ` [PATCH bpf-next 1/6] bpf: add bpf_map_value_size and bp_map_copy_value helper functions Brian Vazquez
2019-07-24 20:53 ` Song Liu
2019-07-24 16:57 ` [PATCH bpf-next 2/6] bpf: add BPF_MAP_DUMP command to dump more than one entry per call Brian Vazquez
2019-07-24 19:54 ` Willem de Bruijn
2019-07-24 22:26 ` Brian Vazquez [this message]
2019-07-24 22:33 ` Willem de Bruijn
2019-07-24 21:40 ` Song Liu
2019-07-24 22:44 ` Brian Vazquez
2019-07-24 23:04 ` Song Liu
2019-07-25 23:25 ` Brian Vazquez
2019-07-25 23:54 ` Alexei Starovoitov
2019-07-26 1:02 ` Willem de Bruijn
2019-07-26 1:24 ` Brian Vazquez
2019-07-26 1:47 ` Alexei Starovoitov
2019-07-26 6:10 ` Yonghong Song
2019-07-26 23:36 ` Brian Vazquez
2019-07-27 0:02 ` Jakub Kicinski
2019-07-27 17:54 ` Yonghong Song
2019-07-24 16:58 ` [PATCH bpf-next 3/6] bpf: keep bpf.h in sync with tools/ Brian Vazquez
2019-07-24 21:41 ` Song Liu
2019-07-24 23:10 ` Andrii Nakryiko
2019-07-25 23:27 ` Brian Vazquez
2019-07-24 16:58 ` [PATCH bpf-next 4/6] libbpf: support BPF_MAP_DUMP command Brian Vazquez
2019-07-24 19:51 ` Willem de Bruijn
2019-07-24 16:58 ` [PATCH bpf-next 5/6] selftests/bpf: test BPF_MAP_DUMP command on a bpf hashmap Brian Vazquez
2019-07-24 21:58 ` Song Liu
2019-07-24 16:58 ` [PATCH bpf-next 6/6] selftests/bpf: add test to measure performance of BPF_MAP_DUMP Brian Vazquez
2019-07-24 22:01 ` Song Liu
2019-07-24 19:20 ` [PATCH bpf-next 0/6] bpf: add BPF_MAP_DUMP command to dump more than one entry per call Song Liu
2019-07-24 22:15 ` Brian Vazquez
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).