Re: [PATCH bpf-next v1 7/8] libbpf: sync progs autoload with maps autocreate for struct_ops maps

[Martin KaFai Lau <martin.lau@linux.dev>]

On 2/27/24 12:45 PM, Eduard Zingerman wrote:
> Make bpf_map__set_autocreate() for struct_ops maps toggle autoload
> state for referenced programs.
> 
> E.g. for the BPF code below:
> 
>      SEC("struct_ops/test_1") int BPF_PROG(foo) { ... }
>      SEC("struct_ops/test_2") int BPF_PROG(bar) { ... }
> 
>      SEC(".struct_ops.link")
>      struct test_ops___v1 A = {
>          .foo = (void *)foo
>      };
> 
>      SEC(".struct_ops.link")
>      struct test_ops___v2 B = {
>          .foo = (void *)foo,
>          .bar = (void *)bar,
>      };
> 
> And the following libbpf API calls:
> 
>      bpf_map__set_autocreate(skel->maps.A, true);
>      bpf_map__set_autocreate(skel->maps.B, false);
> 
> The autoload would be enabled for program 'foo' and disabled for
> program 'bar'.
> 
> Do not apply such toggling if program autoload state is set by a call
> to bpf_program__set_autoload().
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/lib/bpf/libbpf.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b39d3f2898a1..1ea3046724f8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -446,13 +446,18 @@ struct bpf_program {
>   	struct bpf_object *obj;
>   
>   	int fd;
> -	bool autoload;
> +	bool autoload:1;
> +	bool autoload_user_set:1;
>   	bool autoattach;
>   	bool sym_global;
>   	bool mark_btf_static;
>   	enum bpf_prog_type type;
>   	enum bpf_attach_type expected_attach_type;
>   	int exception_cb_idx;
> +	/* total number of struct_ops maps with autocreate == true
> +	 * that reference this program
> +	 */
> +	__u32 struct_ops_refs;

Instead of adding struct_ops_refs and autoload_user_set,

for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking 
prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at 
load time and is only set if it is used by at least one autocreate map, if I 
read patch 2 & 3 correctly.

Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used 
by one struct_ops map with autocreate == true.

If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be 
loaded even the autoload is set. If bpf prog is used in a struct_ops map and its 
autoload is set to false, the struct_ops map will be in broken state. Thus, 
prog->autoload does not fit very well with struct_ops prog and may as well 
depend on whether the struct_ops prog is used by a struct_ops map alone?