Details
- Reviewers
kp - Group Reviewers
network - Commits
- rG8aaffd78c0f5: Add dummymbuf module for testing purposes
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
This is a quick prototype with some edges to polish or re-work completely. It would be interesting to hear opinions before the further effort. The context and use case example are outlined here: https://reviews.freebsd.org/D45927.
It might be good to send a brief overview mail of this and what it's intended for to the testing@ and network@ lists.
It's an interesting approach to allow us to test scenarios that we currently don't have a way of producing.
A slight update: "count" related code needed revising due to it was "offset" in the previous life (as of my first internal iterations).
| sys/modules/dummymbuf/Makefile | ||
|---|---|---|
| 1 | It's not necessary to leave an empty line at the beginning of a newly created Makefile. It was a leftover from removing the VCS line. | |
We're also missing a little more make glue:
diff --git a/sys/conf/files b/sys/conf/files index 1f99c3586b86..e0cb4fef10cf 100644 --- a/sys/conf/files +++ b/sys/conf/files @@ -4189,6 +4189,7 @@ net/debugnet_inet.c optional inet debugnet net/pfil.c optional ether | inet net/radix.c standard net/route.c standard +net/dummymbuf.c optional dummymbuf net/route/nhgrp.c optional route_mpath net/route/nhgrp_ctl.c optional route_mpath net/route/nhop.c standard diff --git a/sys/modules/Makefile b/sys/modules/Makefile index be7539e9eae1..60937c536d51 100644 --- a/sys/modules/Makefile +++ b/sys/modules/Makefile @@ -102,6 +102,7 @@ SUBDIR= \ ${_dpdk_lpm6} \ ${_dpms} \ dummynet \ + dummymbuf \ ${_dwwdt} \ ${_e6000sw} \ ${_efirt} \
| sys/net/dummymbuf.c | ||
|---|---|---|
| 243 | We're relying on the order of module load here. That is, there's nothing to ensure that the dummymbuf pfil hook gets called before the pf/ipfw/ipf one does. I don't immediately have any good ideas for how to handle this problem. | |
The update includes:
- Apply the review suggestions collected so far
- Rework struct op to struct rule to let the interface and its implementation speak the same language
| sys/modules/dummymbuf/Makefile | ||
|---|---|---|
| 1 | Thanks for the confirmation, it resolves my doubts formed after a quick check of several existing modules. | |
| sys/net/dummymbuf.c | ||
| 243 | Yes, this was my cornerstone question of the whole idea. Currently, it's resolved with the following reasoning. The load order is not expected to be an issue due to dummymbuf expects from a user to decide on the linking time. A user is expected to use something like pfilctl(8) for linking. It guarantees to prepend or append a hook. I've tested all four cases to make sure: -i, -i -a, and -o, -o -a. The cases we want are -i and -o -a -- they put dummymbuf hook before the firewalls. That's the technical side. Now it seems to have at least two logical branches from usage perspective:
What do you think? | |
Thanks for the help with closing one of the TODOs. It saved my time -- no need to refresh the info regarding a module integration into the build.
| sys/net/dummymbuf.c | ||
|---|---|---|
| 243 | Oh, I see, yes. I mistakenly assumed that 'pfil_add_hook()' actually hooked us in, but that would have to be 'pfil_link()'. | |
This update brings in the simplest logging to provide some details if it does not work. I believe that it's a must for this module to save time and minimize possible headache of its users. Also, it may add a little help for the testers and reviewers of the patch.
| sys/net/dummymbuf.c | ||
|---|---|---|
| 243 | Yes, good point. It seems that basically a test author is responsible for the respective cleanup. The only help from our side could be to remind it in a man page. And it seems that the majority of the tests, which are users of tests/sys/common/vnet.subr, are covered automagically if they do not forget to use vnet_cleanup(). So that, the hooks are get removed along with their VNET instance without extra code. | |
I think I like this, overall.
Doing string parsing in the kernel is always a little risky, but given that this is test code (and requires root to enable and configure anyway) it's fine.
(An alternative design would be to do the parsing in userspace, and use netlink or ioctl calls to send a more structured version to the kernel. I think I'd insist on that for a feature production users would use, but not for this. For this small and self-contained is better.)
I'd like to see it grown the IPv6 and Ethernet support and a man page before we land it. There's clearly possibilities for more mbuf shaping tricks, but we can worry about those when we run into specific bugs. That does not need to go in the first version of this.
| sys/net/dummymbuf.c | ||
|---|---|---|
| 50 | I wonder if we should dynamically allocate this space when the rules get updated. That's certainly not blocking though. | |
| 208 | It'd be tempting to try to be clever with the locking and do something epoch or rmlock based, but given the use case for this I'd just use a very simple mutex. It'll be easier to do, easier to understand and plenty fast enough for tests. | |
Yes, the idea was to keep it simple.
| sys/net/dummymbuf.c | ||
|---|---|---|
| 50 | I should have added a TODO here. Even after halving from 1024 to 512 this is still about [ O(n), O(n) ] space complexity if we think of huge n of jails. Now it's [ O(1), O(n) ], with O(1) or O(ncpu) in practice. | |
It has been tested for IPv4, IPv6, and Ethernet with the respective fixes on pf side and extra tests in the mbuf.sh. It will be easier to publish them for review after the initial IPv4 case (https://reviews.freebsd.org/D45927) and the module itself get merged.
The man page is pending.
Added the initial draft of dummymbuf.4 man page. Any advice is welcome to help making it as concise and clear as possible.
Non-functional changes:
- Always reset rule.syntax_* fields before the actual parsing
- Use static MALLOC_DEFINE
I think the man page could benefit from an editing pass by a native speaker, but it's already about 98% as good as it can be.
| share/man/man4/dummymbuf.4 | ||
|---|---|---|
| 50 ↗ | (On Diff #141853) | I think I'd like the description to start with something like "This module is intended to test networking code in the face of unusual mbuf layouts.". Or something else, point is that I'd like the first thing users see in the man page to be something that indicates this is a test tool. |
| 52 ↗ | (On Diff #141853) | which can be listed with ? |
| 54 ↗ | (On Diff #141853) | as follows or for example? |
| 62 ↗ | (On Diff #141853) | To activate a hook it .. |
| 73 ↗ | (On Diff #141853) | evaluated sequentially? each matching rule. |
| 142 ↗ | (On Diff #141853) | *sequence |
| 188 ↗ | (On Diff #141853) | the rules? |
| sys/net/dummymbuf.c | ||
| 86 | I usually have macros for locking operations, (e.g. PF_RULES_RLOCK, OVPN_RLOCK, ...). That's not required, but I find it makes it easier to spot locking operations in the code. | |
| 88 | Do we not need to free this memory afterwards? | |
| 119 | Is it worth having this? I think we can just trust VNET_SYSINIT/VNET_SYSUNINIT to only be called once per vnet. | |
Thank you very much for the first aid here.
| share/man/man4/dummymbuf.4 | ||
|---|---|---|
| 50 ↗ | (On Diff #141853) | It looks I was thinking about more abstract description, to show what it is from the pure technical perspective, with the feeling of various application possibility, and mention existing use cases in the examples section. But now I think that it's really better to jump directly to the today's point. |
| sys/net/dummymbuf.c | ||
| 86 | Thanks. I've forgot about this best practice in the kernel. | |
| 88 | It's freed upon vnet uninit by dmb_vnet_uninit(). Or I miss some aspect here? | |
| 119 | Indeed. | |
A non-functional update:
- Align naming of vnet (un)init functions with the others in the kernel
I think this is about ready to land. I'll give others another few days to comment, but I intend to commit this (and the test case that builds on it) next week.
| sys/net/dummymbuf.c | ||
|---|---|---|
| 88 | No, I missed that rulesp is V_dmb_rules. I had assumed that it was only a local variable, but it's actually arg1, which is V_dmb_rules | |