Use pointers to structure as arguments to function instead of coping
structures and less stack size. Also transfer TNUM(_v, _m) to
tnum.h file to be used in differnet files for creating anonymous structures
statically.
Signed-off-by: Karim Eshapa <[email protected]>
Thanks,
Karim
---
include/linux/tnum.h | 4 +++-
kernel/bpf/tnum.c | 14 +++++++-------
kernel/bpf/verifier.c | 11 ++++++-----
3 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/include/linux/tnum.h b/include/linux/tnum.h
index 0d2d3da..72938a0 100644
--- a/include/linux/tnum.h
+++ b/include/linux/tnum.h
@@ -13,6 +13,8 @@ struct tnum {
};
/* Constructors */
+/* Statically tnum constant */
+#define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m}
/* Represent a known constant as a tnum. */
struct tnum tnum_const(u64 value);
/* A completely unknown value */
@@ -26,7 +28,7 @@ struct tnum tnum_lshift(struct tnum a, u8 shift);
/* Shift a tnum right (by a fixed shift) */
struct tnum tnum_rshift(struct tnum a, u8 shift);
/* Add two tnums, return @a + @b */
-struct tnum tnum_add(struct tnum a, struct tnum b);
+void tnum_add(struct tnum *res, struct tnum *a, struct tnum *b);
/* Subtract two tnums, return @a - @b */
struct tnum tnum_sub(struct tnum a, struct tnum b);
/* Bitwise-AND, return @a & @b */
diff --git a/kernel/bpf/tnum.c b/kernel/bpf/tnum.c
index 1f4bf68..89e3182 100644
--- a/kernel/bpf/tnum.c
+++ b/kernel/bpf/tnum.c
@@ -8,7 +8,6 @@
#include <linux/kernel.h>
#include <linux/tnum.h>
-#define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m}
/* A completely unknown value */
const struct tnum tnum_unknown = { .value = 0, .mask = -1 };
@@ -43,16 +42,17 @@ struct tnum tnum_rshift(struct tnum a, u8 shift)
return TNUM(a.value >> shift, a.mask >> shift);
}
-struct tnum tnum_add(struct tnum a, struct tnum b)
+void tnum_add(struct tnum *res, struct tnum *a, struct tnum *b)
{
u64 sm, sv, sigma, chi, mu;
- sm = a.mask + b.mask;
- sv = a.value + b.value;
+ sm = a->mask + b->mask;
+ sv = a->value + b->value;
sigma = sm + sv;
chi = sigma ^ sv;
- mu = chi | a.mask | b.mask;
- return TNUM(sv & ~mu, mu);
+ mu = chi | a->mask | b->mask;
+ res->value = (sv & ~mu);
+ res->mask = mu;
}
struct tnum tnum_sub(struct tnum a, struct tnum b)
@@ -102,7 +102,7 @@ static struct tnum hma(struct tnum acc, u64 value, u64 mask)
{
while (mask) {
if (mask & 1)
- acc = tnum_add(acc, TNUM(0, value));
+ tnum_add(&acc, &acc, &TNUM(0, value));
mask >>= 1;
value <<= 1;
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b414d6b..b31b1c4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -999,7 +999,7 @@ static int check_pkt_ptr_alignment(struct bpf_verifier_env *env,
*/
ip_align = 2;
- reg_off = tnum_add(reg->var_off, tnum_const(ip_align + reg->off + off));
+ tnum_add(®_off, ®->var_off, &TNUM(ip_align + reg->off + off, 0));
if (!tnum_is_aligned(reg_off, size)) {
char tn_buf[48];
@@ -1023,8 +1023,7 @@ static int check_generic_ptr_alignment(struct bpf_verifier_env *env,
/* Byte size accesses are always allowed. */
if (!strict || size == 1)
return 0;
-
- reg_off = tnum_add(reg->var_off, tnum_const(reg->off + off));
+ tnum_add(®_off, ®->var_off, &TNUM(reg->off + off, 0));
if (!tnum_is_aligned(reg_off, size)) {
char tn_buf[48];
@@ -1971,7 +1970,8 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
dst_reg->umin_value = umin_ptr + umin_val;
dst_reg->umax_value = umax_ptr + umax_val;
}
- dst_reg->var_off = tnum_add(ptr_reg->var_off, off_reg->var_off);
+ tnum_add(&dst_reg->var_off, &ptr_reg->var_off,
+ &off_reg->var_off);
dst_reg->off = ptr_reg->off;
if (reg_is_pkt_pointer(ptr_reg)) {
dst_reg->id = ++env->id_gen;
@@ -2108,7 +2108,8 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
dst_reg->umin_value += umin_val;
dst_reg->umax_value += umax_val;
}
- dst_reg->var_off = tnum_add(dst_reg->var_off, src_reg.var_off);
+ tnum_add(&dst_reg->var_off, &dst_reg->var_off,
+ &src_reg.var_off);
break;
case BPF_SUB:
if (signed_sub_overflows(dst_reg->smin_value, smax_val) ||
--
2.7.4
On Sun, Jan 14, 2018 at 12:03:42AM +0200, Karim Eshapa wrote:
> Use pointers to structure as arguments to function instead of coping
> structures and less stack size. Also transfer TNUM(_v, _m) to
> tnum.h file to be used in differnet files for creating anonymous structures
> statically.
>
> Signed-off-by: Karim Eshapa <[email protected]>
...
> +/* Statically tnum constant */
> +#define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m}
> /* Represent a known constant as a tnum. */
> struct tnum tnum_const(u64 value);
> /* A completely unknown value */
> @@ -26,7 +28,7 @@ struct tnum tnum_lshift(struct tnum a, u8 shift);
> /* Shift a tnum right (by a fixed shift) */
> struct tnum tnum_rshift(struct tnum a, u8 shift);
> /* Add two tnums, return @a + @b */
> -struct tnum tnum_add(struct tnum a, struct tnum b);
> +void tnum_add(struct tnum *res, struct tnum *a, struct tnum *b);
...
> - reg_off = tnum_add(reg->var_off, tnum_const(ip_align + reg->off + off));
> + tnum_add(®_off, ®->var_off, &TNUM(ip_align + reg->off + off, 0));
> if (!tnum_is_aligned(reg_off, size)) {
> char tn_buf[48];
>
> @@ -1023,8 +1023,7 @@ static int check_generic_ptr_alignment(struct bpf_verifier_env *env,
> /* Byte size accesses are always allowed. */
> if (!strict || size == 1)
> return 0;
> -
> - reg_off = tnum_add(reg->var_off, tnum_const(reg->off + off));
> + tnum_add(®_off, ®->var_off, &TNUM(reg->off + off, 0));
...
> - dst_reg->var_off = tnum_add(ptr_reg->var_off, off_reg->var_off);
> + tnum_add(&dst_reg->var_off, &ptr_reg->var_off,
> + &off_reg->var_off);
I think that looks much worse and error prone.
Is it gnu or intel style of argumnets ? where is src or dest ?
Can the same pointer be used as src and as dst ? etc, etc
I don't think it saves stack either.
I'd rather leave things as-is.
On 13/01/18 22:03, Karim Eshapa wrote:
> Use pointers to structure as arguments to function instead of coping
> structures and less stack size. Also transfer TNUM(_v, _m) to
> tnum.h file to be used in differnet files for creating anonymous structures
> statically.
>
> Signed-off-by: Karim Eshapa <[email protected]>
NACK (some reasons inline).
> Thanks,
> Karim
> ---
> include/linux/tnum.h | 4 +++-
> kernel/bpf/tnum.c | 14 +++++++-------
> kernel/bpf/verifier.c | 11 ++++++-----
> 3 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/tnum.h b/include/linux/tnum.h
> index 0d2d3da..72938a0 100644
> --- a/include/linux/tnum.h
> +++ b/include/linux/tnum.h
> @@ -13,6 +13,8 @@ struct tnum {
> };
>
> /* Constructors */
> +/* Statically tnum constant */
> +#define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m}
This shouldn't be in the 'public' API, because it's dealing in the internals
of the tnum struct in ways that calling code shouldn't have to worry about.
Instead, callers should use functions like tnum_const() to construct tnums.
> /* Represent a known constant as a tnum. */
> struct tnum tnum_const(u64 value);
> /* A completely unknown value */
> @@ -26,7 +28,7 @@ struct tnum tnum_lshift(struct tnum a, u8 shift);
> /* Shift a tnum right (by a fixed shift) */
> struct tnum tnum_rshift(struct tnum a, u8 shift);
> /* Add two tnums, return @a + @b */
> -struct tnum tnum_add(struct tnum a, struct tnum b);
> +void tnum_add(struct tnum *res, struct tnum *a, struct tnum *b);
I would expect the old tnum_add to be inlined by the compiler. Moreover,
the arguments and return value are clearly separate, whereas in your new
version they could (and often will) alias, thus the function body has to
be careful not to write the result until it has finished reading the args.
I wouldn't be surprised if your versions actually _increased_ total stack
usage by confusing the compiler's inliner and liveness analysis.
> /* Subtract two tnums, return @a - @b */
> struct tnum tnum_sub(struct tnum a, struct tnum b);
> /* Bitwise-AND, return @a & @b */
> diff --git a/kernel/bpf/tnum.c b/kernel/bpf/tnum.c
> index 1f4bf68..89e3182 100644
> --- a/kernel/bpf/tnum.c
> +++ b/kernel/bpf/tnum.c
> @@ -8,7 +8,6 @@
> #include <linux/kernel.h>
> #include <linux/tnum.h>
>
> -#define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m}
> /* A completely unknown value */
> const struct tnum tnum_unknown = { .value = 0, .mask = -1 };
>
> @@ -43,16 +42,17 @@ struct tnum tnum_rshift(struct tnum a, u8 shift)
> return TNUM(a.value >> shift, a.mask >> shift);
> }
>
> -struct tnum tnum_add(struct tnum a, struct tnum b)
> +void tnum_add(struct tnum *res, struct tnum *a, struct tnum *b)
> {
> u64 sm, sv, sigma, chi, mu;
>
> - sm = a.mask + b.mask;
> - sv = a.value + b.value;
> + sm = a->mask + b->mask;
> + sv = a->value + b->value;
> sigma = sm + sv;
> chi = sigma ^ sv;
> - mu = chi | a.mask | b.mask;
> - return TNUM(sv & ~mu, mu);
> + mu = chi | a->mask | b->mask;
> + res->value = (sv & ~mu);
> + res->mask = mu;
> }
>
> struct tnum tnum_sub(struct tnum a, struct tnum b)
> @@ -102,7 +102,7 @@ static struct tnum hma(struct tnum acc, u64 value, u64 mask)
> {
> while (mask) {
> if (mask & 1)
> - acc = tnum_add(acc, TNUM(0, value));
> + tnum_add(&acc, &acc, &TNUM(0, value));
This is much less readable than the original, since instead of using the
assignment operator, the destination is just the first argument - not
nearly as self-documenting.
-Ed