Page MenuHomeFreeBSD

Better control over queue core assignment
ClosedPublic

Authored by shurd on Apr 23 2019, 7:09 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23903
Build 22827: arc lint + arc unit

Event Timeline

shurd created this revision.Apr 23 2019, 7:09 PM
shurd added inline comments.Apr 23 2019, 7:11 PM
sys/net/iflib.c
4392

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 requested changes to this revision.Apr 24 2019, 4:54 PM
marius added a subscriber: marius.
marius added inline comments.
sys/net/iflib.c
4384

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).

4390

Please sort variables according to style(9).

4392

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.

4398

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

4415

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().

4417

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.

4426

return (ret);

This revision now requires changes to proceed.Apr 24 2019, 4:54 PM
shurd updated this revision to Diff 56604.Apr 24 2019, 8:31 PM

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

shurd marked 6 inline comments as done.Apr 24 2019, 8:33 PM
shurd marked an inline comment as done.Apr 24 2019, 8:36 PM
shurd added inline comments.
sys/net/iflib.c
4415

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 requested changes to this revision.Apr 25 2019, 11:56 AM
marius added inline comments.
share/man/man4/iflib.4
59

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

62

Ditto

sys/net/iflib.c
192

#define<tab> according to style(9)

4415

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?

4416

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

This revision now requires changes to proceed.Apr 25 2019, 11:56 AM
shurd updated this revision to Diff 56655.Apr 25 2019, 5:40 PM
shurd marked an inline comment as done.

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

Fix some more style(9) problems.

shurd marked 5 inline comments as done.Apr 25 2019, 5:43 PM
shurd added inline comments.
sys/net/iflib.c
4415

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 accepted this revision.Apr 25 2019, 5:45 PM

Looks good to me now, thanks!

This revision is now accepted and ready to land.Apr 25 2019, 5:45 PM
This revision was automatically updated to reflect the committed changes.
shurd marked an inline comment as done.