On Mon, Oct 21, 2024 at 7:02 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> 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.
fwiw I believe we reached the dead end there.
The whole support for bpf_list and bpf_rb_tree may get deprecated
and removed. The expected users didn't materialize.
> 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/
Sure. That's a different reasoning and use case.
I'm proposing to use a single btf_record with different cnt-s.
The current btf_record->cnt will stay as-is indicating total number of fields
while btf_record->dynptr_cnt will be just for these dynptrs you're introducing.
Then you won't need two top level btf_record-s.
