| Message ID | 20230106195130.1216841-2-void@manifault.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Annotate kfuncs with new __bpf_kfunc macro | expand |
On Fri, Jan 6, 2023 at 11:51 AM David Vernet <void@manifault.com> wrote: > > kfuncs are functions defined in the kernel, which may be invoked by BPF > programs. They may or may not also be used as regular kernel functions, > implying that they may be static (in which case the compiler could e.g. > inline it away), or it could have external linkage, but potentially be > elided in an LTO build if a function is observed to never be used, and > is stripped from the final kernel binary. > > We therefore require some convenience macro that kfunc developers can > use just add to their kfuncs, and which will prevent all of the above > issues from happening. This is in contrast with what we have today, > where some kfunc definitions have "noinline", some have "__used", and > others are static and have neither. > > In addition to providing the obvious correctness benefits, having such a > macro / tag also provides the following advantages: > > - Giving an easy and intuitive thing to query for if people are looking > for kfuncs, as Christoph suggested at the kernel maintainers summit > (https://lwn.net/Articles/908464/). This is currently possible by > grepping for BTF_ID_FLAGS(func, but having something more self > describing would be useful as well. > > - In the future, the tag can be expanded with other useful things such > as the ability to suppress -Wmissing-prototype for the kfuncs rather > than requiring developers to surround the kfunc with __diags to > suppress the warning (this requires compiler support that as far as I > know currently does not exist). Have you considered doing bpf_kfunc_start/bpf_kfunc_end ? The former would include: __diag_push(); __diag_ignore_all(); __used noinline Also how about using bpf_kfunc on the same line ? Then 'git grep' will be easier.
On Fri, Jan 06, 2023 at 05:04:02PM -0800, Alexei Starovoitov wrote: > On Fri, Jan 6, 2023 at 11:51 AM David Vernet <void@manifault.com> wrote: > > > > kfuncs are functions defined in the kernel, which may be invoked by BPF > > programs. They may or may not also be used as regular kernel functions, > > implying that they may be static (in which case the compiler could e.g. > > inline it away), or it could have external linkage, but potentially be > > elided in an LTO build if a function is observed to never be used, and > > is stripped from the final kernel binary. > > > > We therefore require some convenience macro that kfunc developers can > > use just add to their kfuncs, and which will prevent all of the above > > issues from happening. This is in contrast with what we have today, > > where some kfunc definitions have "noinline", some have "__used", and > > others are static and have neither. > > > > In addition to providing the obvious correctness benefits, having such a > > macro / tag also provides the following advantages: > > > > - Giving an easy and intuitive thing to query for if people are looking > > for kfuncs, as Christoph suggested at the kernel maintainers summit > > (https://lwn.net/Articles/908464/). This is currently possible by > > grepping for BTF_ID_FLAGS(func, but having something more self > > describing would be useful as well. > > > > - In the future, the tag can be expanded with other useful things such > > as the ability to suppress -Wmissing-prototype for the kfuncs rather > > than requiring developers to surround the kfunc with __diags to > > suppress the warning (this requires compiler support that as far as I > > know currently does not exist). > > Have you considered doing bpf_kfunc_start/bpf_kfunc_end ? > The former would include: > __diag_push(); __diag_ignore_all(); __used noinline Yeah that's certainly an option. The downside is that all functions within scope of the __diag_push() will be affected, and sometimes we mix kfuncs with non-kfuncs (including e.g. static helper functions that are used by the kfuncs themselves). -Wmissing-prototypes isn't a big deal, but __used and noinline are kind of unfortunate. Not a big deal though, it'll just result in a few extra __bpf_kfuncs_start() and __bpf_kfuncs_end() sprinkled throughout to avoid them being included. The upside is of course that we can get rid of the __diag_push()'es we currently have to prevent -Wmissing-prototypes. Wdyt? I do like the idea of getting rid of those ugly __diag_push()'es. And we could always go back to using a __bpf_kfunc macro if and when compilers ever support using attributes to ignore warnings for specific functions. > > Also how about using bpf_kfunc on the same line ? > Then 'git grep' will be easier. Sure, if we keep this approach I'll do this in v2.
On Fri, Jan 6, 2023 at 6:09 PM David Vernet <void@manifault.com> wrote: > > On Fri, Jan 06, 2023 at 05:04:02PM -0800, Alexei Starovoitov wrote: > > On Fri, Jan 6, 2023 at 11:51 AM David Vernet <void@manifault.com> wrote: > > > > > > kfuncs are functions defined in the kernel, which may be invoked by BPF > > > programs. They may or may not also be used as regular kernel functions, > > > implying that they may be static (in which case the compiler could e.g. > > > inline it away), or it could have external linkage, but potentially be > > > elided in an LTO build if a function is observed to never be used, and > > > is stripped from the final kernel binary. > > > > > > We therefore require some convenience macro that kfunc developers can > > > use just add to their kfuncs, and which will prevent all of the above > > > issues from happening. This is in contrast with what we have today, > > > where some kfunc definitions have "noinline", some have "__used", and > > > others are static and have neither. > > > > > > In addition to providing the obvious correctness benefits, having such a > > > macro / tag also provides the following advantages: > > > > > > - Giving an easy and intuitive thing to query for if people are looking > > > for kfuncs, as Christoph suggested at the kernel maintainers summit > > > (https://lwn.net/Articles/908464/). This is currently possible by > > > grepping for BTF_ID_FLAGS(func, but having something more self > > > describing would be useful as well. > > > > > > - In the future, the tag can be expanded with other useful things such > > > as the ability to suppress -Wmissing-prototype for the kfuncs rather > > > than requiring developers to surround the kfunc with __diags to > > > suppress the warning (this requires compiler support that as far as I > > > know currently does not exist). > > > > Have you considered doing bpf_kfunc_start/bpf_kfunc_end ? > > The former would include: > > __diag_push(); __diag_ignore_all(); __used noinline > > Yeah that's certainly an option. The downside is that all functions > within scope of the __diag_push() will be affected, and sometimes we mix > kfuncs with non-kfuncs (including e.g. static helper functions that are > used by the kfuncs themselves). -Wmissing-prototypes isn't a big deal, > but __used and noinline are kind of unfortunate. Not a big deal though, > it'll just result in a few extra __bpf_kfuncs_start() and > __bpf_kfuncs_end() sprinkled throughout to avoid them being included. > The upside is of course that we can get rid of the __diag_push()'es we > currently have to prevent -Wmissing-prototypes. I meant to use bpf_kfunc_start/bpf_kfunc_end around every kfunc. Ideally bpf_kfunc_start would be on the same line as func proto for nice grepping. Maybe it's an overkill. Maybe 3 macroses then? bpf_kfunc_start to hide __diag bpf_kfunc on the proto line bpf_kfunc_end to finish __diag_pop > Wdyt? I do like the idea of getting rid of those ugly __diag_push()'es. > And we could always go back to using a __bpf_kfunc macro if and when > compilers ever support using attributes to ignore warnings for specific > functions. > > > > > Also how about using bpf_kfunc on the same line ? > > Then 'git grep' will be easier. > > Sure, if we keep this approach I'll do this in v2.
On Mon, Jan 09, 2023 at 04:47:54AM IST, Alexei Starovoitov wrote: > On Fri, Jan 6, 2023 at 6:09 PM David Vernet <void@manifault.com> wrote: > > > > On Fri, Jan 06, 2023 at 05:04:02PM -0800, Alexei Starovoitov wrote: > > > On Fri, Jan 6, 2023 at 11:51 AM David Vernet <void@manifault.com> wrote: > > > > > > > > kfuncs are functions defined in the kernel, which may be invoked by BPF > > > > programs. They may or may not also be used as regular kernel functions, > > > > implying that they may be static (in which case the compiler could e.g. > > > > inline it away), or it could have external linkage, but potentially be > > > > elided in an LTO build if a function is observed to never be used, and > > > > is stripped from the final kernel binary. > > > > > > > > We therefore require some convenience macro that kfunc developers can > > > > use just add to their kfuncs, and which will prevent all of the above > > > > issues from happening. This is in contrast with what we have today, > > > > where some kfunc definitions have "noinline", some have "__used", and > > > > others are static and have neither. > > > > > > > > In addition to providing the obvious correctness benefits, having such a > > > > macro / tag also provides the following advantages: > > > > > > > > - Giving an easy and intuitive thing to query for if people are looking > > > > for kfuncs, as Christoph suggested at the kernel maintainers summit > > > > (https://lwn.net/Articles/908464/). This is currently possible by > > > > grepping for BTF_ID_FLAGS(func, but having something more self > > > > describing would be useful as well. > > > > > > > > - In the future, the tag can be expanded with other useful things such > > > > as the ability to suppress -Wmissing-prototype for the kfuncs rather > > > > than requiring developers to surround the kfunc with __diags to > > > > suppress the warning (this requires compiler support that as far as I > > > > know currently does not exist). > > > > > > Have you considered doing bpf_kfunc_start/bpf_kfunc_end ? > > > The former would include: > > > __diag_push(); __diag_ignore_all(); __used noinline > > > > Yeah that's certainly an option. The downside is that all functions > > within scope of the __diag_push() will be affected, and sometimes we mix > > kfuncs with non-kfuncs (including e.g. static helper functions that are > > used by the kfuncs themselves). -Wmissing-prototypes isn't a big deal, > > but __used and noinline are kind of unfortunate. Not a big deal though, > > it'll just result in a few extra __bpf_kfuncs_start() and > > __bpf_kfuncs_end() sprinkled throughout to avoid them being included. > > The upside is of course that we can get rid of the __diag_push()'es we > > currently have to prevent -Wmissing-prototypes. > > I meant to use bpf_kfunc_start/bpf_kfunc_end around every kfunc. > Ideally bpf_kfunc_start would be on the same line as func proto > for nice grepping. > Maybe it's an overkill. > Maybe 3 macroses then? > bpf_kfunc_start to hide __diag > bpf_kfunc on the proto line > bpf_kfunc_end to finish __diag_pop > There's also the option of doing this: #define BPF_KFUNC(proto) proto; __used noinline proto BPF_KFUNC(void kfunc(arg1, arg2)) { ... } No need to disable the warning with diag push/pop, just put a declaration before the definition to silence the compiler. The only awkward part is entire function prototype becoming a macro argument (unlike the common case void MACRO(...)) but it becomes less noisy and easy to grep as well.
On Mon, Jan 09, 2023 at 05:38:15PM +0530, Kumar Kartikeya Dwivedi wrote: > On Mon, Jan 09, 2023 at 04:47:54AM IST, Alexei Starovoitov wrote: > > On Fri, Jan 6, 2023 at 6:09 PM David Vernet <void@manifault.com> wrote: > > > > > > On Fri, Jan 06, 2023 at 05:04:02PM -0800, Alexei Starovoitov wrote: > > > > On Fri, Jan 6, 2023 at 11:51 AM David Vernet <void@manifault.com> wrote: > > > > > > > > > > kfuncs are functions defined in the kernel, which may be invoked by BPF > > > > > programs. They may or may not also be used as regular kernel functions, > > > > > implying that they may be static (in which case the compiler could e.g. > > > > > inline it away), or it could have external linkage, but potentially be > > > > > elided in an LTO build if a function is observed to never be used, and > > > > > is stripped from the final kernel binary. > > > > > > > > > > We therefore require some convenience macro that kfunc developers can > > > > > use just add to their kfuncs, and which will prevent all of the above > > > > > issues from happening. This is in contrast with what we have today, > > > > > where some kfunc definitions have "noinline", some have "__used", and > > > > > others are static and have neither. > > > > > > > > > > In addition to providing the obvious correctness benefits, having such a > > > > > macro / tag also provides the following advantages: > > > > > > > > > > - Giving an easy and intuitive thing to query for if people are looking > > > > > for kfuncs, as Christoph suggested at the kernel maintainers summit > > > > > (https://lwn.net/Articles/908464/). This is currently possible by > > > > > grepping for BTF_ID_FLAGS(func, but having something more self > > > > > describing would be useful as well. > > > > > > > > > > - In the future, the tag can be expanded with other useful things such > > > > > as the ability to suppress -Wmissing-prototype for the kfuncs rather > > > > > than requiring developers to surround the kfunc with __diags to > > > > > suppress the warning (this requires compiler support that as far as I > > > > > know currently does not exist). > > > > > > > > Have you considered doing bpf_kfunc_start/bpf_kfunc_end ? > > > > The former would include: > > > > __diag_push(); __diag_ignore_all(); __used noinline > > > > > > Yeah that's certainly an option. The downside is that all functions > > > within scope of the __diag_push() will be affected, and sometimes we mix > > > kfuncs with non-kfuncs (including e.g. static helper functions that are > > > used by the kfuncs themselves). -Wmissing-prototypes isn't a big deal, > > > but __used and noinline are kind of unfortunate. Not a big deal though, > > > it'll just result in a few extra __bpf_kfuncs_start() and > > > __bpf_kfuncs_end() sprinkled throughout to avoid them being included. > > > The upside is of course that we can get rid of the __diag_push()'es we > > > currently have to prevent -Wmissing-prototypes. > > > > I meant to use bpf_kfunc_start/bpf_kfunc_end around every kfunc. > > Ideally bpf_kfunc_start would be on the same line as func proto > > for nice grepping. > > Maybe it's an overkill. > > Maybe 3 macroses then? > > bpf_kfunc_start to hide __diag > > bpf_kfunc on the proto line > > bpf_kfunc_end to finish __diag_pop Ah, I see. Hmm, I guess this is better than what we have now, but is still a lot of macros and boilerplate which IMO is a sign we're not going in quite the right direction. I don't really have a better suggestion at this point, though I do like Kumar's suggestion below. > There's also the option of doing this: > > #define BPF_KFUNC(proto) proto; __used noinline proto > > BPF_KFUNC(void kfunc(arg1, arg2)) { > ... > } > > No need to disable the warning with diag push/pop, just put a declaration before > the definition to silence the compiler. The only awkward part is entire function > prototype becoming a macro argument (unlike the common case void MACRO(...)) but > it becomes less noisy and easy to grep as well. If nobody would come after us with pitchforks for this, IMO this is the most user-friendly option and what I would vote for. It doesn't seem like this is violating anything in [0]? [0]: https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
On Mon, Jan 9, 2023 at 9:05 AM David Vernet <void@manifault.com> wrote: > > > Maybe 3 macroses then? > > > bpf_kfunc_start to hide __diag > > > bpf_kfunc on the proto line > > > bpf_kfunc_end to finish __diag_pop > > Ah, I see. Hmm, I guess this is better than what we have now, but is > still a lot of macros and boilerplate which IMO is a sign we're not > going in quite the right direction. I don't really have a better > suggestion at this point, though I do like Kumar's suggestion below. > > > There's also the option of doing this: > > > > #define BPF_KFUNC(proto) proto; __used noinline proto > > > > BPF_KFUNC(void kfunc(arg1, arg2)) { > > ... > > } Fine by me. Just put { on the new line.
diff --git a/include/linux/btf.h b/include/linux/btf.h index 5f628f323442..ff62fa63dc19 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -72,6 +72,14 @@ #define KF_DESTRUCTIVE (1 << 6) /* kfunc performs destructive actions */ #define KF_RCU (1 << 7) /* kfunc only takes rcu pointer arguments */ +/* + * Tag marking a kernel function as a kfunc. This is meant to minimize the + * amount of copy-paste that kfunc authors have to include for correctness so + * as to avoid issues such as the compiler inlining or eliding either a static + * kfunc, or a global kfunc in an LTO build. + */ +#define __bpf_kfunc __used noinline + /* * Return the name of the passed struct, if exists, or halt the build if for * example the structure gets renamed. In this way, developers have to revisit
kfuncs are functions defined in the kernel, which may be invoked by BPF programs. They may or may not also be used as regular kernel functions, implying that they may be static (in which case the compiler could e.g. inline it away), or it could have external linkage, but potentially be elided in an LTO build if a function is observed to never be used, and is stripped from the final kernel binary. We therefore require some convenience macro that kfunc developers can use just add to their kfuncs, and which will prevent all of the above issues from happening. This is in contrast with what we have today, where some kfunc definitions have "noinline", some have "__used", and others are static and have neither. In addition to providing the obvious correctness benefits, having such a macro / tag also provides the following advantages: - Giving an easy and intuitive thing to query for if people are looking for kfuncs, as Christoph suggested at the kernel maintainers summit (https://lwn.net/Articles/908464/). This is currently possible by grepping for BTF_ID_FLAGS(func, but having something more self describing would be useful as well. - In the future, the tag can be expanded with other useful things such as the ability to suppress -Wmissing-prototype for the kfuncs rather than requiring developers to surround the kfunc with __diags to suppress the warning (this requires compiler support that as far as I know currently does not exist). Note that checkpatch complains about this patch with the following: ERROR: Macros with complex values should be enclosed in parentheses +#define __bpf_kfunc __used noinline There seems to be a precedent for using this pattern in other places such as compiler_types.h (see e.g. __randomize_layout and noinstr), so it seems appropriate. Signed-off-by: David Vernet <void@manifault.com> --- include/linux/btf.h | 8 ++++++++ 1 file changed, 8 insertions(+)