From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752675AbaEEKu6 (ORCPT ); Mon, 5 May 2014 06:50:58 -0400 Received: from cantor2.suse.de ([195.135.220.15]:38684 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751085AbaEEKu4 (ORCPT ); Mon, 5 May 2014 06:50:56 -0400 Date: Mon, 5 May 2014 12:50:54 +0200 From: Jan Kara To: Mel Gorman Cc: Linux-MM , Linux-FSDevel , Johannes Weiner , Vlastimil Babka , Jan Kara , Michal Hocko , Hugh Dickins , Linux Kernel Subject: Re: [PATCH 17/17] mm: filemap: Avoid unnecessary barries and waitqueue lookup in unlock_page fastpath Message-ID: <20140505105054.GC23927@quack.suse.cz> References: <1398933888-4940-1-git-send-email-mgorman@suse.de> <1398933888-4940-18-git-send-email-mgorman@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1398933888-4940-18-git-send-email-mgorman@suse.de> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 01-05-14 09:44:48, Mel Gorman wrote: > From: Nick Piggin > > This patch introduces a new page flag for 64-bit capable machines, > PG_waiters, to signal there are processes waiting on PG_lock and uses it to > avoid memory barriers and waitqueue hash lookup in the unlock_page fastpath. > > This adds a few branches to the fast path but avoids bouncing a dirty > cache line between CPUs. 32-bit machines always take the slow path but the > primary motivation for this patch is large machines so I do not think that > is a concern. ... > /* > diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c > index 7d50f79..fb83fe0 100644 > --- a/kernel/sched/wait.c > +++ b/kernel/sched/wait.c > @@ -304,8 +304,7 @@ int wake_bit_function(wait_queue_t *wait, unsigned mode, int sync, void *arg) > = container_of(wait, struct wait_bit_queue, wait); > > if (wait_bit->key.flags != key->flags || > - wait_bit->key.bit_nr != key->bit_nr || > - test_bit(key->bit_nr, key->flags)) > + wait_bit->key.bit_nr != key->bit_nr) > return 0; > else > return autoremove_wake_function(wait, mode, sync, key); This change seems to be really unrelated? And it would deserve a comment on its own I'd think so maybe split that in a separate patch? > diff --git a/mm/filemap.c b/mm/filemap.c > index c60ed0f..93e4385 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > +int __wait_on_page_locked_killable(struct page *page) > +{ > + int ret = 0; > + wait_queue_head_t *wq = page_waitqueue(page); > + DEFINE_WAIT_BIT(wait, &page->flags, PG_locked); > + > + if (!test_bit(PG_locked, &page->flags)) > + return 0; > + do { > + prepare_to_wait(wq, &wait.wait, TASK_KILLABLE); > + if (!PageWaiters(page)) > + SetPageWaiters(page); > + if (likely(PageLocked(page))) > + ret = sleep_on_page_killable(page); > + finish_wait(wq, &wait.wait); > + } while (PageLocked(page) && !ret); So I'm somewhat wondering why this is the only page waiting variant that does finish_wait() inside the loop. Everyone else does it outside the while loop which seems sufficient to me even in this case... Honza -- Jan Kara SUSE Labs, CR