| Message ID | 97ada80f3aaaeb16bf97e31a8fc204513b4fb6a9.1702594357.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Various BPF exception improvements | expand |
On Thu, Dec 14, 2023 at 2:56 PM Daniel Xu <dxu@dxuuu.xyz> wrote: > > These macros are a temporary stop-gap until bpf exceptions support > unwinding acquired entities. Basically these macros act as if they take > a callback which only get executed if the assertion fails. > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > --- > .../testing/selftests/bpf/bpf_experimental.h | 22 +++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h > index 1386baf9ae4a..d63f415bef26 100644 > --- a/tools/testing/selftests/bpf/bpf_experimental.h > +++ b/tools/testing/selftests/bpf/bpf_experimental.h > @@ -263,6 +263,17 @@ extern void bpf_throw(u64 cookie) __ksym; > */ > #define bpf_assert(cond) if (!(cond)) bpf_throw(0); > > +/* Description > + * Assert that a conditional expression is true. If false, runs code in the > + * body before throwing. > + * Returns > + * Void. > + * Throws > + * An exception with the value zero when the assertion fails. > + */ > +#define bpf_assert_if(cond) \ > + for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(0), ___i++) Kumar, Is this approach reliable? I suspect the compiler can still optimize it. I feel it will be annoying to clean up if folks start using it now, since there won't be a drop in replacement. Every such bpf_assert_if() would need to be manually patched. If 2nd part of exception is far, how about we add an equivalent of __bpf_assert() macroses with conditional ops in asm, but with extra 'asm volatile goto' that can be used to construct release of resources. bpf_do_assert_eq(var1, 0) { bpf_spin_unlock(...); } bpf_do_assert_lt(var2, 0) { bpf_spin_unlock(...); }
On Thu, Dec 14, 2023 at 6:46 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Dec 14, 2023 at 2:56 PM Daniel Xu <dxu@dxuuu.xyz> wrote: > > > > These macros are a temporary stop-gap until bpf exceptions support > > unwinding acquired entities. Basically these macros act as if they take > > a callback which only get executed if the assertion fails. > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > > --- > > .../testing/selftests/bpf/bpf_experimental.h | 22 +++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h > > index 1386baf9ae4a..d63f415bef26 100644 > > --- a/tools/testing/selftests/bpf/bpf_experimental.h > > +++ b/tools/testing/selftests/bpf/bpf_experimental.h > > @@ -263,6 +263,17 @@ extern void bpf_throw(u64 cookie) __ksym; > > */ > > #define bpf_assert(cond) if (!(cond)) bpf_throw(0); > > > > +/* Description > > + * Assert that a conditional expression is true. If false, runs code in the > > + * body before throwing. > > + * Returns > > + * Void. > > + * Throws > > + * An exception with the value zero when the assertion fails. > > + */ > > +#define bpf_assert_if(cond) \ > > + for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(0), ___i++) > > Kumar, > > Is this approach reliable? > I suspect the compiler can still optimize it. > I feel it will be annoying to clean up if folks start using it now, > since there won't be a drop in replacement. > Every such bpf_assert_if() would need to be manually patched. > > If 2nd part of exception is far, how about we add an equivalent > of __bpf_assert() macroses with conditional ops in asm, > but with extra 'asm volatile goto' that can be used to construct > release of resources. > > bpf_do_assert_eq(var1, 0) { bpf_spin_unlock(...); } > bpf_do_assert_lt(var2, 0) { bpf_spin_unlock(...); } Just realized that we can go the other way instead. We can get rid of bpf_assert_eq/ne/... and replace with: diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 1386baf9ae4a..1c500287766d 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -254,6 +254,15 @@ extern void bpf_throw(u64 cookie) __ksym; } \ }) +#define _EQ(LHS, RHS) \ + ({ int var = 1;\ + asm volatile goto("if %[lhs] == %[rhs] goto %l[l_yes]" \ + :: [lhs] "r"(LHS), [rhs] "i"(RHS) :: l_yes);\ + var = 0;\ +l_yes:\ + var;\ + }) + /* Description * Assert that a conditional expression is true. * Returns diff --git a/tools/testing/selftests/bpf/progs/exceptions.c b/tools/testing/selftests/bpf/progs/exceptions.c index 2811ee842b01..1111e852f154 100644 --- a/tools/testing/selftests/bpf/progs/exceptions.c +++ b/tools/testing/selftests/bpf/progs/exceptions.c @@ -203,6 +203,7 @@ __noinline int assert_nz_gfunc(u64 c) volatile u64 cookie = c; bpf_assert(cookie != 0); + bpf_assert(_EQ(cookie, 2)); return 0; } we can probably remove bpf_assert_with() and all of the bpf_assert_le|ne|qt|eq|_with() Users can open code everything: if (!_EQ(foo, bar)) bpf_throw(123); bpf_assert_if() can work too, but let's call it bpf_do_assert() and use like: bpf_do_assert(EQ(time, 0)) { // cleanup }
On Thu, Dec 14, 2023 at 7:10 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > Just realized that we can go the other way instead. > > We can get rid of bpf_assert_eq/ne/... and replace with: > > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h > b/tools/testing/selftests/bpf/bpf_experimental.h > index 1386baf9ae4a..1c500287766d 100644 > --- a/tools/testing/selftests/bpf/bpf_experimental.h > +++ b/tools/testing/selftests/bpf/bpf_experimental.h > @@ -254,6 +254,15 @@ extern void bpf_throw(u64 cookie) __ksym; > } > \ > }) > > +#define _EQ(LHS, RHS) \ > + ({ int var = 1;\ > + asm volatile goto("if %[lhs] == %[rhs] goto %l[l_yes]" \ > + :: [lhs] "r"(LHS), [rhs] "i"(RHS) :: l_yes);\ > + var = 0;\ > +l_yes:\ > + var;\ > + }) Realized we can do much better. We can take advantage that bpf assembly syntax resembles C and do: bpf_assert(CMP(cookie, "!=", 0); and use it as generic "volatile compare" that compiler cannot optimize out: Replacing: if (foo < bar) ... with if (CMP(foo, "<", bar)) ... when the compare operator should be preserved. I'll try to prototype it soon. Might go further and use C++ for bpf programs :) Override operator<, opreator==, ... then if (foo < bar) will be in asm code as written in C++ bpf prog.
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 1386baf9ae4a..d63f415bef26 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -263,6 +263,17 @@ extern void bpf_throw(u64 cookie) __ksym; */ #define bpf_assert(cond) if (!(cond)) bpf_throw(0); +/* Description + * Assert that a conditional expression is true. If false, runs code in the + * body before throwing. + * Returns + * Void. + * Throws + * An exception with the value zero when the assertion fails. + */ +#define bpf_assert_if(cond) \ + for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(0), ___i++) + /* Description * Assert that a conditional expression is true. * Returns @@ -272,6 +283,17 @@ extern void bpf_throw(u64 cookie) __ksym; */ #define bpf_assert_with(cond, value) if (!(cond)) bpf_throw(value); +/* Description + * Assert that a conditional expression is true. If false, runs code in the + * body before throwing. + * Returns + * Void. + * Throws + * An exception with the given value when the assertion fails. + */ +#define bpf_assert_with_if(cond, value) \ + for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(value), ___i++) + /* Description * Assert that LHS is equal to RHS. This statement updates the known value * of LHS during verification. Note that RHS must be a constant value, and
These macros are a temporary stop-gap until bpf exceptions support unwinding acquired entities. Basically these macros act as if they take a callback which only get executed if the assertion fails. Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> --- .../testing/selftests/bpf/bpf_experimental.h | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)