Received-SPF: pass (google.com: domain of linux-kernel+bounces-17389-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: i6a694271:Fastmail
Date: Thu, 4 Jan 2024 18:23:58 -0700
From: Daniel Xu <dxu@dxuuu.xyz>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Jiri Olsa <olsajiri@gmail.com>, 
	Martin KaFai Lau <martin.lau@linux.dev>, Andrii Nakryiko <andrii@kernel.org>, 
	Daniel Borkmann <daniel@iogearbox.net>, Alexei Starovoitov <ast@kernel.org>, 
	Quentin Monnet <quentin@isovalent.com>, Alan Maguire <alan.maguire@oracle.com>, 
	Kumar Kartikeya Dwivedi <memxor@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, John Fastabend <john.fastabend@gmail.com>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next 1/2] bpf: btf: Support optional flags for
 BTF_SET8 sets
Message-ID: <dpjloyytomne6efi76qe4mmh3mdjp67penogwbak7ojznf23wc@5gs6o7gc2gdt>
References: <cover.1704324602.git.dxu@dxuuu.xyz>
 <29644dc7906c7c0e6843d8acf92c3e29089845d0.1704324602.git.dxu@dxuuu.xyz>
 <ZZaVFxMvmMjbOlra@krava>
 <CAADnVQKZZw-e12-BOJsMMiA3s+vOskQEYRqhviQC2rBMf4AckA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAADnVQKZZw-e12-BOJsMMiA3s+vOskQEYRqhviQC2rBMf4AckA@mail.gmail.com>

On Thu, Jan 04, 2024 at 09:11:56AM -0800, Alexei Starovoitov wrote:
> On Thu, Jan 4, 2024 at 3:23 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Jan 03, 2024 at 04:31:55PM -0700, Daniel Xu wrote:
> > > This commit adds support for optional flags on BTF_SET8s.
> > > struct btf_id_set8 already supported 32 bits worth of flags, but was
> > > only used for alignment purposes before.
> > >
> > > We now use these bits to encode flags. The next commit will tag all
> > > kfunc sets with a flag so that pahole can recognize which
> > > BTF_ID_FLAGS(func, ..) are actual kfuncs.
> > >
> > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > ---
> > >  include/linux/btf_ids.h | 14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > > index a9cb10b0e2e9..88f914579fa1 100644
> > > --- a/include/linux/btf_ids.h
> > > +++ b/include/linux/btf_ids.h
> > > @@ -183,17 +183,21 @@ extern struct btf_id_set name;
> > >   * .word (1 << 3) | (1 << 1) | (1 << 2)
> > >   *
> > >   */
> > > -#define __BTF_SET8_START(name, scope)                        \
> > > +#define ___BTF_SET8_START(name, scope, flags)                \
> > >  asm(                                                 \
> > >  ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"   \
> > >  "." #scope " __BTF_ID__set8__" #name ";        \n"   \
> > >  "__BTF_ID__set8__" #name ":;                   \n"   \
> > > -".zero 8                                       \n"   \
> > > +".zero 4                                       \n"   \
> > > +".long " #flags                               "\n"   \
> > >  ".popsection;                                  \n");
> > >
> > > -#define BTF_SET8_START(name)                         \
> > > +#define __BTF_SET8_START(name, scope, flags, ...)    \
> > > +___BTF_SET8_START(name, scope, flags)
> > > +
> > > +#define BTF_SET8_START(name, ...)                    \
> > >  __BTF_ID_LIST(name, local)                           \
> > > -__BTF_SET8_START(name, local)
> > > +__BTF_SET8_START(name, local, ##__VA_ARGS__, 0)
> >
> > I think it'd better to use something like:
> >
> >   BTF_SET8_KFUNCS_START(fsverity_set_ids)
> >
> > instead of:
> >
> >   BTF_SET8_START(fsverity_set_ids, BTF_SET8_KFUNC)
> >
> > and to keep current BTF_SET8_START without flags argument, like:
> >
> >   #define BTF_SET8_START(name) \
> >     __BTF_SET8_START(... , 0, ...
> >
> >   #define BTF_SET8_KFUNCS_START(name) \
> >     __BTF_SET8_START(... , BTF_SET8_KFUNC, ...
> 
> I was about to suggest the same :)
> 
> We can drop SET8 part as well, since it's implementation detail.
> Just BTF_KFUNCS_START and pair it with BTF_KFUNCS_END
> that will be the same as BTF_SET8_END.
> Until we need to do something else with these macros.

Ack, will change.

> 
> >
> > also I'd rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS (with S)
> >
> > do you have the pahole changes somewhere? would be great to
> > see all the related changes and try the whole thing
> 
> +1
> without corresponding pahole changes it's not clear whether
> it actually helps.

Here's a checkpointed branch: https://github.com/danobi/pahole/tree/kfunc_btf-mailed .
I won't force push to it.

It should work against this patchset. I might need to clean it up a bit still.

Thanks,
Daniel