| Message ID | 20210825195823.381016-4-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: implement variadic printk helper | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to bpf-next |
| netdev/tree_selection | success | Clearly marked for bpf-next |
On Wed, Aug 25, 2021 at 12:58 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > Instead of being a thin wrapper which calls into bpf_trace_printk, > libbpf's bpf_printk convenience macro now chooses between > bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding > format string) is >3, use bpf_trace_vprintk, otherwise use the older > helper. > > The motivation behind this added complexity - instead of migrating > entirely to bpf_trace_vprintk - is to maintain good developer experience > for users compiling against new libbpf but running on older kernels. > Users who are passing <=3 args to bpf_printk will see no change in their > bytecode. > > __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF > macros elsewhere in the file - it allows use of bpf_trace_vprintk > without manual conversion of varargs to u64 array. Previous > implementation of bpf_printk macro is moved to __bpf_printk for use by > the new implementation. > > This does change behavior of bpf_printk calls with >3 args in the "new > libbpf, old kernels" scenario. On my system, using a clang built from > recent upstream sources (14.0.0 https://github.com/llvm/llvm-project.git > 50b62731452cb83979bbf3c06e828d26a4698dca), attempting to use 4 args to > __bpf_printk (old impl) results in a compile-time error: > > progs/trace_printk.c:21:21: error: too many args to 0x6cdf4b8: i64 = Constant<6> > trace_printk_ret = __bpf_printk("testing,testing %d %d %d %d\n", and with a new bpf_printk it will compile to use bpf_trace_vprintk and gets rejected during load on old kernels, right? That will be the case for any clang. It's fine. Would be good to clarify the commit log. > I was able to replicate this behavior with an older clang as well. When > the format string has >3 format specifiers, there is no output to the > trace_pipe in either case. I don't understand this paragraph. What are the cases? > After this patch, using bpf_printk with 4 args would result in a > trace_vprintk helper call being emitted and a load-time failure on older > kernels. right. > +#define __bpf_printk(fmt, ...) \ > +({ \ > + char ____fmt[] = fmt; \ Andrii was suggesting to make it const while we're at it, but that could be done in a follow up.
On 8/25/21 9:01 PM, Alexei Starovoitov wrote: > On Wed, Aug 25, 2021 at 12:58 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: >> >> Instead of being a thin wrapper which calls into bpf_trace_printk, >> libbpf's bpf_printk convenience macro now chooses between >> bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding >> format string) is >3, use bpf_trace_vprintk, otherwise use the older >> helper. >> >> The motivation behind this added complexity - instead of migrating >> entirely to bpf_trace_vprintk - is to maintain good developer experience >> for users compiling against new libbpf but running on older kernels. >> Users who are passing <=3 args to bpf_printk will see no change in their >> bytecode. >> >> __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF >> macros elsewhere in the file - it allows use of bpf_trace_vprintk >> without manual conversion of varargs to u64 array. Previous >> implementation of bpf_printk macro is moved to __bpf_printk for use by >> the new implementation. >> >> This does change behavior of bpf_printk calls with >3 args in the "new >> libbpf, old kernels" scenario. On my system, using a clang built from >> recent upstream sources (14.0.0 https://github.com/llvm/llvm-project.git >> 50b62731452cb83979bbf3c06e828d26a4698dca), attempting to use 4 args to >> __bpf_printk (old impl) results in a compile-time error: >> >> progs/trace_printk.c:21:21: error: too many args to 0x6cdf4b8: i64 = Constant<6> >> trace_printk_ret = __bpf_printk("testing,testing %d %d %d %d\n", > > and with a new bpf_printk it will compile to use bpf_trace_vprintk > and gets rejected during load on old kernels, right? > That will be the case for any clang. > It's fine. > Would be good to clarify the commit log. Yep, I think we're on the same page here. Wanted to call out the changed behavior in case it felt more like 'breaking user expectations'. Will simplify the commit message for this patch in v3. >> I was able to replicate this behavior with an older clang as well. When >> the format string has >3 format specifiers, there is no output to the >> trace_pipe in either case. > > I don't understand this paragraph. What are the cases? This was me trying to enumerate behavior before/after this patch in order to answer the 'does this break user expectations' question. I was curious whether clang version affected error messages users would see when doing things old bpf_printk didn't support (>3 args, >3 format specifiers). Format specifier >3 case is intentional runtime behavior, so in retrospect there was no reason to focus on clang version there. Will remove from commit msg. >> After this patch, using bpf_printk with 4 args would result in a >> trace_vprintk helper call being emitted and a load-time failure on older >> kernels. > > right. > >> +#define __bpf_printk(fmt, ...) \ >> +({ \ >> + char ____fmt[] = fmt; \ > > Andrii was suggesting to make it const while we're at it, > but that could be done in a follow up. This was intentionally left out of v2 as I wanted to get early feedback on the macro stuff, will add a patch doing this to v3.
On Wed, Aug 25, 2021 at 12:58 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > Instead of being a thin wrapper which calls into bpf_trace_printk, > libbpf's bpf_printk convenience macro now chooses between > bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding > format string) is >3, use bpf_trace_vprintk, otherwise use the older > helper. > > The motivation behind this added complexity - instead of migrating > entirely to bpf_trace_vprintk - is to maintain good developer experience > for users compiling against new libbpf but running on older kernels. > Users who are passing <=3 args to bpf_printk will see no change in their > bytecode. > > __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF > macros elsewhere in the file - it allows use of bpf_trace_vprintk > without manual conversion of varargs to u64 array. Previous > implementation of bpf_printk macro is moved to __bpf_printk for use by > the new implementation. > > This does change behavior of bpf_printk calls with >3 args in the "new > libbpf, old kernels" scenario. On my system, using a clang built from > recent upstream sources (14.0.0 https://github.com/llvm/llvm-project.git > 50b62731452cb83979bbf3c06e828d26a4698dca), attempting to use 4 args to > __bpf_printk (old impl) results in a compile-time error: > > progs/trace_printk.c:21:21: error: too many args to 0x6cdf4b8: i64 = Constant<6> > trace_printk_ret = __bpf_printk("testing,testing %d %d %d %d\n", > > I was able to replicate this behavior with an older clang as well. When > the format string has >3 format specifiers, there is no output to the > trace_pipe in either case. > > After this patch, using bpf_printk with 4 args would result in a > trace_vprintk helper call being emitted and a load-time failure on older > kernels. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++------- > 1 file changed, 37 insertions(+), 8 deletions(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index b9987c3efa3c..5f087306cdfe 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -14,14 +14,6 @@ > #define __type(name, val) typeof(val) *name > #define __array(name, val) typeof(val) *name[] > > -/* Helper macro to print out debug messages */ > -#define bpf_printk(fmt, ...) \ > -({ \ > - char ____fmt[] = fmt; \ > - bpf_trace_printk(____fmt, sizeof(____fmt), \ > - ##__VA_ARGS__); \ > -}) > - > /* > * Helper macro to place programs, maps, license in > * different sections in elf_bpf file. Section names > @@ -224,4 +216,41 @@ enum libbpf_tristate { > ___param, sizeof(___param)); \ > }) > > +/* Helper macro to print out debug messages */ > +#define __bpf_printk(fmt, ...) \ > +({ \ > + char ____fmt[] = fmt; \ > + bpf_trace_printk(____fmt, sizeof(____fmt), \ > + ##__VA_ARGS__); \ > +}) > + > +/* > + * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments > + * instead of an array of u64. > + */ > +#define __bpf_vprintk(fmt, args...) \ > +({ \ > + static const char ___fmt[] = fmt; \ > + unsigned long long ___param[___bpf_narg(args)]; \ > + \ > + _Pragma("GCC diagnostic push") \ > + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > + ___bpf_fill(___param, args); \ > + _Pragma("GCC diagnostic pop") \ > + \ > + bpf_trace_vprintk(___fmt, sizeof(___fmt), \ > + ___param, sizeof(___param)); \ nit: is this really misaligned or it's just Gmail's rendering? > +}) > + > +#define ___bpf_pick_printk(...) \ > + ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ > + __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ > + __bpf_vprintk, __bpf_vprintk, __bpf_printk, __bpf_printk, \ > + __bpf_printk, __bpf_printk) There is no best solution with macros, but I think this one is extremely error prone because __bpf_nth invocation is very long and it's hard to even see where printk turns into vprintk. How about doing it similarly to ___empty in bpf_core_read.h? It will be something like this (untested and not even compiled, just a demo) #define __bpf_printk_kind(...) ___bpf_nth(_, ##__VA_ARGS__, new, new, new, new, new, <however many>, new, old /*3*/, old /*2*/, old /*1*/, old /*0*/) #define bpf_printk(fmt, args...) ___bpf_apply(___bpf_printk_, ___bpf_narg(args))(fmt, args) And you'll have s/__bpf_printk/__bpf_printk_old/ (using bpf_trace_printk) and s/__bpf_printk_new/__bpf_vprintk/ (using bpf_trace_vprintk). This new/old distinction makes it a bit clearer to me. I find __bpf_nth so counterintuitive that I try not to use it directly anywhere at all. > + > +#define bpf_printk(fmt, args...) \ > +({ \ > + ___bpf_pick_printk(args)(fmt, args); \ > +}) not sure ({ }) buys you anything?... > + > #endif > -- > 2.30.2 >
On 8/30/21 7:55 PM, Andrii Nakryiko wrote: > On Wed, Aug 25, 2021 at 12:58 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: >> >> Instead of being a thin wrapper which calls into bpf_trace_printk, >> libbpf's bpf_printk convenience macro now chooses between >> bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding >> format string) is >3, use bpf_trace_vprintk, otherwise use the older >> helper. >> >> The motivation behind this added complexity - instead of migrating >> entirely to bpf_trace_vprintk - is to maintain good developer experience >> for users compiling against new libbpf but running on older kernels. >> Users who are passing <=3 args to bpf_printk will see no change in their >> bytecode. >> >> __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF >> macros elsewhere in the file - it allows use of bpf_trace_vprintk >> without manual conversion of varargs to u64 array. Previous >> implementation of bpf_printk macro is moved to __bpf_printk for use by >> the new implementation. >> >> This does change behavior of bpf_printk calls with >3 args in the "new >> libbpf, old kernels" scenario. On my system, using a clang built from >> recent upstream sources (14.0.0 https://github.com/llvm/llvm-project.git >> 50b62731452cb83979bbf3c06e828d26a4698dca), attempting to use 4 args to >> __bpf_printk (old impl) results in a compile-time error: >> >> progs/trace_printk.c:21:21: error: too many args to 0x6cdf4b8: i64 = Constant<6> >> trace_printk_ret = __bpf_printk("testing,testing %d %d %d %d\n", >> >> I was able to replicate this behavior with an older clang as well. When >> the format string has >3 format specifiers, there is no output to the >> trace_pipe in either case. >> >> After this patch, using bpf_printk with 4 args would result in a >> trace_vprintk helper call being emitted and a load-time failure on older >> kernels. >> >> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> >> --- >> tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++------- >> 1 file changed, 37 insertions(+), 8 deletions(-) >> >> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >> index b9987c3efa3c..5f087306cdfe 100644 >> --- a/tools/lib/bpf/bpf_helpers.h >> +++ b/tools/lib/bpf/bpf_helpers.h >> @@ -14,14 +14,6 @@ >> #define __type(name, val) typeof(val) *name >> #define __array(name, val) typeof(val) *name[] >> >> -/* Helper macro to print out debug messages */ >> -#define bpf_printk(fmt, ...) \ >> -({ \ >> - char ____fmt[] = fmt; \ >> - bpf_trace_printk(____fmt, sizeof(____fmt), \ >> - ##__VA_ARGS__); \ >> -}) >> - >> /* >> * Helper macro to place programs, maps, license in >> * different sections in elf_bpf file. Section names >> @@ -224,4 +216,41 @@ enum libbpf_tristate { >> ___param, sizeof(___param)); \ >> }) >> >> +/* Helper macro to print out debug messages */ >> +#define __bpf_printk(fmt, ...) \ >> +({ \ >> + char ____fmt[] = fmt; \ >> + bpf_trace_printk(____fmt, sizeof(____fmt), \ >> + ##__VA_ARGS__); \ >> +}) >> + >> +/* >> + * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments >> + * instead of an array of u64. >> + */ >> +#define __bpf_vprintk(fmt, args...) \ >> +({ \ >> + static const char ___fmt[] = fmt; \ >> + unsigned long long ___param[___bpf_narg(args)]; \ >> + \ >> + _Pragma("GCC diagnostic push") \ >> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ >> + ___bpf_fill(___param, args); \ >> + _Pragma("GCC diagnostic pop") \ >> + \ >> + bpf_trace_vprintk(___fmt, sizeof(___fmt), \ >> + ___param, sizeof(___param)); \ > > nit: is this really misaligned or it's just Gmail's rendering? It's misaligned, will fix. As is __bpf_pick_printk below. >> +}) >> + >> +#define ___bpf_pick_printk(...) \ >> + ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ >> + __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ >> + __bpf_vprintk, __bpf_vprintk, __bpf_printk, __bpf_printk, \ >> + __bpf_printk, __bpf_printk) > > There is no best solution with macros, but I think this one is > extremely error prone because __bpf_nth invocation is very long and > it's hard to even see where printk turns into vprintk. > > How about doing it similarly to ___empty in bpf_core_read.h? It will > be something like this (untested and not even compiled, just a demo) > > #define __bpf_printk_kind(...) ___bpf_nth(_, ##__VA_ARGS__, new, new, > new, new, new, <however many>, new, old /*3*/, old /*2*/, old /*1*/, > old /*0*/) > > #define bpf_printk(fmt, args...) ___bpf_apply(___bpf_printk_, > ___bpf_narg(args))(fmt, args) > > > And you'll have s/__bpf_printk/__bpf_printk_old/ (using > bpf_trace_printk) and s/__bpf_printk_new/__bpf_vprintk/ (using > bpf_trace_vprintk). > > This new/old distinction makes it a bit clearer to me. I find > __bpf_nth so counterintuitive that I try not to use it directly > anywhere at all. When you're saying 'error prone' here, do you mean something like 'hard to understand and modify'? Asking because IMO adding ___bpf_apply here makes it harder to understand. Having the full helper macros in ___bpf_nth makes it obvious that they're being used somehow. But I feel more strongly that these should not be renamed to __bpf_printk_{old,new}. Although this is admittedly an edge case, I'd like to leave an 'escape hatch' for power users who might not want bpf_printk to change the helper call underneath them - they could use the __bpf_{v}printk macros directly. Of course they could do the same with _{old,new}, but the rename obscures the name of the underlying helper called, which is the very thing the hypothetical power user cares about in this scenario. One concrete example of such a user: someone who keeps up with latest bpf developments but needs to run their programs on a fleet which has some % of older kernels. Using __bpf_printk directly to force a compile error for >3 fmt args instead of being bitten at load time would be desireable. Also, 'new' name leaves open possibility that something newer comes along in the future and turns 'new' into 'old', which feels churny. Although if these are never used directly it doesn't matter. I agree with 'it's hard to even see where printk turns into vprintk' and like your comment idea. If you're fine with keeping names as-is, will still add /*3*/ /*2*/... and perhaps a /*BOUNDARY*/ marking the switch from vprintk to printk. > >> + >> +#define bpf_printk(fmt, args...) \ >> +({ \ >> + ___bpf_pick_printk(args)(fmt, args); \ >> +}) > > not sure ({ }) buys you anything?... Agreed, will fix. >> + >> #endif >> -- >> 2.30.2 >>
On Wed, Sep 1, 2021 at 4:29 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > On 8/30/21 7:55 PM, Andrii Nakryiko wrote: > > On Wed, Aug 25, 2021 at 12:58 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > >> > >> Instead of being a thin wrapper which calls into bpf_trace_printk, > >> libbpf's bpf_printk convenience macro now chooses between > >> bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding > >> format string) is >3, use bpf_trace_vprintk, otherwise use the older > >> helper. > >> > >> The motivation behind this added complexity - instead of migrating > >> entirely to bpf_trace_vprintk - is to maintain good developer experience > >> for users compiling against new libbpf but running on older kernels. > >> Users who are passing <=3 args to bpf_printk will see no change in their > >> bytecode. > >> > >> __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF > >> macros elsewhere in the file - it allows use of bpf_trace_vprintk > >> without manual conversion of varargs to u64 array. Previous > >> implementation of bpf_printk macro is moved to __bpf_printk for use by > >> the new implementation. > >> > >> This does change behavior of bpf_printk calls with >3 args in the "new > >> libbpf, old kernels" scenario. On my system, using a clang built from > >> recent upstream sources (14.0.0 https://github.com/llvm/llvm-project.git > >> 50b62731452cb83979bbf3c06e828d26a4698dca), attempting to use 4 args to > >> __bpf_printk (old impl) results in a compile-time error: > >> > >> progs/trace_printk.c:21:21: error: too many args to 0x6cdf4b8: i64 = Constant<6> > >> trace_printk_ret = __bpf_printk("testing,testing %d %d %d %d\n", > >> > >> I was able to replicate this behavior with an older clang as well. When > >> the format string has >3 format specifiers, there is no output to the > >> trace_pipe in either case. > >> > >> After this patch, using bpf_printk with 4 args would result in a > >> trace_vprintk helper call being emitted and a load-time failure on older > >> kernels. > >> > >> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > >> --- > >> tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++------- > >> 1 file changed, 37 insertions(+), 8 deletions(-) > >> > >> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > >> index b9987c3efa3c..5f087306cdfe 100644 > >> --- a/tools/lib/bpf/bpf_helpers.h > >> +++ b/tools/lib/bpf/bpf_helpers.h > >> @@ -14,14 +14,6 @@ > >> #define __type(name, val) typeof(val) *name > >> #define __array(name, val) typeof(val) *name[] > >> > >> -/* Helper macro to print out debug messages */ > >> -#define bpf_printk(fmt, ...) \ > >> -({ \ > >> - char ____fmt[] = fmt; \ > >> - bpf_trace_printk(____fmt, sizeof(____fmt), \ > >> - ##__VA_ARGS__); \ > >> -}) > >> - > >> /* > >> * Helper macro to place programs, maps, license in > >> * different sections in elf_bpf file. Section names > >> @@ -224,4 +216,41 @@ enum libbpf_tristate { > >> ___param, sizeof(___param)); \ > >> }) > >> > >> +/* Helper macro to print out debug messages */ > >> +#define __bpf_printk(fmt, ...) \ > >> +({ \ > >> + char ____fmt[] = fmt; \ > >> + bpf_trace_printk(____fmt, sizeof(____fmt), \ > >> + ##__VA_ARGS__); \ > >> +}) > >> + > >> +/* > >> + * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments > >> + * instead of an array of u64. > >> + */ > >> +#define __bpf_vprintk(fmt, args...) \ > >> +({ \ > >> + static const char ___fmt[] = fmt; \ > >> + unsigned long long ___param[___bpf_narg(args)]; \ > >> + \ > >> + _Pragma("GCC diagnostic push") \ > >> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > >> + ___bpf_fill(___param, args); \ > >> + _Pragma("GCC diagnostic pop") \ > >> + \ > >> + bpf_trace_vprintk(___fmt, sizeof(___fmt), \ > >> + ___param, sizeof(___param)); \ > > > > nit: is this really misaligned or it's just Gmail's rendering? > > It's misaligned, will fix. As is __bpf_pick_printk below. > > >> +}) > >> + > >> +#define ___bpf_pick_printk(...) \ > >> + ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ > >> + __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ > >> + __bpf_vprintk, __bpf_vprintk, __bpf_printk, __bpf_printk, \ > >> + __bpf_printk, __bpf_printk) > > > > There is no best solution with macros, but I think this one is > > extremely error prone because __bpf_nth invocation is very long and > > it's hard to even see where printk turns into vprintk. > > > > How about doing it similarly to ___empty in bpf_core_read.h? It will > > be something like this (untested and not even compiled, just a demo) > > > > #define __bpf_printk_kind(...) ___bpf_nth(_, ##__VA_ARGS__, new, new, > > new, new, new, <however many>, new, old /*3*/, old /*2*/, old /*1*/, > > old /*0*/) > > > > #define bpf_printk(fmt, args...) ___bpf_apply(___bpf_printk_, > > ___bpf_narg(args))(fmt, args) > > > > > > And you'll have s/__bpf_printk/__bpf_printk_old/ (using > > bpf_trace_printk) and s/__bpf_printk_new/__bpf_vprintk/ (using > > bpf_trace_vprintk). > > > > This new/old distinction makes it a bit clearer to me. I find > > __bpf_nth so counterintuitive that I try not to use it directly > > anywhere at all. > > When you're saying 'error prone' here, do you mean something like > 'hard to understand and modify'? Asking because IMO adding > ___bpf_apply here makes it harder to understand. Having the full > helper macros in ___bpf_nth makes it obvious that they're being used > somehow. Hm... I disagree on ___bpf_nth being easier, because of both *reverse* and *positional* notation. But whichever you prefer, not a big deal. In this particular case it takes lots of attention to even see at which position __bpf_vprintk switches to __bpf_printk. They are too similar and not both verbose and not distinctive enough, IMO. bpf_apply feels more natural, but I'm the one who wrote a bunch of bpf_core_read.h macro using that approach, so I'm totally biased. (Though I wrote and used bpf__nth as well, yet I still hate it, but it's just a necessary evil). > > But I feel more strongly that these should not be renamed to __bpf_printk_{old,new}. > Although this is admittedly an edge case, I'd like to leave an 'escape > hatch' for power users who might not want bpf_printk to change the > helper call underneath them - they could use the __bpf_{v}printk > macros directly. Of course they could do the same with _{old,new}, > but the rename obscures the name of the underlying helper called, > which is the very thing the hypothetical power user cares about in > this scenario. Any of the __ prefixed macro should not be used by anyone and are not considered part of the API. We can rename, remove, break them at any time. So regardless of the above, one should not use __bpf_vprintk or __bpf_vprintk directly in their BPF apps. > > One concrete example of such a user: someone who keeps up with > latest bpf developments but needs to run their programs on a fleet > which has some % of older kernels. Using __bpf_printk directly to > force a compile error for >3 fmt args instead of being bitten at > load time would be desireable. it's not hard for such users to just copy/paste (and actually have cleaner name). __bpf_printk() is not a hard macro that needs to be reused by end users. > > Also, 'new' name leaves open possibility that something newer comes > along in the future and turns 'new' into 'old', which feels churny. > Although if these are never used directly it doesn't matter. Right, internal implementation details, as far as end users are concerned. > > I agree with 'it's hard to even see where printk turns into vprintk' > and like your comment idea. If you're fine with keeping names as-is, > will still add /*3*/ /*2*/... and perhaps a /*BOUNDARY*/ marking the > switch from vprintk to printk. BOUNDARY is probably an overkill. Positional comments might be nice, try it. > > > > >> + > >> +#define bpf_printk(fmt, args...) \ > >> +({ \ > >> + ___bpf_pick_printk(args)(fmt, args); \ > >> +}) > > > > not sure ({ }) buys you anything?... > > Agreed, will fix. > > >> + > >> #endif > >> -- > >> 2.30.2 > >> >
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index b9987c3efa3c..5f087306cdfe 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -14,14 +14,6 @@ #define __type(name, val) typeof(val) *name #define __array(name, val) typeof(val) *name[] -/* Helper macro to print out debug messages */ -#define bpf_printk(fmt, ...) \ -({ \ - char ____fmt[] = fmt; \ - bpf_trace_printk(____fmt, sizeof(____fmt), \ - ##__VA_ARGS__); \ -}) - /* * Helper macro to place programs, maps, license in * different sections in elf_bpf file. Section names @@ -224,4 +216,41 @@ enum libbpf_tristate { ___param, sizeof(___param)); \ }) +/* Helper macro to print out debug messages */ +#define __bpf_printk(fmt, ...) \ +({ \ + char ____fmt[] = fmt; \ + bpf_trace_printk(____fmt, sizeof(____fmt), \ + ##__VA_ARGS__); \ +}) + +/* + * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments + * instead of an array of u64. + */ +#define __bpf_vprintk(fmt, args...) \ +({ \ + static const char ___fmt[] = fmt; \ + unsigned long long ___param[___bpf_narg(args)]; \ + \ + _Pragma("GCC diagnostic push") \ + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ + ___bpf_fill(___param, args); \ + _Pragma("GCC diagnostic pop") \ + \ + bpf_trace_vprintk(___fmt, sizeof(___fmt), \ + ___param, sizeof(___param)); \ +}) + +#define ___bpf_pick_printk(...) \ + ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ + __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \ + __bpf_vprintk, __bpf_vprintk, __bpf_printk, __bpf_printk, \ + __bpf_printk, __bpf_printk) + +#define bpf_printk(fmt, args...) \ +({ \ + ___bpf_pick_printk(args)(fmt, args); \ +}) + #endif
Instead of being a thin wrapper which calls into bpf_trace_printk, libbpf's bpf_printk convenience macro now chooses between bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding format string) is >3, use bpf_trace_vprintk, otherwise use the older helper. The motivation behind this added complexity - instead of migrating entirely to bpf_trace_vprintk - is to maintain good developer experience for users compiling against new libbpf but running on older kernels. Users who are passing <=3 args to bpf_printk will see no change in their bytecode. __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF macros elsewhere in the file - it allows use of bpf_trace_vprintk without manual conversion of varargs to u64 array. Previous implementation of bpf_printk macro is moved to __bpf_printk for use by the new implementation. This does change behavior of bpf_printk calls with >3 args in the "new libbpf, old kernels" scenario. On my system, using a clang built from recent upstream sources (14.0.0 https://github.com/llvm/llvm-project.git 50b62731452cb83979bbf3c06e828d26a4698dca), attempting to use 4 args to __bpf_printk (old impl) results in a compile-time error: progs/trace_printk.c:21:21: error: too many args to 0x6cdf4b8: i64 = Constant<6> trace_printk_ret = __bpf_printk("testing,testing %d %d %d %d\n", I was able to replicate this behavior with an older clang as well. When the format string has >3 format specifiers, there is no output to the trace_pipe in either case. After this patch, using bpf_printk with 4 args would result in a trace_vprintk helper call being emitted and a load-time failure on older kernels. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 8 deletions(-)