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

[Bob Pearson]

> -----Original Message-----
> From: Andrew Morton [mailto:akpm@xxxxxxxxxxxxxxxxxxxx]
> Sent: Friday, September 02, 2011 6:52 PM
> To: Bob Pearson
> Cc: linux-kernel@xxxxxxxxxxxxxxx; fzago@xxxxxxxxxxxxxxxxxxxxx; Joakim
> Tjernlund; George Spelvin
> Subject: Re: [PATCH v6 03/10] crc32-replace-self-test.diff
> 
> On Wed, 31 Aug 2011 17:29:58 -0500
> Bob Pearson <rpearson@xxxxxxxxxxxxxxxxxxxxx> 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.

I wasn't trying to claim perfection here. The variance in resulting
performance results was significantly reduced. I didn't make a detailed
statistical study but the range of outliers was smaller by at least 10X.
Without this change the effect of the coding changes that were being debated
was swamped by random variation.

> 
> > +	local_irq_save(flags);
> > +	local_irq_disable();
> 
> local_irq_save() already does local_irq_disable().

OK

> 
> 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++;
> >  	}
> > ...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/