Re: [dpdk-dev] [PATCH] app/test: add crc32 algorithms equivalence check

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Feb 24, 2015 at 06:36:28PM +0600, Yerden Zhumabekov wrote:
> New function test_crc32_hash_alg_equiv() checks whether software,
> 4-byte operand and 8-byte operand versions of CRC32 hash function
> implementations return the same result value.
> 
> Signed-off-by: Yerden Zhumabekov <e_zhumabekov@sts.kz>

Looks like what we want for this.
In 2.1 we should perhaps look at splitting off unit tests for the hash algorithms
from the tests for the hash tables themselves, since they are all in one file.
However, I don't think such rework is feasible in 2.0 timeframe.

Some comments inline.

/Bruce

> ---
>  app/test/test_hash.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/app/test/test_hash.c b/app/test/test_hash.c
> index 76b1b8f..941dc69 100644
> --- a/app/test/test_hash.c
> +++ b/app/test/test_hash.c
> @@ -177,6 +177,56 @@ static struct rte_hash_parameters ut_params = {
>  	.socket_id = 0,
>  };
>  
> +#define CRC32_ITERATIONS (1U << 16)
This test takes almost no time at all, so maybe we want to do a few more
iterations e.g. 2^18 - 2^20. 

> +#define CRC32_DWORDS (1U << 6)
> +/*
> + * Test if all CRC32 implementations yield the same hash value
> + */
> +static int
> +test_crc32_hash_alg_equiv(void)
> +{
> +	uint32_t hash_val;
> +	uint32_t init_val;
> +	uint64_t data64[CRC32_DWORDS];
> +	unsigned i, j;
> +	size_t data_len;
> +	int res = 0;
> +
> +	printf("# CRC32 implementations equivalence test\n");
> +	for (i = 0; i < CRC32_ITERATIONS; i++) {
> +		/* Randomizing data_len of data set */
> +		data_len = (size_t) (rte_rand() % sizeof(data64) + 1);
I suggest parenthesis around the % operation for clarity.

> +		init_val = (uint32_t) rte_rand();
> +
> +		/* Fill the data set */
> +		for (j = 0; j < CRC32_DWORDS; j++) {
> +			data64[j] = rte_rand();
> +		}
As a matter of style, we generally omit braces for single-statement loop bodies.

> +
> +		/* Calculate software CRC32 */
> +		rte_hash_crc_set_alg(CRC32_SW);
> +		hash_val = rte_hash_crc(data64, data_len, init_val);
> +
> +		/* Check against 4-byte-operand sse4.2 CRC32 if available */
> +		rte_hash_crc_set_alg(CRC32_SSE42);
> +		if (hash_val != rte_hash_crc(data64, data_len, init_val)) {
> +			res = -1;
I think you need a print statement here, stating that the test failed, and
why exactly it failed.
Also, rather than setting res to -1, you can just do a print and break, and
change "return res" below to "return i == CRC32_ITERATIONS ? 0 : -1", making
use of the fact that you can check i to detect early termination on error.

> +			break;
> +		}
> +
> +		/* Check against 8-byte-operand sse4.2 CRC32 if available */
> +		rte_hash_crc_set_alg(CRC32_SSE42_x64);
> +		if (hash_val != rte_hash_crc(data64, data_len, init_val)) {
> +			res = -1;
> +			break;
> +		}
> +	}
> +
> +	/* Resetting to best available algorithm */
> +	rte_hash_crc_set_alg(CRC32_SSE42_x64);
> +	return res;
> +}
> +
>  /*
>   * Test a hash function.
>   */
> @@ -1356,6 +1406,9 @@ test_hash(void)
>  
>  	run_hash_func_tests();
>  
> +	if (test_crc32_hash_alg_equiv() < 0)
> +		return -1;
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.9.5
>