Re: [PATCH bpf-next v2 3/7] bpf: add MAP_LOOKUP_AND_DELETE_ELEM syscall

On 10/11/2018 06:51 PM, Alexei Starovoitov wrote:

On Wed, Oct 10, 2018 at 05:50:01PM -0500, Mauricio Vasquez wrote:

quoted

quoted

quoted

Does it make sense to you?

I reread the other patch, and found it does NOT use the following logic for
queue and stack:

                 rcu_read_lock();
                 ptr = map->ops->map_lookup_and_delete_elem(map, key);
                 if (ptr)
                         memcpy(value, ptr, value_size);

I guess this part is not used at all? Can we just remove it?

Thanks,
Song

This is the base code for map_lookup_and_delete support, it is not used in
queue/stack maps.

I think we can leave it there, so when somebody implements lookup_and_delete
for other maps doesn't have to care about implementing also this.

The code looks useful to me, but I also agree with Song. And in the kernel
we don't typically add 'almost dead code'.
May be provide an implementation of the lookup_and_delete for hash map
so it's actually used ?

I haven't written any code but I think there is a potential problem here.
Current lookup_and_delete returns a pointer to the element, hence 
deletion of the element should be done using call_rcu to guarantee this 
is valid after returning.
In the hashtab, the deletion only uses call_rcu when there is not 
prealloc, otherwise the element is pushed on the list of free elements 
immediately.
If we move the logic to push elements into the free list under a 
call_rcu invocation, it could happen that the free list is empty because 
the call_rcu is still pending (a similar issue we had with the 
queue/stack maps when they used a pass by reference API).

There is another way to implement it without this issue, in syscall.c:
l = ops->lookup(key);
memcpy(l, some_buffer)
ops->delete(key)

The point here is that the lookup_and_delete operation is not being used 
at all, so, is lookup + delete = lookup_and_delete?, can it be generalized?
If this is true, then what is the sense of having lookup_and_delete 
syscall?,

imo it would be a useful feature to have for hash map and will clarify
the intent of it for all other maps and for stack/queue in particular.