[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v13 09/11] pvqspinlock, x86: Add para-virtualization support



On Wed, Oct 29, 2014 at 04:19:09PM -0400, Waiman Long wrote:
>  arch/x86/include/asm/pvqspinlock.h    |  411 
> +++++++++++++++++++++++++++++++++

I do wonder why all this needs to live in x86..

>  
> +#ifdef CONFIG_QUEUE_SPINLOCK
> +
> +static __always_inline void pv_kick_cpu(int cpu)
> +{
> +     PVOP_VCALLEE1(pv_lock_ops.kick_cpu, cpu);
> +}
> +
> +static __always_inline void pv_lockwait(u8 *lockbyte)
> +{
> +     PVOP_VCALLEE1(pv_lock_ops.lockwait, lockbyte);
> +}
> +
> +static __always_inline void pv_lockstat(enum pv_lock_stats type)
> +{
> +     PVOP_VCALLEE1(pv_lock_ops.lockstat, type);
> +}

Why are any of these PV ops? they're only called from other pv_*()
functions. What's the point of pv ops you only call from pv code?

> +/*
> + *   Queue Spinlock Para-Virtualization (PV) Support
> + *
> + * The PV support code for queue spinlock is roughly the same as that
> + * of the ticket spinlock.

Relative comments are bad, esp. since we'll make the ticket code go away
if this works, at which point this is a reference into a black hole.

>                             Each CPU waiting for the lock will spin until it
> + * reaches a threshold. When that happens, it will put itself to a halt state
> + * so that the hypervisor can reuse the CPU cycles in some other guests as
> + * well as returning other hold-up CPUs faster.




> +/**
> + * queue_spin_lock - acquire a queue spinlock
> + * @lock: Pointer to queue spinlock structure
> + *
> + * N.B. INLINE_SPIN_LOCK should not be enabled when PARAVIRT_SPINLOCK is on.

One should write a compile time fail for that, not a comment.

> + */
> +static __always_inline void queue_spin_lock(struct qspinlock *lock)
> +{
> +     u32 val;
> +
> +     val = atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL);
> +     if (likely(val == 0))
> +             return;
> +     if (static_key_false(&paravirt_spinlocks_enabled))
> +             pv_queue_spin_lock_slowpath(lock, val);
> +     else
> +             queue_spin_lock_slowpath(lock, val);
> +}

No, this is just vile.. _that_ is what we have PV ops for. And at that
point its the same function it was before the PV stuff, so that whole
inline thing is then gone.

> +extern void queue_spin_unlock_slowpath(struct qspinlock *lock);
> +
>  /**
>   * queue_spin_unlock - release a queue spinlock
>   * @lock : Pointer to queue spinlock structure
>   *
>   * An effective smp_store_release() on the least-significant byte.
> + *
> + * Inlining of the unlock function is disabled when CONFIG_PARAVIRT_SPINLOCKS
> + * is defined. So _raw_spin_unlock() will be the only call site that will
> + * have to be patched.

again if you hard rely on the not inlining make a build fail not a
comment.

>   */
>  static inline void queue_spin_unlock(struct qspinlock *lock)
>  {
>       barrier();
> +     if (!static_key_false(&paravirt_spinlocks_enabled)) {
> +             native_spin_unlock(lock);
> +             return;
> +     }
>  
> +     /*
> +      * Need to atomically clear the lock byte to avoid racing with
> +      * queue head waiter trying to set _QLOCK_LOCKED_SLOWPATH.
> +      */
> +     if (unlikely(cmpxchg((u8 *)lock, _Q_LOCKED_VAL, 0) != _Q_LOCKED_VAL))
> +             queue_spin_unlock_slowpath(lock);
> +}

Idem, that static key stuff is wrong, use PV ops to switch between
unlock paths.

> @@ -354,7 +394,7 @@ queue:
>        * if there was a previous node; link it and wait until reaching the
>        * head of the waitqueue.
>        */
> -     if (old & _Q_TAIL_MASK) {
> +     if (!pv_link_and_wait_node(old, node) && (old & _Q_TAIL_MASK)) {
>               prev = decode_tail(old);
>               ACCESS_ONCE(prev->next) = node;
> @@ -369,9 +409,11 @@ queue:
>        *
>        * *,x,y -> *,0,0
>        */
> -     while ((val = smp_load_acquire(&lock->val.counter)) &
> -                     _Q_LOCKED_PENDING_MASK)
> +     val = pv_wait_head(lock, node);
> +     while (val & _Q_LOCKED_PENDING_MASK) {
>               cpu_relax();
> +             val = smp_load_acquire(&lock->val.counter);
> +     }
>  
>       /*
>        * claim the lock:

Please make the pv_*() calls return void and reduce to NOPs. This keeps
the logic invariant of the pv stuff.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.