Hi David, Thanks for the detailed explanations and for addressing the review comments in v2.
On Fri, Aug 21, 2026 at 7:53 PM David Woodhouse <[email protected]> wrote: > > On Fri, 2026-08-21 at 11:22 +0800, Kunwu Chan wrote: > > If the common case is that there are no readers and the new fast path > > is intended to avoid the SRCU grace-period machinery, could you explain > > what property of SRCU is still required by the proposed users that > > prevents using ordinary RCU instead? > > My original version did use RCU, but Sean asked me to switch to a > dedicated per-vm SRCU¹ because a system-wide grace period doesn't work > well on large KVM hosting systems. The rationale for the per-VM SRCU and the no-reader fast path is much clearer now. In particular, I understand that the per-VM SRCU is mainly about having a dedicated synchronization domain, while the try path avoids the unnecessary workqueue and scheduling latency in the no-reader case. > > ¹ https://lore.kernel.org/all/[email protected]/ > > > Could this use case instead be addressed by making exp_holdoff > > configurable per srcu_struct, or otherwise allowing a dedicated SRCU > > domain to use a more aggressive expedite policy? > > That doesn't help much; the other concern is latency. And the mere act > of bouncing to the workqueue only to realise we can wake up again was > adding 100µs or so every time for nothing. *Plus* once you schedule at > all, you start to see really high tail latencies of >20ms too — which > is why we've also started looking at synchronize_srcu_atomic() to > eliminate that completely. If we get *that*, then maybe I don't > personally care as much about try_synchronize_srcu(), although it might > be a useful extension to synchronize_srcu_expedited() — this *would* be > the 'more aggressive expedite policy' of which you speak, for the case > where there are no readers. > In particular, tuning exp_holdoff would not address the workqueue round-trip and the resulting tail latency. > > Could you also clarify why observing no readers during this > > check is sufficient for the reclamation/happens-before guarantee > > required by the proposed callers? > > Both halves of the grace-period guarantee are provided when it returns > true. For any reader, either: > > • its lock increment was observed by the sums and its unlock was > too — the reader's final smp_mb() then orders its whole critical > section before the caller's subsequent accesses (that reader has > completed), or > > • its lock increment was not observed — in which case the > store-buffering pairing with the smp_mb() in __srcu_read_lock() > guarantees that reader observes every store the caller made > before calling: it is a "new" reader for the purposes of the > grace period, exactly as if it had started after a real > synchronize_srcu() returned, or > > • its lock increment was observed but its unlock was not — the one > case where the reader may still be running with no guarantee it > saw anything. But that reader inflates the lock sum above the > unlock sum, so this is precisely the case in which we return > false and the caller takes the real grace period. > > It's the same argument srcu_readers_active_idx_check() makes; the only > novelty is summing both epochs so no flip is needed. > > > Since a successful try_synchronize_srcu() does not advance srcu_gp_seq, > > flip the SRCU index, or process callbacks, could we document more > > explicitly that a successful try provides the required > > reclamation/happens-before guarantee to the caller, but does not > > constitute an SRCU grace-period completion for the state/callback APIs? > > Yes; done in v2. > > > What ordering edge does the first smp_mb() establish that is not > > already provided by the middle and final barriers? > > Er.... none? :) > > We need the caller's prior stores ordered before the *lock*-counter > reads, but the middle barrier already does that for us. Dropped in v2, > and the comment on the middle barrier updated. > > > Would it make sense to perform the same lockdep check in the try path? > > Ack. > > > When applying this to current rcu/dev, the patch also needs to account > > for the additional bool is_atomic argument to > > check_init_srcu_struct(). Please rebase accordingly. > > Ooh, shiny, I hadn't spotted that the atomic thing had landed. Maybe I > should retract my 'if we get that' above... but let's see how it > survives. > > I also have more than just this patch to rebase onto rcu/dev — unlike > some, I'm not just hacking RCU for the fun of it; I get my jollies > elsewhere :) > > Btw, the current commit c68271c3f83a message says "no index flip, no > grace-period sequence update" but that isn't true of the implementation > any more, is it? In fact, it *always* does those things, and doesn't > take the try-path optimisation by counting both epochs, that I'm doing > here? > The v2 changes address the points I raised, including removing the redundant leading smp_mb(), adding the lockdep check, and clarifying that a successful try does not constitute an SRCU grace-period completion for the state/callback APIs. I'll take another look at v2, in particular the two-epoch counter proof and its correspondence with srcu_readers_active_idx_check(). Thanks again for the detailed explanations. Thanks, Kunwu > Thanks for the careful review.
