DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Yerden Zhumabekov <e_zhumabekov@sts.kz>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] app/test: add crc32 algorithms equivalence check
Date: Tue, 24 Feb 2015 14:57:34 +0000	[thread overview]
Message-ID: <20150224145734.GA9192@bricha3-MOBL3> (raw)
In-Reply-To: <1424781388-7485-1-git-send-email-e_zhumabekov@sts.kz>

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
> 

  reply	other threads:[~2015-02-24 14:57 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-24 12:36 Yerden Zhumabekov
2015-02-24 14:57 ` Bruce Richardson [this message]
2015-02-25  3:14   ` Yerden Zhumabekov
2015-02-25  4:08 ` [dpdk-dev] [PATCH v2] " Yerden Zhumabekov
2015-02-25 11:34   ` Bruce Richardson
2015-02-25 12:36     ` Yerden Zhumabekov
2015-02-25 12:34 ` [dpdk-dev] [PATCH v3] " Yerden Zhumabekov
2015-02-25 13:14   ` Bruce Richardson
2015-02-25 15:19     ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150224145734.GA9192@bricha3-MOBL3 \
    --to=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=e_zhumabekov@sts.kz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).