Re: [PATCH v6 03/10] crc32-replace-self-test.diff

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 31 Aug 2011 17:29:58 -0500
Bob Pearson <rpearson@systemfabricworks.com> wrote:

> Replaced the unit test provided in crc32.c, which doesn't have a
> makefile and doesn't compile with current headers, with a simpler
> self test routine that also gives a measure of performance and
> runs at module init time. The self test option can be enabled
> through a configuration option CONFIG_CRC32_SELFTEST.
> 
> The test stresses the pre and post loops and is thus not very
> realistic since actual uses will likely have addresses and lengths
> that are at least 4 byte aligned. However, the main loop is long
> enough so that the performance is dominated by that loop.
> 
> The expected values for crc32_le and crc32_be were generated
> with the original version of crc32.c using CRC_BITS_LE = 8 and
> CRC_BITS_BE = 8. These values were then used to check all the
> values of the BITS parameters in both the original and new versions.
> 
> The performance results show some variability from run to run
> in spite of attempts to both warm the cache and reduce the amount
> of OS noise by limiting interrutps during the test. To get comparable
> results and to analyse options wrt performance the best time
> reported over a small sample of runs has been taken.
> 

I don't object to a self-test which actually works, but it seems pretty
lame that the self-test exists in kernel mode when it is so simple to
prepare a much more useful and powerful correctness/performance test
harness in userspace.

> ...
>
> -static u32 test_step(u32 init, unsigned char *buf, size_t len)
> -{
> -	u32 crc1, crc2;
> -	size_t i;
> +		crc ^= crc32_be(test[i].crc, test_buf +
> +		    test[i].start, test[i].length);
> +	}
>  
> -	crc1 = crc32_be(init, buf, len);
> -	store_be(crc1, buf + len);
> -	crc2 = crc32_be(init, buf, len + 4);
> -	if (crc2)
> -		printf("\nCRC cancellation fail: 0x%08x should be 0\n",
> -		       crc2);
> -
> -	for (i = 0; i <= len + 4; i++) {
> -		crc2 = crc32_be(init, buf, i);
> -		crc2 = crc32_be(crc2, buf + i, len + 4 - i);
> -		if (crc2)
> -			printf("\nCRC split fail: 0x%08x\n", crc2);
> +	/* reduce OS noise */

This comment is useless.

> +	local_irq_save(flags);
> +	local_irq_disable();

local_irq_save() already does local_irq_disable().

local_irq_disable() doesn't protect against actions of other CPUs.  I'd
know if this was a bug if the comment wasn't useless :)

> +	getnstimeofday(&start);
> +	for (i = 0; i < 100; i++) {
> +		if (test[i].crc_le != crc32_le(test[i].crc, test_buf +
> +		    test[i].start, test[i].length))
> +			errors++;
> +
> +		if (test[i].crc_be != crc32_be(test[i].crc, test_buf +
> +		    test[i].start, test[i].length))
> +			errors++;
>  	}
> ...