Messages in this thread | | | From | Paolo Bonzini <> | Subject | Re: [EXTERNAL] [PATCH 2/2] KVM: x86: disable interrupts while pvclock_gtod_sync_lock is taken | Date | Thu, 1 Apr 2021 18:36:26 +0200 |
| |
On 01/04/21 17:27, David Woodhouse wrote: >> - spin_lock(&ka->pvclock_gtod_sync_lock); >> + spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags); >> use_master_clock = ka->use_master_clock; >> if (use_master_clock) { >> host_tsc = ka->master_cycle_now; >> kernel_ns = ka->master_kernel_ns; >> } >> - spin_unlock(&ka->pvclock_gtod_sync_lock); >> + spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags); >> >> /* Keep irq disabled to prevent changes to the clock */ >> local_irq_save(flags); > That seems a little gratuitous at the end; restoring the flags as part > of the spin_unlock_irqrestore() and then immediately calling > local_irq_save(). > > Is something going to complain if we just use spin_unlock() there and > then later restore the flags with the existing local_irq_restore()?
Yes, I think it breaks on RT kernels.
> Or should we move the local_irq_save() up above the existing > spin_lock() and leave the spin lock/unlock as they are?
Nope, also breaks on RT (and this one is explicitly called out by Documentation/locking/locktypes.rst). Since it's necessary to use spin_lock_irqsave and spin_unlock_irqrestore, one would need flags and flags2 variables which is really horrible.
I thought of a similar one which is to move the critical section within the IRQ-disabled region:
local_irq_save(flags); ... spin_lock(&ka->pvclock_gtod_sync_lock); use_master_clock = ka->use_master_clock; if (use_master_clock) { host_tsc = ka->master_cycle_now; kernel_ns = ka->master_kernel_ns; } else { host_tsc = rdtsc(); kernel_ns = get_kvmclock_base_ns(); } spin_unlock(&ka->pvclock_gtod_sync_lock); ... local_irq_restore(flags);
... but didn't do it because of RT again.
Paolo
| |