Re: [PATCH v2 bpf-next 6/9] libbpf: refactor ELF section handler definitions

[Dave Marchevsky <davemarchevsky@fb.com>]

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Refactor ELF section handler definitions table to use a set of flags and
> unified SEC_DEF() macro. This allows for more succinct and table-like
> set of definitions, and allows to more easily extend the logic without
> adding more verbosity (this is utilized in later patches in the series).
> 
> This approach is also making libbpf-internal program pre-load callback
> not rely on bpf_sec_def definition, which demonstrates that future
> pluggable ELF section handlers will be able to achieve similar level of
> integration without libbpf having to expose extra types and APIs.
> 
> For starters, update SEC_DEF() definitions and make them more succinct.
> Also convert BPF_PROG_SEC() and BPF_APROG_COMPAT() definitions to
> a common SEC_DEF() use.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/lib/bpf/libbpf.c | 183 ++++++++++++++++-------------------------
>  1 file changed, 73 insertions(+), 110 deletions(-)

To summarize VC convo we had about this patch, you don't expect custom sec_def
writers to necessarily follow your sec_def_flags approach, but it's a good
demonstration that a long's worth of flags is plenty for enabling custom
functionality. And custom sec_def writers can treat the cookie as a ptr to a
config struct if they need something more complicated, without imposing the
struct format on all other sec_defs.

[...]

> @@ -7955,15 +7965,14 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  		.sec = string,						    \
>  		.prog_type = ptype,					    \
>  		.expected_attach_type = eatype,				    \
> -		.is_exp_attach_type_optional = eatype_optional,		    \
> -		.is_attachable = attachable,				    \
> -		.is_attach_btf = attach_btf,				    \
> +		.cookie = (long) (					    \
> +			(eatype_optional ? SEC_EXP_ATTACH_OPT : 0) |   \
> +			(attachable ? SEC_ATTACHABLE : 0) |		    \
> +			(attach_btf ? SEC_ATTACH_BTF : 0)		    \
> +		),							    \
>  		.preload_fn = libbpf_preload_prog,			    \
>  	}
>  
> -/* Programs that can NOT be attached. */

I found this comment and APROG_COMPAT comment useful. Not as clear to me what
SEC_NONE implies without some comment explaining or giving example. The other 
flags are more obvious to me but might be worth being explicit there as well.

> -#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
> -
>  /* Programs that can be attached. */
>  #define BPF_APROG_SEC(string, ptype, atype) \
>  	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> @@ -7976,14 +7985,11 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  #define BPF_PROG_BTF(string, ptype, eatype) \
>  	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
>  
> -/* Programs that can be attached but attach type can't be identified by section
> - * name. Kept for backward compatibility.
> - */
> -#define BPF_APROG_COMPAT(string, ptype) BPF_PROG_SEC(string, ptype)
> -
> -#define SEC_DEF(sec_pfx, ptype, ...) {					    \
> +#define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {			    \
>  	.sec = sec_pfx,							    \
>  	.prog_type = BPF_PROG_TYPE_##ptype,				    \
> +	.expected_attach_type = atype,					    \
> +	.cookie = (long)(flags),					    \
>  	.preload_fn = libbpf_preload_prog,				    \
>  	__VA_ARGS__							    \
>  }
> @@ -7996,92 +8002,49 @@ static struct bpf_link *attach_lsm(const struct bpf_program *prog, long cookie);
>  static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie);
>  
>  static const struct bpf_sec_def section_defs[] = {
> -	BPF_PROG_SEC("socket",			BPF_PROG_TYPE_SOCKET_FILTER),
> +	SEC_DEF("socket",		SOCKET_FILTER, 0, SEC_NONE),

Didn't know how strictly you felt about checkpatch line-length complaints,
won't comment on them further since you mentioned 100 chars being the new
standard. But would complain about the alignment here and elsewhere in 
changes to section_defs even if checkpatch didn't exist :)

>  	BPF_EAPROG_SEC("sk_reuseport/migrate",	BPF_PROG_TYPE_SK_REUSEPORT,
>  						BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
>  	BPF_EAPROG_SEC("sk_reuseport",		BPF_PROG_TYPE_SK_REUSEPORT,
>  						BPF_SK_REUSEPORT_SELECT),

[...]