On 2/3/2026 10:12 PM, Przemek Kitszel wrote:
[...]
If dropping the lock is a no-go, the only viable path forward is to
split the
reset_task so that the waiting portion is decoupled from the netdev_lock
critical section.
We used to do this back before the netdev shaper ops. We didn't
acquire either netdev lock or RTNL during reset.
I thought we had some code in the past that would handle netdev stuff
outside of reset.. but I don't really know and git blame is not making
it easy to find this information.
IIRC it was iavf_config_task that is used to complete stuff requested
under RTNL but later
Perhaps we don't actually need to hold the netdev lock over the reset
task.. except Przemek's refactor to remove the critical lock now makes
us fully dependent on the netdev lock in this case for reset :(
The fact remains that MTU configuration and ring parameter changes are
currently broken in iavf. Changing the MTU on a Virtual Function is a
fundamental configuration not an obscure edge case that can remain
non- functional.
Agreed. This needs a resolution. It is just very tricky to figure out
what the solution should be.
We need to hold the netdev lock during reset, and we need to have our
handlers wait for reset to complete in order to be certain their task
is done... but reset is a separate thread so we can't really
communicate to it that we're holding the lock, and attempts to do that
would be a huge problem.
We don't want to go back to the critical lock and all of its horrible
problems either. The commit that removed it is here: 120f28a6f314
("iavf: get rid of the crit lock")
I would appreciate any further guidance on how you would prefer...
I wish I had some better ideas..
Bad ideas I've thought about so far:
1) this patch with its drop lock and wait, which we discussed as
problematic before. It creates a lot of issues since it means the
operations are no longer atomic and we could potentially get stuck
with some other operation in the event of another thread starting some
core netdev task. No good.
2) not holding netdev_lock in reset, which is now a nogo since we
removed the crit_lock, and apparently we held netdev_lock prior to
that too anyways...
3) we could maybe do some sort of ref counting dance where we take
some reference in threads that queue a reset, and the reset task would
know if that reference was non-zero then another driver thread is
holding netdev_lock so its safe to go into reset without locking...
but this feels extremely ugly and error prone to me...
4) convert reset handling to a separate function that depends on the
netdev_lock, and call that directly from within the threads that
currently "wait for reset" while holding the netdev lock. Thus, we
basically move this entire call chain into the thread already holding
the lock, and call it from the context of the function like the MTU
change, etc.
reset_thread() {
while (!stopped) {
netdev_lock();
reset_step();
netdev_unlock();
}
}
looks cool, IIRC I did something similar with the "after crit lock
removal refactor continuation", but I've put it on hold
https://github.com/pkitszel/linux/commits/undeadlock/
The linked code went even further and merged all of our admin-worker
threads into one and the whole was protected by the netdev_lock :)
That is a lot of patches and doesn't feel like a minimal fix.
This feels like its also a huge issue, and could
potentially lead to some sort of issue where we need to still block
the reset thread from going if we reset at the end of the netdev_lock
thread..
that should queue on the "do later stuff queue", which we don't have
right now (and keep some of such stuff in the state machine), but would
be useful for many other actions too (like virtchnl messages, for which
we have queue-of-1 right now
I think we need to lay out what we need and maybe that will help figure
out a solution.
1) several NDO operations need to perform tasks that require AVF
hardware to reset, and which need to be certain that happens before they
return.
2) reset is handled by a separate workqueue task
3) reset also requires the netdev lock
We can't make the ndo operations avoid the netdev lock
We pretty much can't make reset not require the netdev lock, especially
since we are now using it as effectively the main driver lock now.
I am not certain we can move all of the reset code to work from the ndo
thread context, as we still also have to wait for hardware notification
that reset happened, which seems like a pretty massive refactor to get
right.
We can't drop the waiting in the ndo operation because we need to be
sure that the operation applied properly, otherwise we get errors elsewhere.
So that leaves me without any solution so far, certainly not one that
can be completed quickly... which means we're stuck with a guaranteed
deadlock anytime the MTU is changed?
I don't really like any of these solutions, even if (3) and (4) aren't
fully ruled out as completely broken... they probably have all kinds
of issues...
