Skip to content

Don't walk route_ht while holding peertab->lock - #147

Open
randomizedcoder wants to merge 1 commit into
PlatformLab:mainfrom
randomizedcoder:fix/peertab-lock-rhashtable-walk
Open

randomizedcoder wants to merge 1 commit into
PlatformLab:mainfrom
randomizedcoder:fix/peertab-lock-rhashtable-walk

Conversation

@randomizedcoder

Copy link
Copy Markdown
Contributor

rhashtable_walk_enter, _start and _stop take the table's ht->lock with plain spin_lock, and the rehash worker holds it with softirqs enabled. peertab->lock is taken in softirq (homa_route_get from homa_rpc_alloc_server, homa_route_free from an RCU callback). Two places hold peertab->lock while walking route_ht:

  • homa_peer_free_net (netns exit);
  • homa_route_gc → homa_route_pick_victims.

That can hard-hang: CPU0's rehash worker holds ht->lock and a Homa softirq on CPU0 spins on peertab->lock, while CPU1 holds peertab->lock and spins on ht->lock. It's rare (a rehash, a netns exit or GC, and traffic at the same moment), but lockdep reports it on every run that exits a netns:

WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
kworker (cleanup_net) is trying to acquire &ht->lock at rhashtable_walk_enter
and this task is already holding &peertab->lock at homa_peer_free_net

Fix:

  • New peertab->walk_mutex serializes the two walks and protects route_ht_iter (it's acquired before peertab->lock; both callers run in process context).
  • homa_peer_free_net takes peertab->lock only around each removal and counter update.
  • homa_route_gc picks victims under the mutex and rcu_read_lock(), then takes peertab->lock for the removals. rhashtable_remove_fast's existing check skips a victim someone else removed in between, and RCU keeps it valid until then.
  • READ_ONCE on the lockless num_routes reads.

If you'd rather not add a lock: a fresh local iterator per GC pass would also work, but it loses the persistent cursor, so GC would always restart from the beginning of the table.

Checked:

  • 884/884 unit tests pass (including homa_peer_free_net__*, homa_route_gc__*, homa_route_pick_victims__*).
  • The 7.3 module builds normally and with __STRIP__=1.
  • On a lockdep + KASAN guest kernel: the smoke test (netns create/exit with traffic) and a two-VM run (traffic, netem loss and delay, Homa's qdisc and pacer, rmmod) went from a lockdep report to clean.

No test: the unit harness has no lockdep, and the lock order isn't otherwise observable.

🤖 Generated with Claude Code

The rhashtable walk functions acquire the table's internal lock with
spin_lock, and the rhashtable rehash worker holds that lock with
softirqs enabled. peertab->lock is acquired in softirq context
(homa_route_get, homa_route_free), so holding it while walking the
table can deadlock: lockdep reports "SOFTIRQ-safe -> SOFTIRQ-unsafe
lock order detected" from homa_peer_free_net at netns exit, and
homa_route_gc has the same problem.

Add peertab->walk_mutex to serialize the two walks (and protect
route_ht_iter), and acquire peertab->lock only around the removals.
homa_route_gc now holds the RCU read lock from choosing victims until
it has removed them, since another thread may remove a victim in
between.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@randomizedcoder

Copy link
Copy Markdown
Contributor Author

Heads-up: this conflicts with #144 at the end of homa_peer_free_net (both change it). Whichever lands second, the resolution is mutex_unlock(&peertab->walk_mutex); first, then the rcu_barrier() block from #144. That combination is what I tested (unit tests, plus the lockdep/KASAN and KCSAN VM runs).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant