Re: [PATCH bpf-next v3 4/5] bpftool: generated shadow variables for struct_ops maps.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/21/24 03:49, Quentin Monnet wrote:
> 2024-02-21 01:23 UTC+0000 ~ thinker.li@gmail.com
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Declares and defines a pointer of the shadow type for each struct_ops map.
>>
>> The code generator will create an anonymous struct type as the shadow type
>> for each struct_ops map. The shadow type is translated from the original
>> struct type of the map. The user of the skeleton use pointers of them to
>> access the values of struct_ops maps.
>>
>> However, shadow types only supports certain types of fields, such as scalar
> 
> Nit: "such as" implies the list may not be exhaustive.
> 
>> types and function pointers. Any fields of unsupported types are translated
>> into an array of characters to occupy the space of the original
>> field. Function pointers are translated into pointers of the struct
>> bpf_program. Additionally, padding fields are generated to occupy the space
>> between two consecutive fields.
>>
>> The pointers of shadow types of struct_osp maps are initialized when
>> *__open_opts() in skeletons are called. For a map called FOO, the user can
>> access it through the pointer at skel->struct_ops.FOO.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/bpf/bpftool/gen.c | 229 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 228 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
>> index a9334c57e859..20c5d5912df7 100644
>> --- a/tools/bpf/bpftool/gen.c
>> +++ b/tools/bpf/bpftool/gen.c
>> @@ -909,6 +909,201 @@ codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_li
>>   	}
>>   }
>>   
>> +static int walk_st_ops_shadow_vars(struct btf *btf,
>> +				   const char *ident,
>> +				   const struct bpf_map *map)
>> +{
>> +	DECLARE_LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts,
>> +			    .indent_level = 3,
>> +			    );
>> +	const struct btf_type *map_type, *member_type;
>> +	__u32 map_type_id, member_type_id;
>> +	__u32 offset, next_offset = 0;
>> +	const struct btf_member *m;
>> +	const char *member_name;
>> +	struct btf_dump *d = NULL;
>> +	int i, err = 0;
>> +	int size, map_size;
>> +
>> +	map_type_id = bpf_map__btf_value_type_id(map);
>> +	if (map_type_id == 0)
>> +		return -EINVAL;
>> +	map_type = btf__type_by_id(btf, map_type_id);
>> +	if (!map_type)
>> +		return -EINVAL;
>> +
>> +	d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
>> +	if (!d)
>> +		return -errno;
>> +
>> +	for (i = 0, m = btf_members(map_type);
>> +	     i < btf_vlen(map_type);
>> +	     i++, m++) {
>> +		member_type = skip_mods_and_typedefs(btf, m->type,
>> +						     &member_type_id);
>> +		if (!member_type) {
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		member_name = btf__name_by_offset(btf, m->name_off);
>> +		if (!member_name) {
>> +			err = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		offset = m->offset / 8;
>> +		if (next_offset != offset) {
>> +			printf("\t\t\tchar __padding_%d[%d];\n",
>> +			       i - 1, offset - next_offset);
>> +		}
>> +
>> +		switch (btf_kind(member_type)) {
>> +		case BTF_KIND_INT:
>> +		case BTF_KIND_FLOAT:
>> +		case BTF_KIND_ENUM:
>> +		case BTF_KIND_ENUM64:
>> +			/* scalar type */
>> +			printf("\t\t\t");
>> +			opts.field_name = member_name;
>> +			err = btf_dump__emit_type_decl(d, member_type_id,
>> +						       &opts);
>> +			if (err)
>> +				goto out;
>> +			printf(";\n");
>> +
>> +			size = btf__resolve_size(btf, member_type_id);
>> +			if (size < 0) {
>> +				err = size;
>> +				goto out;
>> +			}
>> +
>> +			next_offset = offset + size;
>> +			break;
>> +
>> +		case BTF_KIND_PTR:
>> +			if (resolve_func_ptr(btf, m->type, NULL)) {
>> +				/* Function pointer */
>> +				printf("\t\t\tconst struct bpf_program *%s;\n",
>> +				       member_name);
>> +
>> +				next_offset = offset + sizeof(void *);
>> +				break;
>> +			}
>> +			fallthrough;
> 
> I wouldn't mind a comment about the "fallthrough;" to state explicitly
> that only function pointers are supported for now.


Sure

> 
>> +
>> +		default:
>> +			/* Unsupported types
>> +			 *
>> +			 * For unsupported types, we have to generate
>> +			 * definitions for them in order to support
>> +			 * them. For example, we need to generate a
>> +			 * definition for a struct type or a union type. It
>> +			 * may cause type conflicts without renaming since
>> +			 * the same type may be defined for several
>> +			 * skeletons, and the user may include these
>> +			 * skeletons in the same compile unit.
>> +			 */
> 
> This comment could be clearer. "For unsupported types, we have to
> generate definitions for them in order to support them". So do we, or do
> we not support them? "It may cause type conflicts [...]" -> do we
> address these?
> 
> My understanding is that this note describes the work to do if we want
> to add support in the future, and this could perhaps be more explicit:
> "We do not support other types yet. The reason is that ... But when we
> generate definitions, we will have to take care of type conflicts
> because ...". What do you think?


Agree! I will rephrase this comment.

> 
>> +			if (i == btf_vlen(map_type) - 1) {
>> +				map_size = btf__resolve_size(btf, map_type_id);
>> +				if (map_size < 0)
>> +					return -EINVAL;
>> +				size = map_size - offset;
>> +			} else {
>> +				size = (m[1].offset - m->offset) / 8;
>> +			}
>> +
>> +			printf("\t\t\tchar __padding_%d[%d];\n", i, size);
>> +
>> +			next_offset = offset + size;
>> +			break;
>> +		}
>> +	}
>> +
>> +out:
>> +	btf_dump__free(d);
>> +
>> +	return err;
>> +}
>> +
>> +/* Generate the pointer of the shadow type for a struct_ops map.
>> + *
>> + * This function adds a pointer of the shadow type for a struct_ops map.
>> + * The members of a struct_ops map can be exported through a pointer to a
>> + * shadow type. The user can access these members through the pointer.
>> + *
>> + * A shadow type includes not all members, only members of some types.
>> + * They are scalar types and function pointers. The function pointers are
>> + * translated to the pointer of the struct bpf_program. The scalar types
>> + * are translated to the original type without any modifiers.
>> + *
>> + * Unsupported types will be translated to a char array to take the same
>> + * space of the original field. However, due to handling padding and
>> + * alignments, the user should not access them directly.
> 
> What's the risk, and how should users know?

The names of unsupported fields are replaced by "__padding_*", and their
types are "char []".
Changing names and types of fields in a struct can lead to accessing
issues, where users may inadvertently corrupt data due to padding and
field reordering in different versions.

I will include the above explanation in the next version.

> 
>> + */
> 
> [...]
> 
> Thanks for this work! The bpftool changes look good. I've got these few
> observations above, but notwithstanding:
> 
> Reviewed-by: Quentin Monnet <quentin@isovalent.com>
> 
> I wonder, did you think of adding a paragraph or an example to the man
> page for "bpftool gen"? Your change is for a specific use case, but
> otherwise I'm not sure how users will ever know that these shadow types
> are available (other than discovering them by luck in a skeleton) or how
> to use them if they need to.

Sure! I will add it in the following version.

> 
> Thanks,
> Quentin