On 1/19/19 10:56 AM, Jakub Kicinski wrote:
> On Thu, 17 Jan 2019 22:50:53 -0800, Alexei Starovoitov wrote:
>> Introduce BPF_F_LOCK flag for map_lookup and map_update syscall commands
>> and for map_update() helper function.
>> In all these cases take a lock of existing element (which was provided
>> in BTF description) before copying (in or out) the rest of map value.
>>
>> Implementation details that are part of uapi:
>>
>> Array:
>> The array map takes the element lock for lookup/update.
>>
>> Hash:
>> hash map also takes the lock for lookup/update and tries to avoid the bucket
>> lock.
>> If old element exists it takes the element lock and updates the element in
>> place.
>> If element doesn't exist it allocates new one and inserts into hash table
>> while holding the bucket lock.
>> In rare case the hashmap has to take both the bucket lock and the element
>> lock
>> to update old value in place.
>>
>> Cgroup local storage:
>> It is similar to array. update in place and lookup are done with lock taken.
>>
>> Signed-off-by: Alexei Starovoitov <a...@kernel.org>
>
>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>> index 48a41bf65e1b..6397b12c130e 100644
>> --- a/kernel/bpf/hashtab.c
>> +++ b/kernel/bpf/hashtab.c
>> @@ -809,11 +809,11 @@ static struct htab_elem *alloc_htab_elem(struct
>> bpf_htab *htab, void *key,
>> static int check_flags(struct bpf_htab *htab, struct htab_elem *l_old,
>> u64 map_flags)
>> {
>> - if (l_old && map_flags == BPF_NOEXIST)
>> + if (l_old && (map_flags & ~BPF_F_LOCK) == BPF_NOEXIST)
>> /* elem already exists */
>> return -EEXIST;
>>
>> - if (!l_old && map_flags == BPF_EXIST)
>> + if (!l_old && (map_flags & ~BPF_F_LOCK) == BPF_EXIST)
>> /* elem doesn't exist, cannot update it */
>> return -ENOENT;
>>
>> @@ -832,7 +832,7 @@ static int htab_map_update_elem(struct bpf_map *map,
>> void *key, void *value,
>> u32 key_size, hash;
>> int ret;
>>
>> - if (unlikely(map_flags > BPF_EXIST))
>> + if (unlikely(map_flags & ~(BPF_F_LOCK | BPF_EXIST | BPF_NOEXIST)))
>> /* unknown flags */
>> return -EINVAL;
>
> Perhaps we should check for:
>
> hweight(map_flags & (BPF_EXIST | BPF_NOEXIST)) < 2
hweight in critical path is imo too slow.
> right now this > BPF_EXIST is a little hacky, as it depends on the fact,
> that BPF_EXIST is 2.. If I read the code correctly BPF_EXIST |
> BPF_NOEXIST will be treated as BPF_NOEXIST. Not sure that matters.
#define BPF_ANY 0 /* create new element or update existing */
#define BPF_NOEXIST 1 /* create new element if it didn't exist */
#define BPF_EXIST 2 /* update existing element */
#define BPF_F_LOCK 4 /* spin_lock-ed map_lookup/map_update */
for all other flags we just 'or' them together and do
flag & ~(FLAG1 | FLAG2 | ..)
so it's easy to add new flags later.
Here I'm only skipping BPF_ANY from this practice because it's zero.
>> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
>> index 0295427f06e2..6b572e2de7fb 100644
>> --- a/kernel/bpf/local_storage.c
>> +++ b/kernel/bpf/local_storage.c
>> @@ -131,7 +131,14 @@ static int cgroup_storage_update_elem(struct bpf_map
>> *map, void *_key,
>> struct bpf_cgroup_storage *storage;
>> struct bpf_storage_buffer *new;
>>
>> - if (flags != BPF_ANY && flags != BPF_EXIST)
>> + if (unlikely(flags & ~(BPF_F_LOCK | BPF_EXIST | BPF_NOEXIST)))
>> + return -EINVAL;
>> +
>> + if (unlikely(flags & BPF_NOEXIST))
>> + return -EINVAL;
>
> nit: for arrays we have a different return code with BPF_NOEXIST, but
> here its both EINVAL, so perhaps just drop the BPF_NOEXIST from
> the mask?
I very much prefer compiler to do such optimizations.
First line is doing:
flag & ~(FLAG1 | FLAG2 | ..)
like in all other places.
And second check if (unlikely(flags & BPF_NOEXIST)) is specific
to this helper.
>> @@ -818,6 +836,12 @@ static int map_update_elem(union bpf_attr *attr)
>> goto err_put;
>> }
>>
>> + if ((attr->flags & BPF_F_LOCK) &&
>> + !map_value_has_spin_lock(map)) {
>
> nit: do we need this check in syscall.c for update? Just wondering.
yes. otherwise the flag will be nop for other map types (percpu, lru, etc)
