| Message ID | 20230615142520.10280-1-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | [RFC,bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h | expand |
On Thu, Jun 15, 2023 at 7:25 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > Update code generation for bpf_helper_defs.h by adding > __attribute__((nomerge)) for a set of helper functions to prevent some > verifier unfriendly compiler optimizations. > > This addresses a recent mailing list thread [1]. > There Zhongqiu Duan and Yonghong Song discussed a C program as below: > > if (data_end - data > 1024) { > bpf_for_each_map_elem(&map1, cb, &cb_data, 0); > } else { > bpf_for_each_map_elem(&map2, cb, &cb_data, 0); > } > > Which was converted by clang to something like this: > > if (data_end - data > 1024) > tmp = &map1; > else > tmp = &map2; > bpf_for_each_map_elem(tmp, cb, &cb_data, 0); > > Which in turn triggered verification error, because > verifier.c:record_func_map() requires a single map address for each > bpf_for_each_map_elem() call. > > In fact, this is a requirement for the following helpers: > - bpf_tail_call > - bpf_map_lookup_elem > - bpf_map_update_elem > - bpf_map_delete_elem > - bpf_map_push_elem > - bpf_map_pop_elem > - bpf_map_peek_elem > - bpf_for_each_map_elem > - bpf_redirect_map > - bpf_map_lookup_percpu_elem > > I had an off-list discussion with Yonghong where we agreed that clang > attribute 'nomerge' (see [2]) could be used to prevent the > optimization hitting in [1]. However, currently 'nomerge' applies only > to functions and statements, hence I submitted change requests [3], > [4] to allow specifying 'nomerge' for function pointers as well. > > The patch below updates bpf_helper_defs.h generation by adding a > definition of __nomerge macro, and using this macro in definitions of > relevant helpers. > > The generated code looks as follows: > > /* This is auto-generated file. See bpf_doc.py for details. */ > > #if __has_attribute(nomerge) > #define __nomerge __attribute__((nomerge)) > #else > #define __nomerge > #endif > > /* Forward declarations of BPF structs */ > ... > static long (*bpf_for_each_map_elem)(void *map, ...) __nomerge = (void *) 164; > ... > > (In non-RFC version the macro definition would have to be updated to > check for supported clang version). > > Does community agree with such approach? Makes sense to me. Let's just be very careful to do proper detection of __nomerge "applicability" to ensure we don't cause compilation errors for unsupported Clang (which I'm sure you are well aware of) *and* make it compatible with GCC, so we don't fix it later. > > [1] https://lore.kernel.org/bpf/03bdf90f-f374-1e67-69d6-76dd9c8318a4@meta.com/ > [2] https://clang.llvm.org/docs/AttributeReference.html#nomerge > [3] https://reviews.llvm.org/D152986 > [4] https://reviews.llvm.org/D152987 > --- > scripts/bpf_doc.py | 37 ++++++++++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 7 deletions(-) > > diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py > index eaae2ce78381..dbd4893c793e 100755 > --- a/scripts/bpf_doc.py > +++ b/scripts/bpf_doc.py > @@ -777,14 +777,33 @@ class PrinterHelpers(Printer): > 'bpf_get_socket_cookie', > 'bpf_sk_assign', > ] > + # Helpers that need __nomerge attribute > + nomerge_helpers = set([ > + "bpf_tail_call", > + "bpf_map_lookup_elem", > + "bpf_map_update_elem", > + "bpf_map_delete_elem", > + "bpf_map_push_elem", > + "bpf_map_pop_elem", > + "bpf_map_peek_elem", > + "bpf_for_each_map_elem", > + "bpf_redirect_map", > + "bpf_map_lookup_percpu_elem" > + ]) > + > + macros = '''\ > +#if __has_attribute(nomerge) > +#define __nomerge __attribute__((nomerge)) > +#else > +#define __nomerge > +#endif''' > > def print_header(self): > - header = '''\ > -/* This is auto-generated file. See bpf_doc.py for details. */ > - > -/* Forward declarations of BPF structs */''' > - > - print(header) > + print('/* This is auto-generated file. See bpf_doc.py for details. */') > + print() > + print(self.macros) > + print() > + print('/* Forward declarations of BPF structs */') > for fwd in self.type_fwds: > print('%s;' % fwd) > print('') > @@ -846,7 +865,11 @@ class PrinterHelpers(Printer): > comma = ', ' > print(one_arg, end='') > > - print(') = (void *) %d;' % helper.enum_val) > + print(')', end='') > + if proto['name'] in self.nomerge_helpers: > + print(' __nomerge', end='') > + > + print(' = (void *) %d;' % helper.enum_val) > print('') > > ############################################################################### > -- > 2.40.1 >
> On Thu, Jun 15, 2023 at 7:25 AM Eduard Zingerman <eddyz87@gmail.com> wrote: >> >> Update code generation for bpf_helper_defs.h by adding >> __attribute__((nomerge)) for a set of helper functions to prevent some >> verifier unfriendly compiler optimizations. >> >> This addresses a recent mailing list thread [1]. >> There Zhongqiu Duan and Yonghong Song discussed a C program as below: >> >> if (data_end - data > 1024) { >> bpf_for_each_map_elem(&map1, cb, &cb_data, 0); >> } else { >> bpf_for_each_map_elem(&map2, cb, &cb_data, 0); >> } >> >> Which was converted by clang to something like this: >> >> if (data_end - data > 1024) >> tmp = &map1; >> else >> tmp = &map2; >> bpf_for_each_map_elem(tmp, cb, &cb_data, 0); >> >> Which in turn triggered verification error, because >> verifier.c:record_func_map() requires a single map address for each >> bpf_for_each_map_elem() call. >> >> In fact, this is a requirement for the following helpers: >> - bpf_tail_call >> - bpf_map_lookup_elem >> - bpf_map_update_elem >> - bpf_map_delete_elem >> - bpf_map_push_elem >> - bpf_map_pop_elem >> - bpf_map_peek_elem >> - bpf_for_each_map_elem >> - bpf_redirect_map >> - bpf_map_lookup_percpu_elem >> >> I had an off-list discussion with Yonghong where we agreed that clang >> attribute 'nomerge' (see [2]) could be used to prevent the >> optimization hitting in [1]. However, currently 'nomerge' applies only >> to functions and statements, hence I submitted change requests [3], >> [4] to allow specifying 'nomerge' for function pointers as well. >> >> The patch below updates bpf_helper_defs.h generation by adding a >> definition of __nomerge macro, and using this macro in definitions of >> relevant helpers. >> >> The generated code looks as follows: >> >> /* This is auto-generated file. See bpf_doc.py for details. */ >> >> #if __has_attribute(nomerge) >> #define __nomerge __attribute__((nomerge)) >> #else >> #define __nomerge >> #endif >> >> /* Forward declarations of BPF structs */ >> ... >> static long (*bpf_for_each_map_elem)(void *map, ...) __nomerge = (void *) 164; >> ... >> >> (In non-RFC version the macro definition would have to be updated to >> check for supported clang version). >> >> Does community agree with such approach? > > Makes sense to me. Let's just be very careful to do proper detection > of __nomerge "applicability" to ensure we don't cause compilation > errors for unsupported Clang (which I'm sure you are well aware of) > *and* make it compatible with GCC, so we don't fix it later. GCC doesn't support the "nomerge" attribute at this point. We will look into adding it or find some other equivalent mechanism that can be abstracted in the __nomerge macro. >> >> [1] https://lore.kernel.org/bpf/03bdf90f-f374-1e67-69d6-76dd9c8318a4@meta.com/ >> [2] https://clang.llvm.org/docs/AttributeReference.html#nomerge >> [3] https://reviews.llvm.org/D152986 >> [4] https://reviews.llvm.org/D152987 >> --- >> scripts/bpf_doc.py | 37 ++++++++++++++++++++++++++++++------- >> 1 file changed, 30 insertions(+), 7 deletions(-) >> >> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py >> index eaae2ce78381..dbd4893c793e 100755 >> --- a/scripts/bpf_doc.py >> +++ b/scripts/bpf_doc.py >> @@ -777,14 +777,33 @@ class PrinterHelpers(Printer): >> 'bpf_get_socket_cookie', >> 'bpf_sk_assign', >> ] >> + # Helpers that need __nomerge attribute >> + nomerge_helpers = set([ >> + "bpf_tail_call", >> + "bpf_map_lookup_elem", >> + "bpf_map_update_elem", >> + "bpf_map_delete_elem", >> + "bpf_map_push_elem", >> + "bpf_map_pop_elem", >> + "bpf_map_peek_elem", >> + "bpf_for_each_map_elem", >> + "bpf_redirect_map", >> + "bpf_map_lookup_percpu_elem" >> + ]) >> + >> + macros = '''\ >> +#if __has_attribute(nomerge) >> +#define __nomerge __attribute__((nomerge)) >> +#else >> +#define __nomerge >> +#endif''' >> >> def print_header(self): >> - header = '''\ >> -/* This is auto-generated file. See bpf_doc.py for details. */ >> - >> -/* Forward declarations of BPF structs */''' >> - >> - print(header) >> + print('/* This is auto-generated file. See bpf_doc.py for details. */') >> + print() >> + print(self.macros) >> + print() >> + print('/* Forward declarations of BPF structs */') >> for fwd in self.type_fwds: >> print('%s;' % fwd) >> print('') >> @@ -846,7 +865,11 @@ class PrinterHelpers(Printer): >> comma = ', ' >> print(one_arg, end='') >> >> - print(') = (void *) %d;' % helper.enum_val) >> + print(')', end='') >> + if proto['name'] in self.nomerge_helpers: >> + print(' __nomerge', end='') >> + >> + print(' = (void *) %d;' % helper.enum_val) >> print('') >> >> ############################################################################### >> -- >> 2.40.1 >>
On Tue, Jun 20, 2023 at 11:27 AM Jose E. Marchesi <jemarch@gnu.org> wrote: > > > > On Thu, Jun 15, 2023 at 7:25 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > >> > >> Update code generation for bpf_helper_defs.h by adding > >> __attribute__((nomerge)) for a set of helper functions to prevent some > >> verifier unfriendly compiler optimizations. > >> > >> This addresses a recent mailing list thread [1]. > >> There Zhongqiu Duan and Yonghong Song discussed a C program as below: > >> > >> if (data_end - data > 1024) { > >> bpf_for_each_map_elem(&map1, cb, &cb_data, 0); > >> } else { > >> bpf_for_each_map_elem(&map2, cb, &cb_data, 0); > >> } > >> > >> Which was converted by clang to something like this: > >> > >> if (data_end - data > 1024) > >> tmp = &map1; > >> else > >> tmp = &map2; > >> bpf_for_each_map_elem(tmp, cb, &cb_data, 0); > >> > >> Which in turn triggered verification error, because > >> verifier.c:record_func_map() requires a single map address for each > >> bpf_for_each_map_elem() call. > >> > >> In fact, this is a requirement for the following helpers: > >> - bpf_tail_call > >> - bpf_map_lookup_elem > >> - bpf_map_update_elem > >> - bpf_map_delete_elem > >> - bpf_map_push_elem > >> - bpf_map_pop_elem > >> - bpf_map_peek_elem > >> - bpf_for_each_map_elem > >> - bpf_redirect_map > >> - bpf_map_lookup_percpu_elem > >> > >> I had an off-list discussion with Yonghong where we agreed that clang > >> attribute 'nomerge' (see [2]) could be used to prevent the > >> optimization hitting in [1]. However, currently 'nomerge' applies only > >> to functions and statements, hence I submitted change requests [3], > >> [4] to allow specifying 'nomerge' for function pointers as well. > >> > >> The patch below updates bpf_helper_defs.h generation by adding a > >> definition of __nomerge macro, and using this macro in definitions of > >> relevant helpers. > >> > >> The generated code looks as follows: > >> > >> /* This is auto-generated file. See bpf_doc.py for details. */ > >> > >> #if __has_attribute(nomerge) > >> #define __nomerge __attribute__((nomerge)) > >> #else > >> #define __nomerge > >> #endif > >> > >> /* Forward declarations of BPF structs */ > >> ... > >> static long (*bpf_for_each_map_elem)(void *map, ...) __nomerge = (void *) 164; > >> ... > >> > >> (In non-RFC version the macro definition would have to be updated to > >> check for supported clang version). > >> > >> Does community agree with such approach? > > > > Makes sense to me. Let's just be very careful to do proper detection > > of __nomerge "applicability" to ensure we don't cause compilation > > errors for unsupported Clang (which I'm sure you are well aware of) > > *and* make it compatible with GCC, so we don't fix it later. > > GCC doesn't support the "nomerge" attribute at this point. We will look > into adding it or find some other equivalent mechanism that can be > abstracted in the __nomerge macro. > > >> > >> [1] https://lore.kernel.org/bpf/03bdf90f-f374-1e67-69d6-76dd9c8318a4@meta.com/ > >> [2] https://clang.llvm.org/docs/AttributeReference.html#nomerge > >> [3] https://reviews.llvm.org/D152986 > >> [4] https://reviews.llvm.org/D152987 > >> --- > >> scripts/bpf_doc.py | 37 ++++++++++++++++++++++++++++++------- > >> 1 file changed, 30 insertions(+), 7 deletions(-) > >> > >> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py > >> index eaae2ce78381..dbd4893c793e 100755 > >> --- a/scripts/bpf_doc.py > >> +++ b/scripts/bpf_doc.py > >> @@ -777,14 +777,33 @@ class PrinterHelpers(Printer): > >> 'bpf_get_socket_cookie', > >> 'bpf_sk_assign', > >> ] > >> + # Helpers that need __nomerge attribute > >> + nomerge_helpers = set([ > >> + "bpf_tail_call", > >> + "bpf_map_lookup_elem", > >> + "bpf_map_update_elem", > >> + "bpf_map_delete_elem", > >> + "bpf_map_push_elem", > >> + "bpf_map_pop_elem", > >> + "bpf_map_peek_elem", > >> + "bpf_for_each_map_elem", > >> + "bpf_redirect_map", > >> + "bpf_map_lookup_percpu_elem" > >> + ]) > >> + > >> + macros = '''\ > >> +#if __has_attribute(nomerge) > >> +#define __nomerge __attribute__((nomerge)) > >> +#else > >> +#define __nomerge > >> +#endif''' Let's add an extensive comment here, so that people looking at bpf_helper_defs.h don't need to search clang documentation on what this attr is doing. I bet even compiler experts won't be able to explain 'why' after reading the doc: https://clang.llvm.org/docs/AttributeReference.html#nomerge The example from the commit log should probably be in the comment too. Other than that the approach makes sense to me.
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index eaae2ce78381..dbd4893c793e 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -777,14 +777,33 @@ class PrinterHelpers(Printer): 'bpf_get_socket_cookie', 'bpf_sk_assign', ] + # Helpers that need __nomerge attribute + nomerge_helpers = set([ + "bpf_tail_call", + "bpf_map_lookup_elem", + "bpf_map_update_elem", + "bpf_map_delete_elem", + "bpf_map_push_elem", + "bpf_map_pop_elem", + "bpf_map_peek_elem", + "bpf_for_each_map_elem", + "bpf_redirect_map", + "bpf_map_lookup_percpu_elem" + ]) + + macros = '''\ +#if __has_attribute(nomerge) +#define __nomerge __attribute__((nomerge)) +#else +#define __nomerge +#endif''' def print_header(self): - header = '''\ -/* This is auto-generated file. See bpf_doc.py for details. */ - -/* Forward declarations of BPF structs */''' - - print(header) + print('/* This is auto-generated file. See bpf_doc.py for details. */') + print() + print(self.macros) + print() + print('/* Forward declarations of BPF structs */') for fwd in self.type_fwds: print('%s;' % fwd) print('') @@ -846,7 +865,11 @@ class PrinterHelpers(Printer): comma = ', ' print(one_arg, end='') - print(') = (void *) %d;' % helper.enum_val) + print(')', end='') + if proto['name'] in self.nomerge_helpers: + print(' __nomerge', end='') + + print(' = (void *) %d;' % helper.enum_val) print('') ###############################################################################
Update code generation for bpf_helper_defs.h by adding __attribute__((nomerge)) for a set of helper functions to prevent some verifier unfriendly compiler optimizations. This addresses a recent mailing list thread [1]. There Zhongqiu Duan and Yonghong Song discussed a C program as below: if (data_end - data > 1024) { bpf_for_each_map_elem(&map1, cb, &cb_data, 0); } else { bpf_for_each_map_elem(&map2, cb, &cb_data, 0); } Which was converted by clang to something like this: if (data_end - data > 1024) tmp = &map1; else tmp = &map2; bpf_for_each_map_elem(tmp, cb, &cb_data, 0); Which in turn triggered verification error, because verifier.c:record_func_map() requires a single map address for each bpf_for_each_map_elem() call. In fact, this is a requirement for the following helpers: - bpf_tail_call - bpf_map_lookup_elem - bpf_map_update_elem - bpf_map_delete_elem - bpf_map_push_elem - bpf_map_pop_elem - bpf_map_peek_elem - bpf_for_each_map_elem - bpf_redirect_map - bpf_map_lookup_percpu_elem I had an off-list discussion with Yonghong where we agreed that clang attribute 'nomerge' (see [2]) could be used to prevent the optimization hitting in [1]. However, currently 'nomerge' applies only to functions and statements, hence I submitted change requests [3], [4] to allow specifying 'nomerge' for function pointers as well. The patch below updates bpf_helper_defs.h generation by adding a definition of __nomerge macro, and using this macro in definitions of relevant helpers. The generated code looks as follows: /* This is auto-generated file. See bpf_doc.py for details. */ #if __has_attribute(nomerge) #define __nomerge __attribute__((nomerge)) #else #define __nomerge #endif /* Forward declarations of BPF structs */ ... static long (*bpf_for_each_map_elem)(void *map, ...) __nomerge = (void *) 164; ... (In non-RFC version the macro definition would have to be updated to check for supported clang version). Does community agree with such approach? [1] https://lore.kernel.org/bpf/03bdf90f-f374-1e67-69d6-76dd9c8318a4@meta.com/ [2] https://clang.llvm.org/docs/AttributeReference.html#nomerge [3] https://reviews.llvm.org/D152986 [4] https://reviews.llvm.org/D152987 --- scripts/bpf_doc.py | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)