Re: [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC

From: Dave Marchevsky <davemarchevsky@fb.com>
To: Andrii Nakryiko <andrii@kernel.org>, <bpf@vger.kernel.org>,
	<ast@kernel.org>, <daniel@iogearbox.net>
Cc: <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 7/9] libbpf: complete SEC() table unification for BPF_APROG_SEC/BPF_EAPROG_SEC
Date: Tue, 21 Sep 2021 21:42:26 -0400	[thread overview]
Message-ID: <c4d6fa1a-182b-8cfa-7a53-374f97af4ecd@fb.com> (raw)
In-Reply-To: <20210920234320.3312820-8-andrii@kernel.org>

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Complete SEC() table refactoring towards unified form by rewriting
> BPF_APROG_SEC and BPF_EAPROG_SEC definitions with
> SEC_DEF(SEC_ATTACHABLE_OPT) (for optional expected_attach_type) and
> SEC_DEF(SEC_ATTACHABLE) (mandatory expected_attach_type), respectively.
> Drop BPF_APROG_SEC, BPF_EAPROG_SEC, and BPF_PROG_SEC_IMPL macros after
> that, leaving SEC_DEF() macro as the only one used.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/lib/bpf/libbpf.c | 136 +++++++++++------------------------------
>  1 file changed, 35 insertions(+), 101 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 734be7dc52a0..56082865ceff 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7959,32 +7959,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  	prog->expected_attach_type = type;
>  }
>  
> -#define BPF_PROG_SEC_IMPL(string, ptype, eatype, eatype_optional,	    \
> -			  attachable, attach_btf)			    \
> -	{								    \
> -		.sec = string,						    \
> -		.prog_type = ptype,					    \
> -		.expected_attach_type = eatype,				    \
> -		.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 be attached. */
> -#define BPF_APROG_SEC(string, ptype, atype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> -
> -/* Programs that must specify expected attach type at load time. */
> -#define BPF_EAPROG_SEC(string, ptype, eatype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 1, 0)
> -
> -/* Programs that use BTF to identify attach point */
> -#define BPF_PROG_BTF(string, ptype, eatype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
> -

Similar thoughts about comment usefulness as patch 6.

>  #define SEC_DEF(sec_pfx, ptype, atype, flags, ...) {			    \
>  	.sec = sec_pfx,							    \
>  	.prog_type = BPF_PROG_TYPE_##ptype,				    \
> @@ -8003,10 +7977,8 @@ static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie)
>  
>  static const struct bpf_sec_def section_defs[] = {
>  	SEC_DEF("socket",		SOCKET_FILTER, 0, SEC_NONE),
> -	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),
> +	SEC_DEF("sk_reuseport/migrate",	SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, SEC_ATTACHABLE),
> +	SEC_DEF("sk_reuseport",		SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT, SEC_ATTACHABLE),

Ah, I see that after this patch the alignment issue from patch 6 is better.
Nevermind then.

>  	SEC_DEF("kprobe/",		KPROBE,	0, SEC_NONE, attach_kprobe),
>  	SEC_DEF("uprobe/",		KPROBE,	0, SEC_NONE),
>  	SEC_DEF("kretprobe/",		KPROBE, 0, SEC_NONE, attach_kprobe),

[...]

> +	SEC_DEF("sk_skb/stream_parser",	SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
> +	SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),

checkpatch really doesn't like the lack of space after comma here, I agree
with it.

>  	SEC_DEF("sk_skb",		SK_SKB, 0, SEC_NONE),
> -	BPF_APROG_SEC("sk_msg",			BPF_PROG_TYPE_SK_MSG,
> -						BPF_SK_MSG_VERDICT),
> -	BPF_APROG_SEC("lirc_mode2",		BPF_PROG_TYPE_LIRC_MODE2,

[...]