| Message ID | 20210310220211.1454516-4-revest@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add a snprintf eBPF helper | expand |
On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > When initializing the __param array with a one liner, if all args are > const, the initial array value will be placed in the rodata section but > because libbpf does not support relocation in the rodata section, any > pointer in this array will stay NULL. > > This is a workaround, ideally the rodata relocation should be supported > by libbpf but this would require a disproportionate amount of work given > the actual usecases. (it is very unlikely that one uses a const array of > relocated addresses) > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > index f9ef37707888..f6a2deb3cd5b 100644 > --- a/tools/lib/bpf/bpf_tracing.h > +++ b/tools/lib/bpf/bpf_tracing.h > @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx) \ > } \ > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > +#define ___bpf_build_param0(narg, x) > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > + ___bpf_build_param1(narg, args) > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > + ___bpf_build_param2(narg, args) > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > + ___bpf_build_param3(narg, args) > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > + ___bpf_build_param4(narg, args) > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > + ___bpf_build_param5(narg, args) > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > + ___bpf_build_param6(narg, args) > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > + ___bpf_build_param7(narg, args) > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > + ___bpf_build_param8(narg, args) > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > + ___bpf_build_param9(narg, args) > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > + ___bpf_build_param10(narg, args) > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > + ___bpf_build_param11(narg, args) took me some time to get why the [narg - 12] :) it makes sense, but then I started wondering why not #define ___bpf_build_param12(narg, x, args...) ___bpf_build_param11(narg, args); ___param[11] = x ? seems more straightforward, no? also please keep all of them on single line. And to make lines shorter, let's call it ___bpf_fillX? I also don't like hard-coded ___param, which is both inflexible and is obscure at the point of use of this macro. So let's pass it as the first argument? > +#define ___bpf_build_param(args...) \ > + unsigned long long ___param[___bpf_narg(args)]; \ > + ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args) > + And here I'd pass array as a parameter and let caller define it, so macro is literally just filling the array elements, not defining the array itself and what's the type of elements > /* > * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values > * in a structure. > @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > _Pragma("GCC diagnostic push") \ > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > static const char ___fmt[] = fmt; \ > - unsigned long long ___param[] = { args }; \ > + ___bpf_build_param(args); \ > _Pragma("GCC diagnostic pop") \ > int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \ > ___param, sizeof(___param)); \ here you are violating separation of variables and code, ___bpf_build_param is defining a variable, then has code statements, then you are declaring ___ret after the code. So please split ___ret definition, > -- > 2.30.1.766.gb4fecdf3b7-goog >
On Mon, Mar 15, 2021 at 9:36 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > > When initializing the __param array with a one liner, if all args are > > const, the initial array value will be placed in the rodata section but > > because libbpf does not support relocation in the rodata section, any > > pointer in this array will stay NULL. > > > > This is a workaround, ideally the rodata relocation should be supported > > by libbpf but this would require a disproportionate amount of work given > > the actual usecases. (it is very unlikely that one uses a const array of > > relocated addresses) Can you please drop this paragraph? This is not a workaround, it's a completely working code that should continue working. And this is not something that libbpf doesn't support, there is no kernel interface to make it work at all. Please add Fixes: tag as well. > > > > Signed-off-by: Florent Revest <revest@chromium.org> > > --- > > tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++- > > 1 file changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > > index f9ef37707888..f6a2deb3cd5b 100644 > > --- a/tools/lib/bpf/bpf_tracing.h > > +++ b/tools/lib/bpf/bpf_tracing.h > > @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx) \ > > } \ > > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > > > +#define ___bpf_build_param0(narg, x) > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > > + ___bpf_build_param1(narg, args) > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > > + ___bpf_build_param2(narg, args) > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > > + ___bpf_build_param3(narg, args) > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > > + ___bpf_build_param4(narg, args) > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > > + ___bpf_build_param5(narg, args) > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > > + ___bpf_build_param6(narg, args) > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > > + ___bpf_build_param7(narg, args) > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > > + ___bpf_build_param8(narg, args) > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > > + ___bpf_build_param9(narg, args) > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > > + ___bpf_build_param10(narg, args) > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > > + ___bpf_build_param11(narg, args) > > took me some time to get why the [narg - 12] :) it makes sense, but > then I started wondering why not > > #define ___bpf_build_param12(narg, x, args...) > ___bpf_build_param11(narg, args); ___param[11] = x > > ? seems more straightforward, no? > > also please keep all of them on single line. And to make lines > shorter, let's call it ___bpf_fillX? I also don't like hard-coded > ___param, which is both inflexible and is obscure at the point of use > of this macro. So let's pass it as the first argument? > > > +#define ___bpf_build_param(args...) \ > > + unsigned long long ___param[___bpf_narg(args)]; \ > > + ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args) > > + > > And here I'd pass array as a parameter and let caller define it, so > macro is literally just filling the array elements, not defining the > array itself and what's the type of elements > > > /* > > * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values > > * in a structure. > > @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > _Pragma("GCC diagnostic push") \ > > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > > static const char ___fmt[] = fmt; \ > > - unsigned long long ___param[] = { args }; \ > > + ___bpf_build_param(args); \ > > _Pragma("GCC diagnostic pop") \ > > int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \ > > ___param, sizeof(___param)); \ > > here you are violating separation of variables and code, > ___bpf_build_param is defining a variable, then has code statements, > then you are declaring ___ret after the code. So please split ___ret > definition, > > > -- > > 2.30.1.766.gb4fecdf3b7-goog > >
On Tue, Mar 16, 2021 at 5:36 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > +#define ___bpf_build_param0(narg, x) > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > > + ___bpf_build_param1(narg, args) > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > > + ___bpf_build_param2(narg, args) > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > > + ___bpf_build_param3(narg, args) > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > > + ___bpf_build_param4(narg, args) > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > > + ___bpf_build_param5(narg, args) > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > > + ___bpf_build_param6(narg, args) > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > > + ___bpf_build_param7(narg, args) > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > > + ___bpf_build_param8(narg, args) > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > > + ___bpf_build_param9(narg, args) > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > > + ___bpf_build_param10(narg, args) > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > > + ___bpf_build_param11(narg, args) > > took me some time to get why the [narg - 12] :) it makes sense, but > then I started wondering why not > > #define ___bpf_build_param12(narg, x, args...) > ___bpf_build_param11(narg, args); ___param[11] = x > > ? seems more straightforward, no? Unless I'm misunderstanding something, I don't think this would work. The awkward "narg - 12" comes from the fact that these variadic macros work by taking the first argument out of the variadic arguments (x followed by args) and calling another macro with what's left (args). So if you do __bpf_build_param(arg1, arg2) you will have __bpf_build_param2() called with arg1 and __bpf_build_param1() called with arg2. And if you do __bpf_build_param(arg1, arg2, arg3) you will have __bpf_build_param3() called with arg1, __bpf_build_param2() called with arg2, and __bpf_build_param1() called with arg3. Basically, things are inverted, the position at which you need to insert in ___param evolves in the opposite direction of the X after ___bpf_build_param which is the number of arguments left. No matter in which order __bpf_build_paramX calls __bpf_build_param(X-1) (before or after setting ___param[n]) you will be unable to know just from the macro name at which cell in __param you need to write the argument. (except for __bpf_build_param12 which is an exception, because the max number of arg is 12, if this macro gets called, then we know that narg=12 and we will always write at __param[0]) That being said, I share your concern that this code is hard to read. So instead of giving narg to each macro, I tried to give a pos argument which indicates in which cell the macro should write. pos is basically a counter that goes from 0 to narg as macros go from narg to 0. #define ___bpf_fill0(array, pos, x) #define ___bpf_fill1(array, pos, x) array[pos] = x #define ___bpf_fill2(array, pos, x, args...) array[pos] = x; ___bpf_fill1(array, pos + 1, args) #define ___bpf_fill3(array, pos, x, args...) array[pos] = x; ___bpf_fill2(array, pos + 1, args) #define ___bpf_fill4(array, pos, x, args...) array[pos] = x; ___bpf_fill3(array, pos + 1, args) #define ___bpf_fill5(array, pos, x, args...) array[pos] = x; ___bpf_fill4(array, pos + 1, args) #define ___bpf_fill6(array, pos, x, args...) array[pos] = x; ___bpf_fill5(array, pos + 1, args) #define ___bpf_fill7(array, pos, x, args...) array[pos] = x; ___bpf_fill6(array, pos + 1, args) #define ___bpf_fill8(array, pos, x, args...) array[pos] = x; ___bpf_fill7(array, pos + 1, args) #define ___bpf_fill9(array, pos, x, args...) array[pos] = x; ___bpf_fill8(array, pos + 1, args) #define ___bpf_fill10(array, pos, x, args...) array[pos] = x; ___bpf_fill9(array, pos + 1, args) #define ___bpf_fill11(array, pos, x, args...) array[pos] = x; ___bpf_fill10(array, pos + 1, args) #define ___bpf_fill12(array, pos, x, args...) array[pos] = x; ___bpf_fill11(array, pos + 1, args) #define ___bpf_fill(array, args...) \ ___bpf_apply(___bpf_fill, ___bpf_narg(args))(array, 0, args) I hope this makes things a bit clearer ? (I often joke that BPF is written in preprocessor... :p)
On Tue, Mar 16, 2021 at 3:43 PM Florent Revest <revest@chromium.org> wrote: > > On Tue, Mar 16, 2021 at 5:36 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > +#define ___bpf_build_param0(narg, x) > > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > > > + ___bpf_build_param1(narg, args) > > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > > > + ___bpf_build_param2(narg, args) > > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > > > + ___bpf_build_param3(narg, args) > > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > > > + ___bpf_build_param4(narg, args) > > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > > > + ___bpf_build_param5(narg, args) > > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > > > + ___bpf_build_param6(narg, args) > > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > > > + ___bpf_build_param7(narg, args) > > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > > > + ___bpf_build_param8(narg, args) > > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > > > + ___bpf_build_param9(narg, args) > > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > > > + ___bpf_build_param10(narg, args) > > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > > > + ___bpf_build_param11(narg, args) > > > > took me some time to get why the [narg - 12] :) it makes sense, but > > then I started wondering why not > > > > #define ___bpf_build_param12(narg, x, args...) > > ___bpf_build_param11(narg, args); ___param[11] = x > > > > ? seems more straightforward, no? > > Unless I'm misunderstanding something, I don't think this would work. > The awkward "narg - 12" comes from the fact that these variadic macros > work by taking the first argument out of the variadic arguments (x > followed by args) and calling another macro with what's left (args). You are right, of course, silly me. > > So if you do __bpf_build_param(arg1, arg2) you will have > __bpf_build_param2() called with arg1 and __bpf_build_param1() called > with arg2. And if you do __bpf_build_param(arg1, arg2, arg3) you will > have __bpf_build_param3() called with arg1, __bpf_build_param2() > called with arg2, and __bpf_build_param1() called with arg3. > Basically, things are inverted, the position at which you need to > insert in ___param evolves in the opposite direction of the X after > ___bpf_build_param which is the number of arguments left. > > No matter in which order __bpf_build_paramX calls > __bpf_build_param(X-1) (before or after setting ___param[n]) you will > be unable to know just from the macro name at which cell in __param > you need to write the argument. (except for __bpf_build_param12 which > is an exception, because the max number of arg is 12, if this macro > gets called, then we know that narg=12 and we will always write at > __param[0]) > > That being said, I share your concern that this code is hard to read. > So instead of giving narg to each macro, I tried to give a pos > argument which indicates in which cell the macro should write. pos is > basically a counter that goes from 0 to narg as macros go from narg to > 0. > > #define ___bpf_fill0(array, pos, x) > #define ___bpf_fill1(array, pos, x) array[pos] = x > #define ___bpf_fill2(array, pos, x, args...) array[pos] = x; > ___bpf_fill1(array, pos + 1, args) > #define ___bpf_fill3(array, pos, x, args...) array[pos] = x; > ___bpf_fill2(array, pos + 1, args) > #define ___bpf_fill4(array, pos, x, args...) array[pos] = x; > ___bpf_fill3(array, pos + 1, args) > #define ___bpf_fill5(array, pos, x, args...) array[pos] = x; > ___bpf_fill4(array, pos + 1, args) > #define ___bpf_fill6(array, pos, x, args...) array[pos] = x; > ___bpf_fill5(array, pos + 1, args) > #define ___bpf_fill7(array, pos, x, args...) array[pos] = x; > ___bpf_fill6(array, pos + 1, args) > #define ___bpf_fill8(array, pos, x, args...) array[pos] = x; > ___bpf_fill7(array, pos + 1, args) > #define ___bpf_fill9(array, pos, x, args...) array[pos] = x; > ___bpf_fill8(array, pos + 1, args) > #define ___bpf_fill10(array, pos, x, args...) array[pos] = x; > ___bpf_fill9(array, pos + 1, args) > #define ___bpf_fill11(array, pos, x, args...) array[pos] = x; > ___bpf_fill10(array, pos + 1, args) > #define ___bpf_fill12(array, pos, x, args...) array[pos] = x; > ___bpf_fill11(array, pos + 1, args) > #define ___bpf_fill(array, args...) \ > ___bpf_apply(___bpf_fill, ___bpf_narg(args))(array, 0, args) Yeah, this is still more straightforward, I think. Please use shorter names to keep it a bit more succinct: arr and p seems clear enough. > > I hope this makes things a bit clearer ? (I often joke that BPF is > written in preprocessor... :p) Definitely true for BPF_CORE_READ macros :)
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h index f9ef37707888..f6a2deb3cd5b 100644 --- a/tools/lib/bpf/bpf_tracing.h +++ b/tools/lib/bpf/bpf_tracing.h @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx) \ } \ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) +#define ___bpf_build_param0(narg, x) +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ + ___bpf_build_param1(narg, args) +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ + ___bpf_build_param2(narg, args) +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ + ___bpf_build_param3(narg, args) +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ + ___bpf_build_param4(narg, args) +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ + ___bpf_build_param5(narg, args) +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ + ___bpf_build_param6(narg, args) +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ + ___bpf_build_param7(narg, args) +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ + ___bpf_build_param8(narg, args) +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ + ___bpf_build_param9(narg, args) +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ + ___bpf_build_param10(narg, args) +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ + ___bpf_build_param11(narg, args) +#define ___bpf_build_param(args...) \ + unsigned long long ___param[___bpf_narg(args)]; \ + ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args) + /* * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values * in a structure. @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) _Pragma("GCC diagnostic push") \ _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ static const char ___fmt[] = fmt; \ - unsigned long long ___param[] = { args }; \ + ___bpf_build_param(args); \ _Pragma("GCC diagnostic pop") \ int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \ ___param, sizeof(___param)); \
When initializing the __param array with a one liner, if all args are const, the initial array value will be placed in the rodata section but because libbpf does not support relocation in the rodata section, any pointer in this array will stay NULL. This is a workaround, ideally the rodata relocation should be supported by libbpf but this would require a disproportionate amount of work given the actual usecases. (it is very unlikely that one uses a const array of relocated addresses) Signed-off-by: Florent Revest <revest@chromium.org> --- tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)