Re: [PATCH bpf-next 03/16] bpf: Parse bpf_dynptr in map key

[Hou Tao <houtao@huaweicloud.com>]

Hi,

On 10/12/2024 12:29 AM, Alexei Starovoitov wrote:
> On Tue, Oct 8, 2024 at 2:02 AM Hou Tao <houtao@huaweicloud.com> wrote:
>> +#define MAX_DYNPTR_CNT_IN_MAP_KEY 4
>> +
>>  static int map_check_btf(struct bpf_map *map, struct bpf_token *token,
>>                          const struct btf *btf, u32 btf_key_id, u32 btf_value_id)
>>  {
>> @@ -1103,6 +1113,40 @@ static int map_check_btf(struct bpf_map *map, struct bpf_token *token,
>>         if (!value_type || value_size != map->value_size)
>>                 return -EINVAL;
>>
>> +       if (btf_type_is_dynptr(btf, key_type))
>> +               map->key_record = btf_new_bpf_dynptr_record();
>> +       else
>> +               map->key_record = btf_parse_fields(btf, key_type, BPF_DYNPTR, map->key_size);
>> +       if (!IS_ERR_OR_NULL(map->key_record)) {
>> +               if (map->key_record->cnt > MAX_DYNPTR_CNT_IN_MAP_KEY) {
>> +                       ret = -E2BIG;
>> +                       goto free_map_tab;
> Took me a while to grasp that map->key_record is only for dynptr fields
> and map->record is for the rest except dynptr fields.
>
> Maybe rename key_record to dynptr_fields ?
> Or at least add a comment to struct bpf_map to explain
> what each btf_record is for.

I was trying to rename map->record to map->value_record, however, I was
afraid that it may introduce too much churn, so I didn't do that. But I
think it is a good idea to add comments for both btf_record. And
considering that only bpf_dynptr is enabled for map key, renaming it to
dynptr_fields seems reasonable as well.
>
> It's kinda arbitrary decision to support multiple dynptr-s per key
> while other fields are not.
> Maybe worth looking at generalizing it a bit so single btf_record
> can have multiple of certain field kinds?
> In addition to btf_record->cnt you'd need btf_record->dynptr_cnt
> but that would be easier to extend in the future ?

Map value has already supported multiple kptrs or bpf_list_node. And in
the discussion [1], I thought multiple dynptr support in map key is
necessary, so I enabled it.

[1]:
https://lore.kernel.org/bpf/CAADnVQJWaBRB=P-ZNkppwm=0tZaT3qP8PKLLJ2S5SSA2-S8mxg@mail.gmail.com/