Re: [PATCH] mm/mmu_notifier: Remove non_block_start/end() from notifier invocation
David Woodhouse <[email protected]>
| Newsgroups | org.kernel.vger.kvm,dev.linux.lists.linux-rt-devel,org.kernel.vger.linux-kernel,org.kvack.linux-mm |
|---|---|
| Message-ID | <[email protected]> |
On Fri, 2026-08-21 at 10:38 -0700, Paul E. McKenney wrote:
> The current state is that if you invoke synchronize_srcu_atomic()
> from a given context, then srcu_read_lock_atomic() must be invoked
> from that same context, or a more strict one.
>
> So, for example, you can disable preemption across your invocation of
> synchronize_srcu_atomic(), but only if you also do so across the entire
> reader, starting before srcu_read_lock_atomic() and ending after the
> matching srcu_read_unlock_atomic(). Or you could instead disable
> interrupts across the entire SRCU reader.
>
> Does that work?
Yes, I think that should work; thanks. I don't think we *need* to
disable interrupts or even preemption for the KVM case, but I played
with it a bit to look at the tail latencies that Sean was concerned
about.
I converted the GPC series to use srcu_read_lock_atomic() and
synchronize_srcu_atomic()¹ and redid the same invalidation flood
testing: 8× shinfo + 4× vcpu_info reproducers on 128-way Ice Lake.
On its own it isn't much of a win for this use case AFAICT, but it does
work nicely when combined with the try_synchronize_srcu() thing that I
posted before².
I compared three variants: synchronize_srcu_atomic() as in your dev
branch; the same with try_synchronize_srcu() as an inline fast path
in front of the GP spin loop; and my previous try_synchronize_srcu()
+ synchronize_srcu_expedited() fallback for comparison. With
PREEMPT_DYNAMIC, and without all the KASAN and lockdep debugging I had
added for correctness checking:
avg max
atomic spin 8.0µs 6.0ms
try + atomic 3.6µs 326µs
try + expedited 3.7µs 4.4ms
The try fast path more than halves the average — it skips
srcu_gp_start() and the flip-driven advance loop entirely for the
no-readers case, which is >99% of them here. It also seems to fix
a problem where synchronize_srcu_atomic() calls spend their time
queued behind *each other* — atomic alone has no sub-4µs drains at
all, and 40% of its drains take 8-16µs, presumably contending for
the driver role and waiting out each other's flip cycles. With the
try in front, only the genuinely-contended calls enter the GP spin
loop at all.
I was initially dubious about that 326µs max — the slow path is still
just spinning like the atomic variant, just with two orders of
magnitude fewer entries into it, so perhaps it just didn't hit the same
p100 in my test runs. So I redid it with the try-failed slow path
instrumented separately, and it does seem to be real. Of ~5M drains,
3474 took the real GP slow path: avg 13.7µs, max 326µs, none over a
millisecond — and the *fast* path's max was 282µs, so the worst case is
something like interrupt noise, not the actual grace period. For
comparison, the expedited fallback was consistently producing drains
over 1ms (sleeping and waiting for the workqueue under a saturated
machine). There were about 100 such >1ms waits in each 300-second test
window (of about 60k fallbacks).
So the tail win over try+expedited is real, if we care about hundreds
of µs vs single-digit ms. Which I think we do — that's what brings the
p100 to parity with the existing rwlock code (~330µs on the same test)
while also scaling better on the readers and resolving the whole
PREEMPT_RT issue.
One thing I didn't expect: the try falls through to the slow path about
10× less often in front of the atomic variant (0.07% of drains vs 0.5-
0.7% falling back to synchronize_srcu_expedited()). I suspected the
preempt-enabled readers in the latter, but rerunning that test with the
readers non-preemptible made no difference at all. So it's an effect of
the wait side itself somehow. It looks like the sleeping variant lets
the abusive invalidate/refresh cycle churn about twice as fast (11M
drains per window vs 5M), so the try observes readers in flight more
often? I'm not sure that higher throughput of the abusive test is the
metric we're here to measure though.
Could we put try_synchronize_srcu() at the front of
synchronize_srcu_atomic()? Without it, the atomic version is slower
than what I had before, across the board.
However...
Testing with my KASAN+lockdep kernels showed the p100 latency going up
to 200ms for the atomic version, while with try+expedited it was only
about 25ms (the same order of magnitude as the existing rwlock-based
implementation in the debug setup):
avg max
atomic spin 10.2µs 172-212ms
try + atomic 10.2µs 173ms
try + expedited 14.7µs 25ms
Pinning the grace period driver with preempt_disable() didn't make a
difference. A sched_switch-instrumented run showed the >1ms tail is
bimodal: half the samples ~100% off-CPU (the *waiter* descheduled after
the wait was already over), and half genuinely on-CPU spinning and
waiting for readers.
So on a debug kernel under CPU saturation a spinning waiter has a much
worse worst case than a sleeping one; on a production-like kernel the
try+spin mode does seem to give a big win in the tail latencies that
Sean was concerned about. I think that's OK.
¹ https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/gpc-atomic-dev
² https://lore.kernel.org/all/[email protected]/
smime.p7s
(application/pkcs7-signature, 6 KB) - not displayed