Adding Paul McKenney...
On 29/05/2017 16:30, Linu Cherian wrote:
> In KVM irq injection path, irqfd_wakeup in interrupt context
> calls srcu_read_lock(&kvm->irq_srcu). A worker thread, irqfd_inject
> in process context also calls srcu_read_lock inside kvm_set_irq.
>
> This can lead to race condition while incrementing the
> lock_count(using __this_cpu_inc), since atomic instructions being not
> used.
>
> Replacing __this_cpu_inc with this_cpu_inc(atomic version) workarounds the
> problem which backs up this analysis.
Paul, how would you feel about either using this_cpu_inc in
__srcu_read_lock and __srcu_read_unlock? Many architectures already
provide efficient this_cpu_inc, and any architecture that has LL/SC
could provide it easily.
Thanks,
Paolo
> Possible solution
> -----------------
> One way is to avoid the srcu_read_lock/unlock usage in irq context.
> In arm/arm64 architecture,
> if (kvm_arch_set_irq_inatomic(&irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1,
> false) == -EWOULDBLOCK)
> in irqfd_wakefup is always true and hence schedule_work can be called
> directly
>
> @@ -195,6 +195,11 @@ int __attribute__((weak))
> kvm_arch_set_irq_inatomic(
> int idx;
>
> if (flags & POLLIN) {
> + if (kvm_arch_set_irq_will_block_always()) {
> + schedule_work(&irqfd->inject);
> + goto skiplock;
> + }
> +
> idx = srcu_read_lock(&kvm->irq_srcu);
> do {
> seq =
> read_seqcount_begin(&irqfd->irq_entry_sc);
> @@ -208,6 +213,7 @@ int __attribute__((weak))
> kvm_arch_set_irq_inatomic(
> srcu_read_unlock(&kvm->irq_srcu, idx);
> }
>
> +skiplock:
> if (flags & POLLHUP) {
> /* The eventfd is closing, detach from KVM */
> unsigned long flags;
>
> This works without giving any warnings as well.
>
> Is a patch welcome in that direction ?
>
> Appreicate your feedback on this.
>
>
>
