iflib(4): Replace admin taskqueue group with per-interface taskqueues
Needs ReviewPublic
Actions

Authored by kgalazka on Jul 22 2024, 4:26 PM.

Details

Reviewers

shurd
jhb
jeffrey.e.pieper_intel.com
kbowling

Group Reviewers

iflib

Summary

Using one taskqueue group with single thread to execute all admin
tasks may lead to unexpected timeouts when long running task (e.g.
handling a reset after FW update) for one interface prevents
tasks from other interfaces being executed. Taskqueue group API
doesn't let to dynamically add threads, and pre-allocating thread
for each CPU as it's done for traffic queues would be a waste
of resources on systems with small number of interfaces. Replace
global taskqueue group for admin tasks with taskqueue allocated
for each interface to allow independent execution.

Signed-off-by: Krzysztof Galazka <krzysztof.galazka@intel.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 58764
Build 55652: arc lint + arc unit

Event Timeline

kgalazka created this revision.Jul 22 2024, 4:26 PM

Herald added a reviewer: shurd. · View Herald TranscriptJul 22 2024, 4:26 PM

Herald added a reviewer: shurd. · View Herald Transcript

Herald added a reviewer: iflib. · View Herald Transcript

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald Transcript

kgalazka requested review of this revision.Jul 22 2024, 4:26 PM

kgalazka added a parent revision: D46061: iflib: Simplify iflib_legacy_setup.

Harbormaster completed remote builds in B58764: Diff 141231.Jul 22 2024, 4:26 PM

kgalazka mentioned this in D12142: Don't start grouptaskqueue threads for unused grouptaskqueues.Oct 1 2024, 3:23 PM

Rebased on top of master.
Free taskqueue after releasing interrupts per jhb comment on Github.

Harbormaster completed remote builds in B60062: Diff 145103.Oct 18 2024, 10:28 AM

This looks ok to me, though I'm not intimately familiar with all of iflib.

This revision is now accepted and ready to land.Oct 21 2024, 3:52 PM

kgalazka added a reviewer: jeffrey.e.pieper_intel.com.Oct 21 2024, 4:10 PM

Seems reasonable to me. If there is any work that depends or interacts with these changes it would be nice to see them linked/referenced so we can look at that too but I can't foresee any problems.

sys/net/iflib.c
6346	Can this line be removed entirely?
6408–6409	Can you explain how this works? Looks like we used to have `vflr_task` but this will not be needed anymore with the per interface task which I suppose could do the same work?

In D46062#1076736, @kbowling wrote:

Seems reasonable to me. If there is any work that depends or interacts with these changes it would be nice to see them linked/referenced so we can look at that too but I can't foresee any problems.

There is no dependent work.

sys/net/iflib.c
6408–6409	Uh, I'm sorry, I don't know what I was thinking. I changed iflib_iov_intr_deferred to enqueue a task to admin taskqueue but this task is never initialized. I need to fix that. Thanks for pointing this out.

Add forgotten task init for IFLIB_INTR_IOV soft irq type.

This revision now requires review to proceed.Nov 25 2024, 5:49 PM

Harbormaster completed remote builds in B60763: Diff 146916.Nov 25 2024, 5:49 PM

kbowling accepted this revision.Nov 25 2024, 7:08 PM

This revision is now accepted and ready to land.Nov 25 2024, 7:08 PM

kbowling added inline comments.Nov 26 2024, 4:44 AM

sys/net/iflib.c
5297	Prefix name with `if_`
6408–6409	Does this need cleanup of the taskqueue?

Prefix taskqueue name with 'if_'.

This revision now requires review to proceed.Dec 6 2024, 7:59 PM

Harbormaster completed remote builds in B60987: Diff 147614.Dec 6 2024, 7:59 PM

kgalazka added inline comments.Dec 6 2024, 8:21 PM

sys/net/iflib.c
6346	gtask is used below: info->ifi_task = gtask; but I guess I can add initialization to NULL in the declaration instead. Is that what you mean?
6408–6409	Do you mean something more elaborate than taskqueue_free in iflib_device_deregister?