Page MenuHomeFreeBSD

Better control over queue core assignment
ClosedPublic

Authored by shurd on Apr 23 2019, 7:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 4:53 PM
Unknown Object (File)
Mon, Dec 9, 3:37 AM
Unknown Object (File)
Wed, Dec 4, 5:34 PM
Unknown Object (File)
Thu, Nov 28, 4:26 PM
Unknown Object (File)
Thu, Nov 28, 4:26 PM
Unknown Object (File)
Sun, Nov 24, 4:19 AM
Unknown Object (File)
Wed, Nov 20, 9:38 PM
Unknown Object (File)
Nov 15 2024, 7:37 AM
Subscribers

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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23851
Build 22787: arc lint + arc unit

Event Timeline

sys/net/iflib.c
4389

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
4381

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

Please sort variables according to style(9).

4389

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

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

4412

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

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

return (ret);

This revision now requires changes to proceed.Apr 24 2019, 4:54 PM

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

shurd added inline comments.
sys/net/iflib.c
4412

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 ↗(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

#define<tab> according to style(9)

4412

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?

4413

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 marked an inline comment as done.

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

Fix some more style(9) problems.

shurd added inline comments.
sys/net/iflib.c
4412

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.

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.