Better control over queue core assignment

[shurd]

Summary

By default, cores are now assigned to queues in a sequential
manner rather than all NICs starting at the first core. On a four-core
system with two NICs each using two queue pairs, the nic:queue -> core
mapping has changed from this:

0:0 -> 0
0:1 -> 1
1:0 -> 0
1:1 -> 1

To this:

0:0 -> 0
0:1 -> 1
1:0 -> 2
1:1 -> 3

Additionally, a device can be configured to use separate cores for TX
and RX queues.

Two new tunables have been added, dev.X.Y.iflib.separate_txrx and
dev.X.Y.iflib.core_offset. If core_offset is set, the NIC is not part
of the auto-assigned sequence.

Test Plan

Ensure performance improves. Also, test performance of
forwarding with RX and TX queues on separate cores.

[shurd]

sys/net/iflib.c

4389 ↗

(On Diff #56549)

Just to highlight this... this is, in fact, terrible. I didn't want to use hints to ensure iflib is in the name, and didn't find any other immediately obvious method to have a non-zero default tunable value. Something less terrible would be great.

[marius]

sys/net/iflib.c

4381 ↗

(On Diff #56549)

Please place the function type on its own line as suggested by style(9) (so searching for the implementation via grep ^get_ctx_core_offset works).

4387 ↗

(On Diff #56549)

Please sort variables according to style(9).

4389 ↗

(On Diff #56549)

Hum, what happens if you simply set ctx->ifc_sysctl_core_offset to 0xffff before calling SYSCTL_ADD_U16(9)? It doesn't look like sysctl(9) has special code to initialize dynamically added SYSCTLs to 0 and in the usual case, their default value of 0 is simply preserved.
If that doesn't work and you keep implementing your own kenv fetching, you probably should add this SYSCTL using CTLFLAG_RDTUN | CTLFLAG_NOFETCH to keep sysctl(9) from doing its own fetching.

4395 ↗

(On Diff #56549)

Please enclose the returned valued in parentheses a la style(9).

4412 ↗

(On Diff #56549)

Looks like this memory will be leaked when a device goes away. Similarly, the whole cpu_offset entry probably should be removed from the list again in iflib_device_deregister().

4414 ↗

(On Diff #56549)

It doesn't look like a good idea to allocate memory using M_NOWAIT and then to immediately panic if malloc(9) failed, especially if the code below looks like it would be handling the allocation failure gracefully.

4423 ↗

(On Diff #56549)

return (ret);

[shurd]

Fix style(), clean up default sysctl values, log message on malloc() failure.

[shurd]

sys/net/iflib.c

4412 ↗

(On Diff #56549)

This allocation is shared by all iflib drivers. I've added a comment in iflib_module_event_handler() in case MOD_UNLOAD is ever change to return zero.

[marius]

share/man/man4/iflib.4

59 ↗

(On Diff #56604)

No hard sentence breaks in man pages, i. e. start the new sentence in a new line

62 ↗

(On Diff #56604)

Ditto

sys/net/iflib.c

192 ↗

(On Diff #56604)

#define<tab> according to style(9)

4415 ↗

(On Diff #56604)

style(9): "Closing and opening braces go on the same line as the else."

4412 ↗

(On Diff #56549)

Hum, can't you just add a reference count to the cpu_offest entries and once the count for a particular CPU set drops to 0 on iflib_device_deregister(), remove that entry from the list and free its memory?

[shurd]

"Reference" count core offsets and free() when all drivers unloaded.

Fix some more style(9) problems.

[shurd]

sys/net/iflib.c

4412 ↗

(On Diff #56549)

It feels a bit weird to reference-count it since the driver doesn't actually have a reference, but at least this prevents it from leaking in the future.

[marius]

Looks good to me now, thanks!