From: Eduard Zingerman <eddyz87@gmail.com>
To: bpf@vger.kernel.org, ast@kernel.org
Cc: andrii@kernel.org, daniel@iogearbox.net, martin.lau@linux.dev,
kernel-team@fb.com, yhs@fb.com, jemarch@gnu.org,
david.faust@oracle.com, dzq.aishenghu0@gmail.com,
Eduard Zingerman <eddyz87@gmail.com>
Subject: [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h
Date: Thu, 15 Jun 2023 17:25:20 +0300 [thread overview]
Message-ID: <20230615142520.10280-1-eddyz87@gmail.com> (raw)
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(-)
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
next reply other threads:[~2023-06-15 14:25 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-15 14:25 Eduard Zingerman [this message]
2023-06-16 17:03 ` [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h Andrii Nakryiko
2023-06-20 18:27 ` Jose E. Marchesi
2023-06-22 21:35 ` Alexei Starovoitov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230615142520.10280-1-eddyz87@gmail.com \
--to=eddyz87@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=david.faust@oracle.com \
--cc=dzq.aishenghu0@gmail.com \
--cc=jemarch@gnu.org \
--cc=kernel-team@fb.com \
--cc=martin.lau@linux.dev \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).