Re: [PATCH] kernel:bpf Remove structure passing and assignment to save stack and no coping structures

[Edward Cree]

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 <karim.eshapa@xxxxxxxxx>
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