On 10/11/2018 06:51 PM, Alexei Starovoitov wrote:
On Wed, Oct 10, 2018 at 05:50:01PM -0500, Mauricio Vasquez wrote:
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.
