Received-SPF: pass (google.com: domain of linux-kernel+bounces-164504-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 30 Apr 2024 10:24:03 -0700
From: Yury Norov <yury.norov@gmail.com>
To: Kuan-Wei Chiu <visitorckw@gmail.com>
Cc: akpm@linux-foundation.org, linux@rasmusvillemoes.dk,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] lib/find_bit_benchmark: Add benchmark test for
 fns()
Message-ID: <ZjEpMy3hFXqfavva@yury-ThinkPad>
References: <20240430054912.124237-1-visitorckw@gmail.com>
 <20240430054912.124237-2-visitorckw@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240430054912.124237-2-visitorckw@gmail.com>

On Tue, Apr 30, 2024 at 01:49:11PM +0800, Kuan-Wei Chiu wrote:
> Introduce a benchmark test for the fns(). It measures the total time
> taken by fns() to process 1,000,000 test data generated using
> get_random_long() for each n in the range [0, BITS_PER_LONG].

Can you also print an example of test output?
 
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>  lib/find_bit_benchmark.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/lib/find_bit_benchmark.c b/lib/find_bit_benchmark.c
> index d3fb09e6eff1..8712eacf3bbd 100644
> --- a/lib/find_bit_benchmark.c
> +++ b/lib/find_bit_benchmark.c
> @@ -146,6 +146,28 @@ static int __init test_find_next_and_bit(const void *bitmap,
>  	return 0;
>  }
>  
> +static int __init test_fns(void)
> +{
> +	const unsigned long round = 1000000;
> +	s64 time[BITS_PER_LONG + 1];
> +	unsigned int i, n;
> +	volatile unsigned long x, y;
> +
> +	for (n = 0; n <= BITS_PER_LONG; n++) {

n == BITS_PER_LONG is an error. Testing error case together with
normal cases is even worse error because it fools readers.

> +		time[n] = ktime_get();
> +		for (i = 0; i < round; i++) {
> +			x = get_random_long();
> +			y = fns(x, n);
> +		}

Here you count fns() + get_random_long() time. For your microbench
purposes it would be better exclude a random number generation
overhead.

> +		time[n] = ktime_get() - time[n];
> +	}
> +
> +	for (n = 0; n <= BITS_PER_LONG; n++)
> +		pr_err("fns: n = %2u: %12lld ns\n", n, time[n]);

Nah, not like that. Each test in there prints one line in the
report. Let's keep it that way for test_fns() too. Unless we have
a strong evidence that fns() for a particular input is worth to be
tracked separately, let's just print a total gross?

> +
> +	return 0;
> +}

I'd suggest to modify it like:

        static unsigned long buf[1000000];

        static int __init test_fns(void)
        {
                get_random_bytes(buf, ARRAY_SIZE(buf));
                time = ktime_get();

                for (n = 0; n < BITS_PER_LONG; n++)
                        for (i = 0; i < 1000000; i++)
                                fns(buf[i], n);

                time = ktime_get() - time;
                pr_err(...);
        }

>  static int __init find_bit_test(void)
>  {
>  	unsigned long nbits = BITMAP_LEN / SPARSE;
> @@ -186,6 +208,9 @@ static int __init find_bit_test(void)
>  	test_find_first_and_bit(bitmap, bitmap2, BITMAP_LEN);
>  	test_find_next_and_bit(bitmap, bitmap2, BITMAP_LEN);
>  
> +	pr_err("\nStart testing for fns()\n");
> +	test_fns();

There are 2 sections in the test - one for regular, and another for
sparse data. Adding a new section for a just one function doesn't look
like a good idea.

Even more, the fns() is already tested here. Maybe test_bitops is a
better place for this test?

> +
>  	/*
>  	 * Everything is OK. Return error just to let user run benchmark
>  	 * again without annoying rmmod.
> -- 
> 2.34.1