|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] rwlock: add per-cpu reader-writer locks
On 05/11/15 13:48, Marcos E. Matsunaga wrote:
> Hi Malcolm,
>
> I tried your patches against staging yesterday and as soon as I started a
> guest, it panic. I have
> lock_profile enabled and applied your patches against:
I tested with a non debug version of Xen (because I was analysing the
performance of Xen) and thus
those ASSERTS were never run.
The ASSERTS can be safely removed, the rwlock behaviour is slightly different
in that it's possible
for a writer to hold the write lock whilst a reader is progressing through the
read critical
section, this is safe because the writer is waiting for the percpu variables to
clear before
actually progressing through it's own critical section.
I have an updated version of the patch series which fixes this. Do you want me
to post it or are you
happy to remove the ASSERTS yourself ( or switch to non-debug build of Xen)
Sorry for not catching this before it hit the list.
Malcolm
>
> 6f04de658574833688c3f9eab310e7834d56a9c0 x86: cleanup of early cpuid handling
>
>
>
> (XEN) HVM1 save: CPU
> (XEN) HVM1 save: PIC
> (XEN) HVM1 save: IOAPIC
> (XEN) HVM1 save: LAPIC
> (XEN) HVM1 save: LAPIC_REGS
> (XEN) HVM1 save: PCI_IRQ
> (XEN) HVM1 save: ISA_IRQ
> (XEN) HVM1 save: PCI_LINK
> (XEN) HVM1 save: PIT
> (XEN) HVM1 save: RTC
> (XEN) HVM1 save: HPET
> (XEN) HVM1 save: PMTIMER
> (XEN) HVM1 save: MTRR
> (XEN) HVM1 save: VIRIDIAN_DOMAIN
> (XEN) HVM1 save: CPU_XSAVE
> (XEN) HVM1 save: VIRIDIAN_VCPU
> (XEN) HVM1 save: VMCE_VCPU
> (XEN) HVM1 save: TSC_ADJUST
> (XEN) HVM1 restore: CPU 0
> [ 394.163143] loop: module loaded
> (XEN) Assertion 'rw_is_locked(&t->lock)' failed at grant_table.c:215
> (XEN) ----[ Xen-4.7-unstable x86_64 debug=y Tainted: C ]----
> (XEN) CPU: 0
> (XEN) RIP: e008:[<ffff82d08011019e>] do_grant_table_op+0x63f/0x2e04
> (XEN) RFLAGS: 0000000000010246 CONTEXT: hypervisor (d0v0)
> (XEN) rax: 0000000000000000 rbx: ffff83400f9dc9e0 rcx: 0000000000000000
> (XEN) rdx: 0000000000000001 rsi: ffff82d080342b10 rdi: ffff83400819b784
> (XEN) rbp: ffff8300774ffef8 rsp: ffff8300774ffdf8 r8: 0000000000000002
> (XEN) r9: 0000000000000002 r10: 0000000000000002 r11: 00000000ffffffff
> (XEN) r12: 0000000000000000 r13: 0000000000000000 r14: ffff83400819b780
> (XEN) r15: ffff83400f9d0000 cr0: 0000000080050033 cr4: 00000000001526e0
> (XEN) cr3: 000001007f613000 cr2: ffff8800746182b8
> (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: e010 cs: e008
> (XEN) Xen stack trace from rsp=ffff8300774ffdf8:
> (XEN) ffff8300774ffe08 ffff82d000000000 ffff8300774ffef8 ffff82d08017fc9b
> (XEN) ffff82d080342b28 ffff83400f9d8600 ffff82d080342b10 0000000000000000
> (XEN) ffff83400f9dca20 ffff832100000000 ffff834008188000 0000000000000001
> (XEN) 00000001772ee000 ffff8801e98d03e0 ffff8300774ffe88 0000000000000000
> (XEN) 0000000000000000 ffff8300774fff18 00000021d0269c10 000000000001001a
> (XEN) ffffffff00000001 0000000000000000 0000000000000246 00007ff7de45a407
> (XEN) 0000000000000100 00007ff7de45a407 0000000000000033 ffff8300772ee000
> (XEN) ffff8801eb0e3c00 ffff880004bf57e8 ffff8801e98d03e0 ffff8801eb0a5938
> (XEN) 00007cff88b000c7 ffff82d08023d952 ffffffff8100128a 0000000000000014
> (XEN) 0000000000000000 0000000000000001 ffff8801f6e18388 ffffffff81d3d740
> (XEN) ffff8801efb7bd40 ffff88000542e780 0000000000000282 0000000000000000
> (XEN) ffff8801e98d03a0 ffff8801efe07000 0000000000000014 ffffffff8100128a
> (XEN) 0000000000000001 ffff8801e98d03e0 0000000000000000 0001010000000000
> (XEN) ffffffff8100128a 000000000000e033 0000000000000282 ffff8801efb7bce0
> (XEN) 000000000000e02b 0000000000000000 0000000000000000 0000000000000000
> (XEN) 0000000000000000 0000000000000000 ffff8300772ee000 0000000000000000
> (XEN) 0000000000000000
> (XEN) Xen call trace:
> (XEN) [<ffff82d08011019e>] do_grant_table_op+0x63f/0x2e04
> (XEN) [<ffff82d08023d952>] lstar_enter+0xe2/0x13c
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 'rw_is_locked(&t->lock)' failed at grant_table.c:215
> (XEN) ****************************************
> (XEN)
> (XEN) Manual reset required ('noreboot' specified)
>
>
> Thanks for your help.
>
> On 11/03/2015 12:58 PM, Malcolm Crossley wrote:
>> Per-cpu read-write locks allow for the fast path read case to have low
>> overhead
>> by only setting/clearing a per-cpu variable for using the read lock.
>> The per-cpu read fast path also avoids locked compare swap operations which
>> can
>> be particularly slow on coherent multi-socket systems, particularly if there
>> is
>> heavy usage of the read lock itself.
>>
>> The per-cpu reader-writer lock uses a global variable to control the read
>> lock
>> fast path. This allows a writer to disable the fast path and ensure the
>> readers
>> use the underlying read-write lock implementation.
>>
>> Once the writer has taken the write lock and disabled the fast path, it must
>> poll the per-cpu variable for all CPU's which have entered the critical
>> section
>> for the specific read-write lock the writer is attempting to take. This
>> design
>> allows for a single per-cpu variable to be used for read/write locks
>> belonging
>> to seperate data structures as long as multiple per-cpu read locks are not
>> simultaneously held by one particular cpu. This also means per-cpu
>> reader-writer locks are not recursion safe.
>>
>> Slow path readers which are unblocked set the per-cpu variable and drop the
>> read lock. This simplifies the implementation and allows for fairness in the
>> underlying read-write lock to be taken advantage of.
>>
>> There may be slightly more overhead on the per-cpu write lock path due to
>> checking each CPUs fast path read variable but this overhead is likely be
>> hidden
>> by the required delay of waiting for readers to exit the critical section.
>> The loop is optimised to only iterate over the per-cpu data of active readers
>> of the rwlock.
>>
>> Signed-off-by: Malcolm Crossley <malcolm.crossley@xxxxxxxxxx>
>> ---
>> xen/common/spinlock.c | 32 ++++++++++++++++++++++++++++++++
>> xen/include/asm-arm/percpu.h | 5 +++++
>> xen/include/asm-x86/percpu.h | 6 ++++++
>> xen/include/xen/percpu.h | 4 ++++
>> xen/include/xen/spinlock.h | 37 +++++++++++++++++++++++++++++++++++++
>> 5 files changed, 84 insertions(+)
>>
>> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
>> index 7f89694..a526216 100644
>> --- a/xen/common/spinlock.c
>> +++ b/xen/common/spinlock.c
>> @@ -492,6 +492,38 @@ int _rw_is_write_locked(rwlock_t *lock)
>> return (lock->lock == RW_WRITE_FLAG); /* writer in critical section? */
>> }
>> +void percpu_write_lock(rwlock_t **per_cpudata, bool_t *writer_activating,
>> + rwlock_t *rwlock)
>> +{
>> + int cpu;
>> + cpumask_t active_readers;
>> +
>> + cpumask_copy(&active_readers, &cpu_online_map);
>> + /* First take the write lock to protect against other writers */
>> + write_lock(rwlock);
>> +
>> + /* Now set the global variable so that readers start using read_lock */
>> + *writer_activating = true;
>> + smp_mb();
>> +
>> + /* Check if there are any percpu readers in progress on our rwlock*/
>> + do
>> + {
>> + for_each_cpu(cpu, &active_readers)
>> + {
>> + /* Remove any percpu readers not contending
>> + * from our check mask */
>> + if ( per_cpu_ptr(per_cpudata, cpu) != rwlock )
>> + cpumask_clear_cpu(cpu, &active_readers);
>> + }
>> + /* Check if we've cleared all percpu readers */
>> + if ( cpumask_empty(&active_readers) )
>> + break;
>> + /* Give the coherency fabric a break */
>> + cpu_relax();
>> + } while ( 1 );
>> +}
>> +
>> #ifdef LOCK_PROFILE
>> struct lock_profile_anc {
>> diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
>> index 71e7649..c308a56 100644
>> --- a/xen/include/asm-arm/percpu.h
>> +++ b/xen/include/asm-arm/percpu.h
>> @@ -27,6 +27,11 @@ void percpu_init_areas(void);
>> #define __get_cpu_var(var) \
>> (*RELOC_HIDE(&per_cpu__##var, READ_SYSREG(TPIDR_EL2)))
>> +#define per_cpu_ptr(var, cpu) \
>> + (*RELOC_HIDE(&var, __per_cpu_offset[cpu]))
>> +#define __get_cpu_ptr(var) \
>> + (*RELOC_HIDE(&var, READ_SYSREG(TPIDR_EL2)))
>> +
>> #define DECLARE_PER_CPU(type, name) extern __typeof__(type) per_cpu__##name
>> DECLARE_PER_CPU(unsigned int, cpu_id);
>> diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
>> index 604ff0d..51562b9 100644
>> --- a/xen/include/asm-x86/percpu.h
>> +++ b/xen/include/asm-x86/percpu.h
>> @@ -20,4 +20,10 @@ void percpu_init_areas(void);
>> #define DECLARE_PER_CPU(type, name) extern __typeof__(type)
>> per_cpu__##name
>> +#define __get_cpu_ptr(var) \
>> + (*RELOC_HIDE(var, get_cpu_info()->per_cpu_offset))
>> +
>> +#define per_cpu_ptr(var, cpu) \
>> + (*RELOC_HIDE(var, __per_cpu_offset[cpu]))
>> +
>> #endif /* __X86_PERCPU_H__ */
>> diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h
>> index abe0b11..c896863 100644
>> --- a/xen/include/xen/percpu.h
>> +++ b/xen/include/xen/percpu.h
>> @@ -16,6 +16,10 @@
>> /* Preferred on Xen. Also see arch-defined per_cpu(). */
>> #define this_cpu(var) __get_cpu_var(var)
>> +#define this_cpu_ptr(ptr) __get_cpu_ptr(ptr)
>> +
>> +#define get_per_cpu_var(var) (per_cpu__##var)
>> +
>> /* Linux compatibility. */
>> #define get_cpu_var(var) this_cpu(var)
>> #define put_cpu_var(var)
>> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
>> index fb0438e..f929f1b 100644
>> --- a/xen/include/xen/spinlock.h
>> +++ b/xen/include/xen/spinlock.h
>> @@ -3,6 +3,7 @@
>> #include <asm/system.h>
>> #include <asm/spinlock.h>
>> +#include <xen/stdbool.h>
>> #ifndef NDEBUG
>> struct lock_debug {
>> @@ -261,4 +262,40 @@ int _rw_is_write_locked(rwlock_t *lock);
>> #define rw_is_locked(l) _rw_is_locked(l)
>> #define rw_is_write_locked(l) _rw_is_write_locked(l)
>> +static inline void percpu_read_lock(rwlock_t **per_cpudata, bool_t
>> *writer_activating,
>> + rwlock_t *rwlock)
>> +{
>> + /* Indicate this cpu is reading */
>> + this_cpu_ptr(per_cpudata) = rwlock;
>> + smp_mb();
>> + /* Check if a writer is waiting */
>> + if ( unlikely(*writer_activating) )
>> + {
>> + /* Let the waiting writer know we aren't holding the lock */
>> + this_cpu_ptr(per_cpudata) = NULL;
>> + /* Wait using the read lock to keep the lock fair */
>> + read_lock(rwlock);
>> + /* Set the per CPU data again and continue*/
>> + this_cpu_ptr(per_cpudata) = rwlock;
>> + /* Drop the read lock because we don't need it anymore */
>> + read_unlock(rwlock);
>> + }
>> +}
>> +
>> +static inline void percpu_read_unlock(rwlock_t **per_cpudata)
>> +{
>> + this_cpu_ptr(per_cpudata) = NULL;
>> + smp_wmb();
>> +}
>> +
>> +/* Don't inline percpu write lock as it's a complex function */
>> +void percpu_write_lock(rwlock_t **per_cpudata, bool_t *writer_activating,
>> + rwlock_t *rwlock);
>> +
>> +static inline void percpu_write_unlock(bool_t *writer_activating, rwlock_t
>> *rwlock)
>> +{
>> + *writer_activating = false;
>> + write_unlock(rwlock);
>> +}
>> +
>> #endif /* __SPINLOCK_H__ */
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |