On Fri, Dec 23, 2022, Paolo Bonzini wrote: > On 12/20/22 08:47, Hao Peng wrote: > > > > + old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu, > > > > + lockdep_is_held(&kvm->irq_lock)); > > > Readers of irq_routing are protected via kvm->irq_srcu, but this writer is never > > > called with kvm->irq_srcu held. I do like the of replacing '1' with > > > lockdep_is_held(&kvm->irq_lock) to document the protection, so what about just > > > doing that? I.e. > > > > > Sorry for the long delay in replying. Although kvm->irq_srcu is not required > > to protect irq_routing here, this interface function srcu_dereference_check > > indicates that irq_routing is protected by kvm->irq_srcu in the kvm subsystem. > > Thanks. > > > > I agree, the last two arguments basically are alternative conditions to > satisfy the check: > > #define srcu_dereference_check(p, ssp, c) \ > __rcu_dereference_check((p), __UNIQUE_ID(rcu), \ > (c) || srcu_read_lock_held(ssp), __rcu) > > The idea is to share the code between readers and writers, But readers and writers naturally don't share code, and the subsequent synchronize_srcu_expedited() is what really documents the interaction between readers and writers. It's definitely not a sticking point though, and this one does seems to be the outlier in KVM. > so what do you think of adding a > > #define kvm_get_irq_routing(kvm) srcu_dereference_check(...) > > macro at the top of virt/kvm/irqchip.c? I'm fine with any approach, though a macro seems like overkill.
