Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756208AbcJ0Osf (ORCPT ); Thu, 27 Oct 2016 10:48:35 -0400 Received: from outbound-smtp06.blacknight.com ([81.17.249.39]:34853 "EHLO outbound-smtp06.blacknight.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751032AbcJ0Osd (ORCPT ); Thu, 27 Oct 2016 10:48:33 -0400 Date: Thu, 27 Oct 2016 10:59:50 +0100 From: Mel Gorman To: Peter Zijlstra Cc: Linus Torvalds , Andy Lutomirski , Andreas Gruenbacher , Andy Lutomirski , LKML , Bob Peterson , Steven Whitehouse , linux-mm Subject: Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Message-ID: <20161027095950.GH2699@techsingularity.net> References: <20161026203158.GD2699@techsingularity.net> <20161026220339.GE2699@techsingularity.net> <20161026230726.GF2699@techsingularity.net> <20161027080852.GC3568@worktop.programming.kicks-ass.net> <20161027090742.GG2699@techsingularity.net> <20161027094449.GL3102@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <20161027094449.GL3102@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2146 Lines: 59 On Thu, Oct 27, 2016 at 11:44:49AM +0200, Peter Zijlstra wrote: > On Thu, Oct 27, 2016 at 10:07:42AM +0100, Mel Gorman wrote: > > > Something like so could work I suppose, but then there's a slight > > > regression in the page_unlock() path, where we now do an unconditional > > > spinlock; iow. we loose the unlocked waitqueue_active() test. > > > > > > > I can't convince myself it's worthwhile. At least, I can't see a penalty > > of potentially moving one of the two bits to the high word. It's the > > same cache line and the same op when it matters. > > I'm having trouble connecting these here two paragraphs. Or were you > replying to something else? > > So the current unlock code does: > > wake_up_page() > if (waitqueue_active()) > __wake_up() /* takes waitqueue spinlocks here */ > > While the new one does: > > spin_lock(&q->lock); > if (waitqueue_active()) { > __wake_up_common() > } > spin_unlock(&q->lock); > > Which is an unconditional atomic op (which go for about ~20 cycles each, > when uncontended). > Ok, we were thinking about different things but I'm not sure I get your concern. With your patch, in the uncontended case we check the waiters bit and if there is no contention, we carry on. In the contended case, the lock is taken. Given that contention is likely to be due to IO being completed, I don't think the atomic op on top is going to make that much of a difference. About the only hazard I can think of is when unrelated pages hash to the same queue and so there is an extra op for the "fake contended" case. I don't think it's worth worrying about given that a false contention and atomic op might hurt some workload but the common case is avoiding a lookup. > > I don't see why it should be NUMA-specific even though with Linus' > > patch, NUMA is a concern. Even then, you still need a 64BIT check > > because 32BIT && NUMA is allowed on a number of architectures. > > Oh, I thought we killed 32bit NUMA and didn't check. I can make it > CONFIG_64BIT and be done with it. s/CONFIG_NUMA/CONFIG_64BIT/ on the > patch should do :-) > Sounds good. -- Mel Gorman SUSE Labs