Subject: Re: [PATCH] kernel:bpf Remove structure passing and assignment to
 save stack and no coping structures
To: Karim Eshapa <karim.eshapa@gmail.com>, <ast@kernel.org>
CC: <daniel@iogearbox.net>, <davem@davemloft.net>,
        <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>
References: <1515881022-15237-1-git-send-email-karim.eshapa@gmail.com>
From: Edward Cree <ecree@solarflare.com>
Message-ID: <f954c087-2813-ed9b-ccd3-5c2543c06b41@solarflare.com>
Date: Tue, 16 Jan 2018 13:55:56 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <1515881022-15237-1-git-send-email-karim.eshapa@gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Sender: linux-kernel-owner@vger.kernel.org

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@gmail.com>
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