Re: [PATCH v1 bpf-next] bpf: Add __bpf_kfunc_{start,end}_defs macros

[David Marchevsky <david.marchevsky@linux.dev>]

On 10/31/23 2:51 AM, Yafang Shao wrote:
> On Tue, Oct 31, 2023 at 2:23 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Mon, Oct 30, 2023 at 10:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>>>
>>> On Tue, Oct 31, 2023 at 5:07 AM Dave Marchevsky <davemarchevsky@fb.com> 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@mail.gmail.com/
>>>>   [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/
>>>>
>>>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>>>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>>>> Cc: Jiri Olsa <olsajiri@gmail.com>
>>>> ---
>>>>
>>>> This patch was submitted earlier as part of task_vma
>>>> iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@fb.com/
>>>>
>>>> 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
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? 

>> 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@kernel.org>
>>
>>> --
>>> Regards
>>> Yafang
> 
> 
>