| Message ID | CAPm50aJTh7optC=gBXfj+1HKVu+9U0165mYH0sjj3Jqgf8Aivg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: use unified srcu interface function | expand |
On Thu, Dec 08, 2022, Hao Peng wrote: > From: Peng Hao <flyingpeng@tencent.com> > > kvm->irq_routing is protected by kvm->irq_srcu. > > Signed-off-by: Peng Hao <flyingpeng@tencent.com> > --- > virt/kvm/irqchip.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c > index 1e567d1f6d3d..90f54f04e37c 100644 > --- a/virt/kvm/irqchip.c > +++ b/virt/kvm/irqchip.c > @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm, > } > > mutex_lock(&kvm->irq_lock); > - old = rcu_dereference_protected(kvm->irq_routing, 1); > + 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. diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c index 1e567d1f6d3d..77a18b4dc103 100644 --- a/virt/kvm/irqchip.c +++ b/virt/kvm/irqchip.c @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm, } mutex_lock(&kvm->irq_lock); - old = rcu_dereference_protected(kvm->irq_routing, 1); + old = rcu_dereference_protected(kvm->irq_routing, + lockdep_is_held(&kvm->irq_lock)); rcu_assign_pointer(kvm->irq_routing, new); kvm_irq_routing_update(kvm); kvm_arch_irq_routing_update(kvm); > rcu_assign_pointer(kvm->irq_routing, new); > kvm_irq_routing_update(kvm); > kvm_arch_irq_routing_update(kvm); > -- > 2.27.0
On Fri, Dec 9, 2022 at 9:22 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Dec 08, 2022, Hao Peng wrote: > > From: Peng Hao <flyingpeng@tencent.com> > > > > kvm->irq_routing is protected by kvm->irq_srcu. > > > > Signed-off-by: Peng Hao <flyingpeng@tencent.com> > > --- > > virt/kvm/irqchip.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c > > index 1e567d1f6d3d..90f54f04e37c 100644 > > --- a/virt/kvm/irqchip.c > > +++ b/virt/kvm/irqchip.c > > @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm, > > } > > > > mutex_lock(&kvm->irq_lock); > > - old = rcu_dereference_protected(kvm->irq_routing, 1); > > + 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. > diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c > index 1e567d1f6d3d..77a18b4dc103 100644 > --- a/virt/kvm/irqchip.c > +++ b/virt/kvm/irqchip.c > @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm, > } > > mutex_lock(&kvm->irq_lock); > - old = rcu_dereference_protected(kvm->irq_routing, 1); > + old = rcu_dereference_protected(kvm->irq_routing, > + lockdep_is_held(&kvm->irq_lock)); > rcu_assign_pointer(kvm->irq_routing, new); > kvm_irq_routing_update(kvm); > kvm_arch_irq_routing_update(kvm); > > > > rcu_assign_pointer(kvm->irq_routing, new); > > kvm_irq_routing_update(kvm); > > kvm_arch_irq_routing_update(kvm); > > -- > > 2.27.0
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, 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? Thanks, Paolo
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.
diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c index 1e567d1f6d3d..90f54f04e37c 100644 --- a/virt/kvm/irqchip.c +++ b/virt/kvm/irqchip.c @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm, } mutex_lock(&kvm->irq_lock); - old = rcu_dereference_protected(kvm->irq_routing, 1); + old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu, + lockdep_is_held(&kvm->irq_lock)); rcu_assign_pointer(kvm->irq_routing, new); kvm_irq_routing_update(kvm); kvm_arch_irq_routing_update(kvm);