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
