Hi,
On 2/14/2025 7:56 AM, Alexei Starovoitov wrote:
> On Sat, Jan 25, 2025 at 2:59 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>
>> When there is bpf_dynptr field in the map key btf type or the map key
>> btf type is bpf_dyntr, set BPF_INT_F_DYNPTR_IN_KEY in map_flags.
>>
>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>> ---
>> kernel/bpf/syscall.c | 36 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 07c67ad1a6a07..46b96d062d2db 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1360,6 +1360,34 @@ static struct btf *get_map_btf(int btf_fd)
>> return btf;
>> }
>>
>> +static int map_has_dynptr_in_key_type(struct btf *btf, u32 btf_key_id, u32 key_size)
>> +{
>> + const struct btf_type *type;
>> + struct btf_record *record;
>> + u32 btf_key_size;
>> +
>> + if (!btf_key_id)
>> + return 0;
>> +
>> + type = btf_type_id_size(btf, &btf_key_id, &btf_key_size);
>> + if (!type || btf_key_size != key_size)
>> + return -EINVAL;
>> +
>> + /* For dynptr key, key BTF type must be struct */
>> + if (!__btf_type_is_struct(type))
>> + return 0;
>> +
>> + if (btf_type_is_dynptr(btf, type))
>> + return 1;
>> +
>> + record = btf_parse_fields(btf, type, BPF_DYNPTR, key_size);
>> + if (IS_ERR(record))
>> + return PTR_ERR(record);
>> +
>> + btf_record_free(record);
>> + return !!record;
>> +}
>> +
>> #define BPF_MAP_CREATE_LAST_FIELD map_token_fd
>> /* called via syscall */
>> static int map_create(union bpf_attr *attr)
>> @@ -1398,6 +1426,14 @@ static int map_create(union bpf_attr *attr)
>> btf = get_map_btf(attr->btf_fd);
>> if (IS_ERR(btf))
>> return PTR_ERR(btf);
>> +
>> + err = map_has_dynptr_in_key_type(btf, attr->btf_key_type_id, attr->key_size);
>> + if (err < 0)
>> + goto put_btf;
>> + if (err > 0) {
>> + attr->map_flags |= BPF_INT_F_DYNPTR_IN_KEY;
> I don't like this inband signaling in the uapi field.
> The whole refactoring in patch 4 to do patch 6 and
> subsequent bpf_map_has_dynptr_key() in various places
> feels like reinventing the wheel.
>
> We already have map_check_btf() mechanism that works for
> existing special fields inside BTF.
> Please use it.
Yes. However map->key_record is only available after the map is created,
but the creation of hash map needs to check it before the map is
created. Instead of using an internal flag, how about adding extra
argument for both ->map_alloc_check() and ->map_alloc() as proposed in
the commit message of the previous patch ?
>
> map_has_dynptr_in_key_type() can be done in map_check_btf()
> after map is created, no ?
No. both ->map_alloc_check() and ->map_alloc() need to know whether
dynptr is enabled (as explained in the previous commit message). Both of
these functions are called before the map is created.
> Then when it passes map->map_type check set a bool inside
> struct bpf_map, so that bpf_map_has_dynptr_key() can be fast
> in the critical path of hashtab.
> Or better yet use:
> static inline bool bpf_map_has_dynptr_key(const struct bpf_map *map)
> {
> /* key_record is not NULL when the map key contains bpf_dynptr_user */
> return !!map->key_record;
> }
> since htab_map_hash() has to read key_record anyway,
> hence better D$ access.
> .
