Re: [PATCH v10 08/19] qspinlock: Make a new qnode structure to support virtualization

[Peter Zijlstra]

On Sat, May 10, 2014 at 04:14:17PM +0200, Peter Zijlstra wrote:
> On Fri, May 09, 2014 at 09:08:56PM -0400, Waiman Long wrote:
> > On 05/08/2014 03:04 PM, Peter Zijlstra wrote:
> > >On Wed, May 07, 2014 at 11:01:36AM -0400, Waiman Long wrote:
> > >>  /*
> > >>+ * To have additional features for better virtualization support, it is
> > >>+ * necessary to store additional data in the queue node structure. So
> > >>+ * a new queue node structure will have to be defined and used here.
> > >>+ */
> > >>+struct qnode {
> > >>+	struct mcs_spinlock mcs;
> > >>+};
> > >You can ditch this entire patch; its pointless, just add a new
> > >DEFINE_PER_CPU for the para-virt muck.
> > 
> > Yes, I can certainly merge it to the next one in the series. I break it out
> > to make each individual patch smaller, more single-purpose and easier to
> > review.
> 
> No, don't merge it, _drop_ it. Wrapping things in a struct generates a
> ton of pointless change.
> 
> Put the new data in a new DEFINE_PER_CPU and leave the existing code as
> is.

So I had a look at the resulting code:

struct qnode {
	struct mcs_spinlock mcs;
#ifdef CONFIG_PARAVIRT_UNFAIR_LOCKS
	int		lsteal_mask;	/* Lock stealing frequency mask	*/
	u32		prev_tail;	/* Tail code of previous node	*/
#ifndef CONFIG_PARAVIRT_SPINLOCKS
	struct qnode   *qprev;		/* Previous queue node addr	*/
#endif
#endif
	struct pv_qvars pv;		/* For para-virtualization	*/
};

With all the bells and whistles on (say an enterprise distro), that
single node will now fill an entire cacheline on its own.

That means that the normal case for normal people who stay the heck away
from virt shit will very often hit _3_ cachelines for their spin_lock().

1 - the cacheline that has the spinlock_t in,
2 - the cacheline that has node[0].count in to find which node to use
3 - the cacheline that has the actual right node in

That's of course complete and utter crap.

Not to mention that the final result of those 19 patches is going to
take me days to untangle :-(

Days I don't really have because I get to go hunt bugs in existing code
before thinking about adding shiny new stuff.

Attachment: pgpbc1T6c1XdF.pgp
Description: PGP signature