On Tue, Oct 31, 2023 at 10:04 AM David Marchevsky
<david.marchevsky@xxxxxxxxx> wrote:
>
> On 10/31/23 2:51 AM, Yafang Shao wrote:
> > On Tue, Oct 31, 2023 at 2:23 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> >>
> >> On Mon, Oct 30, 2023 at 10:56 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> >>>
> >>> On Tue, Oct 31, 2023 at 5:07 AM Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
> >>>>
> >>>> BPF kfuncs are meant to be called from BPF programs. Accordingly, most
> >>>> kfuncs are not called from anywhere in the kernel, which the
> >>>> -Wmissing-prototypes warning is unhappy about. We've peppered
> >>>> __diag_ignore_all("-Wmissing-prototypes", ... everywhere kfuncs are
> >>>> defined in the codebase to suppress this warning.
> >>>>
> >>>> This patch adds two macros meant to bound one or many kfunc definitions.
> >>>> All existing kfunc definitions which use these __diag calls to suppress
> >>>> -Wmissing-prototypes are migrated to use the newly-introduced macros.
> >>>> A new __diag_ignore_all - for "-Wmissing-declarations" - is added to the
> >>>> __bpf_kfunc_start_defs macro based on feedback from Andrii on an earlier
> >>>> version of this patch [0] and another recent mailing list thread [1].
> >>>>
> >>>> In the future we might need to ignore different warnings or do other
> >>>> kfunc-specific things. This change will make it easier to make such
> >>>> modifications for all kfunc defs.
> >>>>
> >>>> [0]: https://lore.kernel.org/bpf/CAEf4BzaE5dRWtK6RPLnjTW-MW9sx9K3Fn6uwqCTChK2Dcb1Xig@xxxxxxxxxxxxxx/
> >>>> [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/
> >>>>
> >>>> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> >>>> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >>>> Cc: Jiri Olsa <olsajiri@xxxxxxxxx>
> >>>> ---
> >>>>
> >>>> This patch was submitted earlier as part of task_vma
> >>>> iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@xxxxxx/
> >>>>
> >>>> This separate submission addresses Andrii's comments from
> >>>> that thread.
> >>>>
> >>>> include/linux/btf.h | 9 +++++++++
> >>>> kernel/bpf/bpf_iter.c | 6 ++----
> >>>> kernel/bpf/cpumask.c | 6 ++----
> >>>> kernel/bpf/helpers.c | 6 ++----
> >>>> kernel/bpf/map_iter.c | 6 ++----
> >>>> kernel/bpf/task_iter.c | 6 ++----
> >>>> kernel/trace/bpf_trace.c | 6 ++----
> >>>> net/bpf/test_run.c | 7 +++----
> >>>> net/core/filter.c | 13 ++++---------
> >>>> net/core/xdp.c | 6 ++----
> >>>> net/ipv4/fou_bpf.c | 6 ++----
> >>>> net/netfilter/nf_conntrack_bpf.c | 6 ++----
> >>>> net/netfilter/nf_nat_bpf.c | 6 ++----
> >>>> net/xfrm/xfrm_interface_bpf.c | 6 ++----
> >>>> 14 files changed, 38 insertions(+), 57 deletions(-)
> >>>>
> >>>
> >>> Thanks for your work.
> >>>
> >>> By using a simple grep for "__diag_ignore_all(\"-Wmissing-prototypes",
> >>> it appears that the files net/socket.c,
> >>> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c,
> >>> kernel/cgroup/rstat.c and Documentation/bpf/kfuncs.rst are missing. It
> >>> seems that we should also update them.
> >>>
> >>
> >> rstat.c and net/socket.c don't have kfuncs, so those are not relevant
> >> here.
> >
> > The bpf_rstat_flush() and update_socket_protocol() can also trigger
> > the -Wmissing-declarations.
> > These two functions are for BPF only. Shouldn't we better include them as well ?
> >
>
> I had this conundrum when writing the patch as well. Since they're not kfuncs
> and the macros are meant to wrap kfunc definitions, I felt that it would be
yeah, I was going to say the same, it's misleading
> confusing to someone unfamiliar with BPF internals. But I agree that the current
> state isn't ideal either.
>
> How about either:
> * I use the __bpf_kfunc_{start,end}_defs macros in those two places,
> with comment describing that they're not wrapping kfunc def, but rather
> BPF hook point that throws the same warnings.
> * Two additional macros, __bpf_hook_{start,end} are added, just
> pointing to __bpf_kfunc_{start,end} for now. They're used for
> these two functions
>
> WDYT?
I like the option #2 best
>
> >> But we are missing changes also in kernel/bpf/task_iter.c and
> >> kernel/bpf/cgroup_iter.c
> >>
> >> And let's update Documentation/bpf/kfuncs.rst to use your new set of macros?
> >>
> >> With the above addressed, please add my ack. Thanks!
> >>
> >> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >>
> >>> --
> >>> Regards
> >>> Yafang
> >
> >
> >
