mbox series

[0/1] Add KUnit tests for lib/crc16.c

Message ID 20240922232643.535329-1-vpeixoto@lkcamp.dev
Headers show
Series Add KUnit tests for lib/crc16.c | expand

Message

Vinicius Peixoto Sept. 22, 2024, 11:26 p.m. UTC 
Hi all,

This patch was developed during a hackathon organized by LKCAMP [1],
with the objective of writing KUnit tests, both to introduce people to
the kernel development process and to learn about different subsystems
(with the positive side effect of improving the kernel test coverage, of
course).

We noticed there were tests for CRC32 in lib/crc32test.c and thought it
would be nice to have something similar for CRC16, since it seems to be
widely used in network drivers (as well as in some ext4 code).

Although this patch turned out quite big, most of the LOCs come from
tables containing randomly-generated test data that we use to validate
the kernel's implementation of CRC-16.

We would really appreciate any feedback/suggestions on how to improve
this. Thanks! :-)

Vinicius Peixoto (1):
  lib/crc16_kunit.c: add KUnit tests for crc16

 lib/Kconfig.debug |   8 +
 lib/Makefile      |   1 +
 lib/crc16_kunit.c | 715 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 724 insertions(+)
 create mode 100644 lib/crc16_kunit.c

Comments

Vinicius Peixoto Sept. 30, 2024, 12:57 a.m. UTC | #1 
Hi David,

On 9/26/24 13:21, David Laight wrote:
> ...
>> The checksums for the randomly-generated test cases were calculated
>> using a reference implementation [1] and this test compares them against
>> the values yielded by the kernel's implementation.
> 
> I'd just use a naïve implementation - doesn't really matter
> if it is a bit slow.

Thanks for the feedback. I agree that it makes more sense to use a naive 
implementation to validate the results from the kernel's crc16 instead 
of having a table of pre-computed results. I will include in v2 a 
bog-standard implementation of crc16 similar to yours (using a loop 
instead of a lookup table) to validate the results.

Thanks,
Vinicius

> 
> Slow is relative - this code only takes 35ms to crc-64 over 5MB of data.
> 
> {
>      volatile const uint32_t *r = (const void *)buf;
>      for (crc = 0; r < (const uint32_t *)buf_end; r++) {
>          uint64_t val = le32toh(*r);
>          crc ^= bswap64(val);
>          for (i = 0; i < 32; i++) {
>              if (crc & (1ull << 63))
>                  crc = (crc << 1) ^ 0x42f0e1eba9ea3693ull;
>              else
>                  crc = crc << 1;
>          }
>      }
> }
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)