netmap: add suite of unit tests

[vmaffione]

Summary

Import the unit tests from upstream (https://github.com/luigirizzo/netmap 2b6674f ),
and make them ready for use with Kyua.
There are currently 38 regression tests, which test the kernel control ABI exposed by
netmap to userspace applications:

1: test for port info get
2-5: tests for basic port registration
6-9: tests for VALE
10-11: tests for getting netmap allocator info
12-15: tests for netmap pipes
16: test on polling mode
17-18: tests on options
19-27: tests for sync-kloop subsystem
28-39: tests for null ports
31-38: tests for the legacy NIOCREGIF registers

Test Plan

Tested with Kyua

1. kyua test -k /usr/tests/sys/netmap/Kyuafile

[ngie]

tests/sys/netmap/Makefile

8 ↗

(On Diff #51775)

At the very least, WARNS needs to be enabled on this test.

tests/sys/netmap/ctrl-api-test.c

56 ↗

(On Diff #51775)

Why is this commented out?

334–335 ↗

(On Diff #51775)

Crashing a test program because this assert was triggered at some point in the test program can be difficult to trace down if the issue is transient.

697–702 ↗

(On Diff #51775)

Modifying a stack variable, assigning it to a field in a heap variable provided by the function, then doing some magic to copy it around, whilst it still being attached to the context, potentially... Seems a bit risky and relies heavily on the implementation making a copy of the value.

776–781 ↗

(On Diff #51775)

Is there a bug open for this?

866 ↗

(On Diff #51775)

This can't be reconfigured via sysctl?

914 ↗

(On Diff #51775)

Again, why 22?

[vmaffione]

tests/sys/netmap/ctrl-api-test.c

56 ↗

(On Diff #51775)

Sorry, a leftover I forgot.

334–335 ↗

(On Diff #51775)

This check was meant to make sure num_registered_rings() is only called after port_register().
However, I can remove it since the caller already asserts csb_size > 0

697–702 ↗

(On Diff #51775)

Indeed, thanks for spotting this.
I changed it to avoid string pointers to stack variables.

776–781 ↗

(On Diff #51775)

No, because it's an experimental (niche) feature implemented on Linux.
FreeBSD just does not implement it, and we don't plan to do that (since there is no interest for now).

866 ↗

(On Diff #51775)

This netmap feature is supported only on Linux for now, where module parameters can be changed by writing to /sys/module/xxxx.

904 ↗

(On Diff #51775)

It's an arbitrary value, but large enough to hold netmap memory mapped data structures.

1373–1375 ↗

(On Diff #51775)

Right, I converted the example to use POSIX semaphores.

1560 ↗

(On Diff #51775)

Ok, thanks for the suggestion.

1664–1668 ↗

(On Diff #51775)

Yes. I added some randomization on the TAP interface name to improve a bit the situation.

1669 ↗

(On Diff #51775)

Yes, that was the simple approach.
Changed to use fork and exec.

1679 ↗

(On Diff #51775)

Yes, noted in the Makefile.

[vmaffione]

Addressed reviewers' comments.

[vmaffione]

This regression test suite is also run on Linux for continuous integration (see the travis page for more details https://travis-ci.org/netmap-unipi/netmap) and manual tests.
The compatibility with Linux is the the reason why it is a plain C program, so that I don't have two maintain two versions of the same test suite.

This file is part of the netmap code, which I'm actively maintaining (as a source committer).
I'll keep this aligned with the netmap code, to ensure consistency.

[vmaffione]

Fixed some compilation warnings.

[vmaffione]

(errata)

[vmaffione]

Hi,

Many thanks for the thorough review.

Does it look good enough for commit now?

Thanks

[ngie]

tests/sys/netmap/Makefile

12 ↗

(On Diff #51813)

https://tenor.com/search/its-a-bold-move-gifs

tests/sys/netmap/ctrl-api-test.c

1–18 ↗

(On Diff #51813)

Could you please sort this per style(9), if this is going to be committed directly upstream?

24 ↗

(On Diff #51813)

eventfd(int x __unused, int y __unused) is the proper FreeBSD way of doing things

28 ↗

(On Diff #51813)

Please use ENODEV instead.

83 ↗

(On Diff #51813)

Why not capture the return code from waitpid?

Technically, if waitpid fails, the value recorded in child_status is suspect.

96–118 ↗

(On Diff #51813)

This structure is very inefficiently aligned. Is there a reason why it's structured this way?

148 ↗

(On Diff #51813)

if (ret != 0) { would be better.

165 ↗

(On Diff #51813)

Missing space between . and */.

203 ↗

(On Diff #51813)

+1 for the != 0 comment I made above.

242 ↗

(On Diff #51813)

-0?! Should this be 0?

360 ↗

(On Diff #51813)

Why 20?

369 ↗

(On Diff #51813)

Why 20?

446 ↗

(On Diff #51813)

Why 256?

505 ↗

(On Diff #51813)

Using != 0 will make the contract more explicit.

569 ↗

(On Diff #51813)

Why 12?

620–622 ↗

(On Diff #51813)

Why should the return value from ioctl be ignored?

1406 ↗

(On Diff #51813)

This should be checked as it can fail.

904 ↗

(On Diff #51775)

How is this value derived? It should be a more clearly named constant.

[vmaffione]

tests/sys/netmap/Makefile

12 ↗

(On Diff #51813)

Wasn't that the proper way to "enable WARNS" for this test?
I'm sorry, but I couldn't find any documentation about this.

tests/sys/netmap/ctrl-api-test.c

96–118 ↗

(On Diff #51813)

Just for readability. Anyway, I rearranged it a little bit to save some memory.

242 ↗

(On Diff #51813)

ofc, it's just a typo.
Thanks for spotting this.

360 ↗

(On Diff #51813)

Arbitrary value, added a comment for that.

369 ↗

(On Diff #51813)

Arbitrary value. Added a comment for that.

446 ↗

(On Diff #51813)

Minimal value specified.

569 ↗

(On Diff #51813)

Added a comment for that.

620–622 ↗

(On Diff #51813)

If vale_attach_detach was successful, the return value of the DELIF ioctl() is not ignored.
If not successful, this function returns the return value of vale_attach_detach, which is the first error that occurred, independently on the DELIF ioctl failing or not.

[vmaffione]

Addressed more reviewer's comments.

[ngie]

tests/sys/netmap/Makefile

12 ↗

(On Diff #51813)

Totally valid. I'm just poking a bit of fun about the fact that the warnings are now bumped up to their maximum value :).

tests/sys/netmap/ctrl-api-test.c

96–118 ↗

(On Diff #51813)

This is actually very inefficient alignment wise.

It should be:

pointers-sorted-by-name /* ... since the pointers are all the same size */
scalars-sorted-by-stack-size

It shouldn't matter by default due to compiler optimizations, but will matter if the code is compiled with -O0 (no optimizations).

[ngie]

tests/sys/netmap/ctrl-api-test.c

1819 ↗

(On Diff #52081)

Why is this over-allocating 13 slots for FreeBSD and 12 for Linux?

[vmaffione]

tests/sys/netmap/Makefile

12 ↗

(On Diff #51813)

Oh well, the stricter the better :)
Also because we have been compiling that with -Wall so far, so no new warnings should show up.

tests/sys/netmap/ctrl-api-test.c

1819 ↗

(On Diff #52081)

I just assumed it does not matter. I refined this a bit and added some assertions.

[vmaffione]

Addressed more reviewer's comments.

[vmaffione]

tests/sys/netmap/ctrl-api-test.c

96–118 ↗

(On Diff #51813)

But as they are now they are aligned reasonably well. All the fields are naturally aligned, and they can all grouped to fill 64-bit words without leaving any hole... Also, the pointers are all together, and there are also four of them, which means that they fill a whole number of 64-bit words also on 32bit architectures.
I prefer to keep the pointers at the end of the struct for readability, as they are "less important" (not used in all the tests).
The order of the nr_* fields just follows the same order as they appear in struct nmreq_register in net/netmap.h.

In any case, I don't see how the alignment in this struct could possibly matter, considering the very limited effect it may have on memory footprint (maybe ~16 bytes size variation), number of cache misses required for the CPU to work on that struct, and even the cost of unaligned memory access (there are none here afaiu). These tests exercise some kernel control ABI, and I don't see any performance-related concerns.

Does it make sense?

[vmaffione]

tests/sys/netmap/ctrl-api-test.c

904 ↗

(On Diff #51775)

I rearranged the code to hopefully make this more clear.

[vmaffione]

Improve extmem tests.

[ngie]

I'll look over the code more in-depth tonight. You've done a lot of things to improve it. Just need to make sure it functions in the negative case (when netmap isn't available).

tests/sys/netmap/ctrl-api-test.c

1869 ↗

(On Diff #52104)

I think that getting to this point would have already caused a segfault :/...

[vmaffione]

Sounds good.
In case netmap is not available, you should get something similar

`
Executing command: ifconfig create tap7361 up
==> Start of Test #1 [port_info_get]
open(/dev/netmap): No such file or directory
Executing command: ifconfig destroy tap7361
`

tests/sys/netmap/ctrl-api-test.c

1869 ↗

(On Diff #52104)

That's right, I added a macro to avoid segfaults in cause of insufficient size.

[vmaffione]

Make sure av[] on-stack arrays do not overflow.

[vmaffione]

Any more comments? Can I go ahead and commit this?
Thanks

[ngie]

In D18490#397458, @v.maffione_gmail.com wrote:

Any more comments? Can I go ahead and commit this?
Thanks

Yes, a couple...

1. There's an issue with the mtree file indentation. You need to use spaces, not hard tabs. This is a commit blocking comment, because it will break installing tests.
2. This test fails if tap isn't in the kernel -- it should probably check for the module's existence before failing with an exit code of 255, or just fail with a non-zero error code and claim the test results questionable:

$ sudo kyua debug -k /usr/tests/sys/netmap/Kyuafile ctrl-api-test:main                                                                                                
Executing command: ifconfig tap4367 create up
Failed to create tap interface
ctrl-api-test:main  ->  failed: Returned non-success exit status 255

This is arguably a commit blocking item as well, due to the fact that if_tap can be omitted from the kernel and this would make all kernels without it fail with false positives.

This is part of the reason why I suggested using a structured test reporting mechanism/protocol; it is so much better than hand rolling the equivalent logic. Furthermore, the plain test format in kyua is very rudimentary (success: 0; failure otherwise), so you cannot easily express setup/cleanup failures.

TAP supports a SKIP directive, which would clearly identify this issue as a problem with setting up the environment, not resulting in a test failure (false negative) or test success (false positive). The catch is that you'd have to have an exit handler setup (via atexit in C, trap in /bin/sh, etc) to tear down the tap interfaces (this is an issue with the existing code as well -- it will leave if_tap interfaces behind if interrupted).

You could also move that portion of the logic out into a script, which would shift the test program focus on executing the test cases, as opposed to setting up the environment needed to execute them in.

etc/mtree/BSD.tests.dist

763 ↗

(On Diff #52157)

When I applied the patch, this indentation is off (using hard tabs). You need to indent with spaces in *.dist mtree files.

[vmaffione]

Formatted .dist file.

[ngie]

Looks great to me! Thank you very much for all your work on the diff :)...

As a next step, please convert this to TAP and add a SKIP directive when the if_tap interface cannot be created (that, or I could do the conversion).

[vmaffione]

Thank you very much for your thorough review!
I will try to convert that to a TAP test, and ask for help if I find obstacles.