Hi,
On 10/12/2024 12:29 AM, Alexei Starovoitov wrote:
> On Tue, Oct 8, 2024 at 2:02 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/
