Re: [PATCH bpf-next] bpf: add lookup_and_delete_elem support to hashtab

[Denis Salopek <denis.salopek@sartura.hr>]

On Fri, Jan 29, 2021 at 04:58:12PM +0100, Daniel Borkmann wrote:
> On 1/27/21 6:12 PM, Denis Salopek wrote:
> > Extend the existing bpf_map_lookup_and_delete_elem() functionality to
> > hashtab maps, in addition to stacks and queues.
> > Create a new hashtab bpf_map_ops function that does lookup and deletion
> > of the element under the same bucket lock and add the created map_ops to
> > bpf.h.
> > Add the appropriate test case to 'maps' selftests.
> > 
> > Signed-off-by: Denis Salopek <denis.salopek@sartura.hr>
> > Cc: Juraj Vijtiuk <juraj.vijtiuk@sartura.hr>
> > Cc: Luka Oreskovic <luka.oreskovic@sartura.hr>
> > Cc: Luka Perkov <luka.perkov@sartura.hr>
> 
> This is already possible in a more generic form, that is, via bpf(2) cmd
> BPF_MAP_LOOKUP_AND_DELETE_BATCH which is implemented by the different htab
> map flavors which also take the bucket lock. Did you have a try at that?
> 
> > ---
> >   include/linux/bpf.h                     |  1 +
> >   kernel/bpf/hashtab.c                    | 38 +++++++++++++++++++++++++
> >   kernel/bpf/syscall.c                    |  9 ++++++
> >   tools/testing/selftests/bpf/test_maps.c |  7 +++++
> >   4 files changed, 55 insertions(+)
> > 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 1aac2af12fed..003c1505f0e3 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -77,6 +77,7 @@ struct bpf_map_ops {
> >   	/* funcs callable from userspace and from eBPF programs */
> >   	void *(*map_lookup_elem)(struct bpf_map *map, void *key);
> > +	int (*map_lookup_and_delete_elem)(struct bpf_map *map, void *key, void *value);
> >   	int (*map_update_elem)(struct bpf_map *map, void *key, void *value, u64 flags);
> >   	int (*map_delete_elem)(struct bpf_map *map, void *key);
> >   	int (*map_push_elem)(struct bpf_map *map, void *value, u64 flags);
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index c1ac7f964bc9..8d8463e0ea34 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -973,6 +973,43 @@ static int check_flags(struct bpf_htab *htab, struct htab_elem *l_old,
> >   	return 0;
> >   }
> > +/* Called from syscall or from eBPF program */
> > +static int htab_map_lookup_and_delete_elem(struct bpf_map *map, void *key, void *value)
> > +{
> > +	struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> > +	struct hlist_nulls_head *head;
> > +	struct bucket *b;
> > +	struct htab_elem *l;
> > +	unsigned long flags;
> > +	u32 hash, key_size;
> > +	int ret;
> > +
> > +	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held());
> > +
> > +	key_size = map->key_size;
> > +
> > +	hash = htab_map_hash(key, key_size, htab->hashrnd);
> > +	b = __select_bucket(htab, hash);
> > +	head = &b->head;
> > +
> > +	ret = htab_lock_bucket(htab, b, hash, &flags);
> > +	if (ret)
> > +		return ret;
> > +
> > +	l = lookup_elem_raw(head, hash, key, key_size);
> > +
> > +	if (l) {
> > +		copy_map_value(map, value, l->key + round_up(key_size, 8));
> > +		hlist_nulls_del_rcu(&l->hash_node);
> > +		free_htab_elem(htab, l);
> > +	} else {
> > +		ret = -ENOENT;
> > +	}
> > +
> > +	htab_unlock_bucket(htab, b, hash, flags);
> > +	return ret;
> > +}
> > +
> >   /* Called from syscall or from eBPF program */
> >   static int htab_map_update_elem(struct bpf_map *map, void *key, void *value,
> >   				u64 map_flags)
> > @@ -1877,6 +1914,7 @@ const struct bpf_map_ops htab_map_ops = {
> >   	.map_free = htab_map_free,
> >   	.map_get_next_key = htab_map_get_next_key,
> >   	.map_lookup_elem = htab_map_lookup_elem,
> > +	.map_lookup_and_delete_elem = htab_map_lookup_and_delete_elem,
> >   	.map_update_elem = htab_map_update_elem,
> >   	.map_delete_elem = htab_map_delete_elem,
> >   	.map_gen_lookup = htab_map_gen_lookup,
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index e5999d86c76e..4ff45c8d1077 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -1505,6 +1505,15 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
> >   	if (map->map_type == BPF_MAP_TYPE_QUEUE ||
> >   	    map->map_type == BPF_MAP_TYPE_STACK) {
> >   		err = map->ops->map_pop_elem(map, value);
> > +	} else if (map->map_type == BPF_MAP_TYPE_HASH) {
> > +		if (!bpf_map_is_dev_bound(map)) {
> > +			bpf_disable_instrumentation();
> > +			rcu_read_lock();
> > +			err = map->ops->map_lookup_and_delete_elem(map, key, value);
> > +			rcu_read_unlock();
> > +			bpf_enable_instrumentation();
> > +			maybe_wait_bpf_programs(map);
> > +		}
> >   	} else {
> >   		err = -ENOTSUPP;
> >   	}

Hi,

Is there something wrong with adding hashmap functionality to the
map_lookup_and_delete_elem() function? As per commit
bd513cd08f10cbe28856f99ae951e86e86803861, adding this functionality for
other map types was the plan at the time this function was implemented,
so wouldn't this addition be useful? Otherwise, this function would only
be used for stack/queue popping. So why does it even exist in the first
place, if the _batch function can be used instead?

I understand the _batch function can do the same job, but is there a
reason why using it would be a better option? There is also a
performance impact in the _batch function when compared to my function -
not too big, but still worth the mention. In my benchmarks, the _batch
function takes 15% longer for lookup and deletion of one element.

Denis