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)
Feb 12 2024, 12:21 PM
Unknown Object (File)
Dec 23 2023, 4:01 AM
Unknown Object (File)
Dec 20 2023, 6:16 PM
Unknown Object (File)
Dec 7 2023, 3:37 PM
Unknown Object (File)
Nov 28 2023, 10:24 AM
Unknown Object (File)
Nov 25 2023, 6:46 AM
Unknown Object (File)
Nov 21 2023, 11:18 PM
Unknown Object (File)
Nov 19 2023, 4:37 PM
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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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

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

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.