Page MenuHomeFreeBSD

Better control over queue core assignment

Authored by shurd on Apr 23 2019, 7:09 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
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

shurd created this revision.Apr 23 2019, 7:09 PM
shurd added inline comments.Apr 23 2019, 7:11 PM
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
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.
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)


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?

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

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