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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 10 May 2014 20:21:34 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Waiman Long <waiman.long@...com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, linux-arch@...r.kernel.org,
	x86@...nel.org, linux-kernel@...r.kernel.org,
	virtualization@...ts.linux-foundation.org,
	xen-devel@...ts.xenproject.org, kvm@...r.kernel.org,
	Paolo Bonzini <paolo.bonzini@...il.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Rik van Riel <riel@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Raghavendra K T <raghavendra.kt@...ux.vnet.ibm.com>,
	David Vrabel <david.vrabel@...rix.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Gleb Natapov <gleb@...hat.com>,
	Scott J Norton <scott.norton@...com>,
	Chegu Vinod <chegu_vinod@...com>
Subject: Re: [PATCH v10 08/19] qspinlock: Make a new qnode structure to
 support virtualization

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.

Content of type "application/pgp-signature" skipped