On 12/6/22 8:41 PM, Alexei Starovoitov wrote:
> On Tue, Dec 06, 2022 at 03:09:51PM -0800, Dave Marchevsky wrote:
>> Many of the structs recently added to track field info for linked-list
>> head are useful as-is for rbtree root. So let's do a mechanical renaming
>> of list_head-related types and fields:
>>
>> include/linux/bpf.h:
>> struct btf_field_list_head -> struct btf_field_datastructure_head
>> list_head -> datastructure_head in struct btf_field union
>> kernel/bpf/btf.c:
>> list_head -> datastructure_head in struct btf_field_info
>
> Looking through this patch and others it eventually becomes
> confusing with 'datastructure head' name.
> I'm not sure what is 'head' of the data structure.
> There is head in the link list, but 'head of tree' is odd.
>
> The attemp here is to find a common name that represents programming
> concept where there is a 'root' and there are 'nodes' that added to that 'root'.
> The 'data structure' name is too broad in that sense.
> Especially later it becomes 'datastructure_api' which is even broader.
>
> I was thinking to propose:
> struct btf_field_list_head -> struct btf_field_tree_root
> list_head -> tree_root in struct btf_field union
>
> and is_kfunc_tree_api later...
> since link list is a tree too.
>
> But reading 'tree' next to other names like 'field', 'kfunc'
> it might be mistaken that 'tree' applies to the former.
> So I think using 'graph' as more general concept to describe both
> link list and rb-tree would be the best.
>
> So the proposal:
> struct btf_field_list_head -> struct btf_field_graph_root
> list_head -> graph_root in struct btf_field union
>
> and is_kfunc_graph_api later...
>
> 'graph' is short enough and rarely used in names,
> so it stands on its own next to 'field' and in combination
> with other names.
> wdyt?
>
I'm not a huge fan of 'graph', but it's certainly better than
'datastructure_api', and avoids the "all next-gen datastructures must do this"
implication of a 'ng_ds' name. So will try the rename in v2.
(all specific GRAPH naming suggestions in subsequent patches will
be done as well)
list 'head' -> list 'root' SGTM as well. Not ideal, but alternatives
are worse (rbtree 'head'...)
>>
>> This is a nonfunctional change, functionality to actually use these
>> fields for rbtree will be added in further patches.
>>
>> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
>> ---
>> include/linux/bpf.h | 4 ++--
>> kernel/bpf/btf.c | 21 +++++++++++----------
>> kernel/bpf/helpers.c | 4 ++--
>> kernel/bpf/verifier.c | 21 +++++++++++----------
>> 4 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 4920ac252754..9e8b12c7061e 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -189,7 +189,7 @@ struct btf_field_kptr {
>> u32 btf_id;
>> };
>>
>> -struct btf_field_list_head {
>> +struct btf_field_datastructure_head {
>> struct btf *btf;
>> u32 value_btf_id;
>> u32 node_offset;
>> @@ -201,7 +201,7 @@ struct btf_field {
>> enum btf_field_type type;
>> union {
>> struct btf_field_kptr kptr;
>> - struct btf_field_list_head list_head;
>> + struct btf_field_datastructure_head datastructure_head;
>> };
>> };
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index c80bd8709e69..284e3e4b76b7 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -3227,7 +3227,7 @@ struct btf_field_info {
>> struct {
>> const char *node_name;
>> u32 value_btf_id;
>> - } list_head;
>> + } datastructure_head;
>> };
>> };
>>
>> @@ -3334,8 +3334,8 @@ static int btf_find_list_head(const struct btf *btf, const struct btf_type *pt,
>> return -EINVAL;
>> info->type = BPF_LIST_HEAD;
>> info->off = off;
>> - info->list_head.value_btf_id = id;
>> - info->list_head.node_name = list_node;
>> + info->datastructure_head.value_btf_id = id;
>> + info->datastructure_head.node_name = list_node;
>> return BTF_FIELD_FOUND;
>> }
>>
>> @@ -3603,13 +3603,14 @@ static int btf_parse_list_head(const struct btf *btf, struct btf_field *field,
>> u32 offset;
>> int i;
>>
>> - t = btf_type_by_id(btf, info->list_head.value_btf_id);
>> + t = btf_type_by_id(btf, info->datastructure_head.value_btf_id);
>> /* We've already checked that value_btf_id is a struct type. We
>> * just need to figure out the offset of the list_node, and
>> * verify its type.
>> */
>> for_each_member(i, t, member) {
>> - if (strcmp(info->list_head.node_name, __btf_name_by_offset(btf, member->name_off)))
>> + if (strcmp(info->datastructure_head.node_name,
>> + __btf_name_by_offset(btf, member->name_off)))
>> continue;
>> /* Invalid BTF, two members with same name */
>> if (n)
>> @@ -3626,9 +3627,9 @@ static int btf_parse_list_head(const struct btf *btf, struct btf_field *field,
>> if (offset % __alignof__(struct bpf_list_node))
>> return -EINVAL;
>>
>> - field->list_head.btf = (struct btf *)btf;
>> - field->list_head.value_btf_id = info->list_head.value_btf_id;
>> - field->list_head.node_offset = offset;
>> + field->datastructure_head.btf = (struct btf *)btf;
>> + field->datastructure_head.value_btf_id = info->datastructure_head.value_btf_id;
>> + field->datastructure_head.node_offset = offset;
>> }
>> if (!n)
>> return -ENOENT;
>> @@ -3735,11 +3736,11 @@ int btf_check_and_fixup_fields(const struct btf *btf, struct btf_record *rec)
>>
>> if (!(rec->fields[i].type & BPF_LIST_HEAD))
>> continue;
>> - btf_id = rec->fields[i].list_head.value_btf_id;
>> + btf_id = rec->fields[i].datastructure_head.value_btf_id;
>> meta = btf_find_struct_meta(btf, btf_id);
>> if (!meta)
>> return -EFAULT;
>> - rec->fields[i].list_head.value_rec = meta->record;
>> + rec->fields[i].datastructure_head.value_rec = meta->record;
>>
>> if (!(rec->field_mask & BPF_LIST_NODE))
>> continue;
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index cca642358e80..6c67740222c2 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -1737,12 +1737,12 @@ void bpf_list_head_free(const struct btf_field *field, void *list_head,
>> while (head != orig_head) {
>> void *obj = head;
>>
>> - obj -= field->list_head.node_offset;
>> + obj -= field->datastructure_head.node_offset;
>> head = head->next;
>> /* The contained type can also have resources, including a
>> * bpf_list_head which needs to be freed.
>> */
>> - bpf_obj_free_fields(field->list_head.value_rec, obj);
>> + bpf_obj_free_fields(field->datastructure_head.value_rec, obj);
>> /* bpf_mem_free requires migrate_disable(), since we can be
>> * called from map free path as well apart from BPF program (as
>> * part of map ops doing bpf_obj_free_fields).
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 6f0aac837d77..bc80b4c4377b 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8615,21 +8615,22 @@ static int process_kf_arg_ptr_to_list_node(struct bpf_verifier_env *env,
>>
>> field = meta->arg_list_head.field;
>>
>> - et = btf_type_by_id(field->list_head.btf, field->list_head.value_btf_id);
>> + et = btf_type_by_id(field->datastructure_head.btf, field->datastructure_head.value_btf_id);
>> t = btf_type_by_id(reg->btf, reg->btf_id);
>> - if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, 0, field->list_head.btf,
>> - field->list_head.value_btf_id, true)) {
>> + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, 0, field->datastructure_head.btf,
>> + field->datastructure_head.value_btf_id, true)) {
>> verbose(env, "operation on bpf_list_head expects arg#1 bpf_list_node at offset=%d "
>> "in struct %s, but arg is at offset=%d in struct %s\n",
>> - field->list_head.node_offset, btf_name_by_offset(field->list_head.btf, et->name_off),
>> + field->datastructure_head.node_offset,
>> + btf_name_by_offset(field->datastructure_head.btf, et->name_off),
>> list_node_off, btf_name_by_offset(reg->btf, t->name_off));
>> return -EINVAL;
>> }
>>
>> - if (list_node_off != field->list_head.node_offset) {
>> + if (list_node_off != field->datastructure_head.node_offset) {
>> verbose(env, "arg#1 offset=%d, but expected bpf_list_node at offset=%d in struct %s\n",
>> - list_node_off, field->list_head.node_offset,
>> - btf_name_by_offset(field->list_head.btf, et->name_off));
>> + list_node_off, field->datastructure_head.node_offset,
>> + btf_name_by_offset(field->datastructure_head.btf, et->name_off));
>> return -EINVAL;
>> }
>> /* Set arg#1 for expiration after unlock */
>> @@ -9078,9 +9079,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>
>> mark_reg_known_zero(env, regs, BPF_REG_0);
>> regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC;
>> - regs[BPF_REG_0].btf = field->list_head.btf;
>> - regs[BPF_REG_0].btf_id = field->list_head.value_btf_id;
>> - regs[BPF_REG_0].off = field->list_head.node_offset;
>> + regs[BPF_REG_0].btf = field->datastructure_head.btf;
>> + regs[BPF_REG_0].btf_id = field->datastructure_head.value_btf_id;
>> + regs[BPF_REG_0].off = field->datastructure_head.node_offset;
>> } else if (meta.func_id == special_kfunc_list[KF_bpf_cast_to_kern_ctx]) {
>> mark_reg_known_zero(env, regs, BPF_REG_0);
>> regs[BPF_REG_0].type = PTR_TO_BTF_ID | PTR_TRUSTED;
>> --
>> 2.30.2
>>
