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

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Oct 27, 2014 at 01:15:53PM -0400, Waiman Long wrote:
> On 10/24/2014 06:04 PM, Peter Zijlstra wrote:
> >On Fri, Oct 24, 2014 at 04:53:27PM -0400, Waiman Long wrote:
> >>The additional register pressure may just cause a few more register moves
> >>which should be negligible in the overall performance . The additional
> >>icache pressure, however, may have some impact on performance. I was trying
> >>to balance the performance of the pv and non-pv versions so that we won't
> >>penalize the pv code too much for a bit more performance in the non-pv code.
> >>Doing it your way will add a lot of function call and register
> >>saving/restoring to the pv code.
> >If people care about performance they should not be using virt crap :-)
> >
> >I only really care about bare metal.
> 
> Yes, I am aware of that. However, the whole point of doing PV spinlock is to
> improve performance in a virtual guest.

Anything that avoids the lock holder preemption nonsense is a _massive_
win for them, a few function calls should not even register on that
scale.

> +#ifdef _GEN_PV_LOCK_SLOWPATH
> +static void pv_queue_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> +#else
>  void queue_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> +#endif

If you have two functions you might as well use the PV stuff to patch in
the right function call at the usage sites and avoid:

> +       if (pv_enabled()) {
> +               pv_queue_spin_lock_slowpath(lock, val);
> +               return;
> +       }

this alltogether.

>         this_cpu_dec(mcs_nodes[0].count);
>  }
>  EXPORT_SYMBOL(queue_spin_lock_slowpath);
> +
> +#if !defined(_GEN_PV_LOCK_SLOWPATH) && defined(CONFIG_PARAVIRT_SPINLOCKS)
> +/*
> + * Generate the PV version of the queue_spin_lock_slowpath function
> + */
> +#undef pv_init_node
> +#undef pv_wait_check
> +#undef pv_link_and_wait_node
> +#undef pv_wait_head
> +#undef EXPORT_SYMBOL
> +#undef in_pv_code
> +
> +#define _GEN_PV_LOCK_SLOWPATH
> +#define EXPORT_SYMBOL(x)
> +#define in_pv_code     return_true
> +#define pv_enabled     return_false
> +
> +#include "qspinlock.c"
> +
> +#endif

That's properly disgusting :-) But a lot better than actually
duplicating everything I suppose.