Page MenuHomeFreeBSD

Better control over queue core assignment

Authored by shurd on Apr 23 2019, 7:09 PM.
Referenced Files
F61984034: D20029.id56669.diff
Mon, May 29, 3:18 PM
Unknown Object (File)
Mon, May 8, 11:13 PM
Unknown Object (File)
Mon, May 8, 11:09 PM
Unknown Object (File)
Apr 10 2023, 12:56 AM
Unknown Object (File)
Mar 17 2023, 5:00 AM
Unknown Object (File)
Mar 6 2023, 6:58 PM
Unknown Object (File)
Feb 25 2023, 1:31 PM
Unknown Object (File)
Feb 22 2023, 5:06 PM



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

rS FreeBSD src repository - subversion
Lint Not Applicable
Tests Not Applicable

Event Timeline

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


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