From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753102AbcAVPUz (ORCPT ); Fri, 22 Jan 2016 10:20:55 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33071 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750944AbcAVPUx (ORCPT ); Fri, 22 Jan 2016 10:20:53 -0500 Date: Fri, 22 Jan 2016 17:20:50 +0200 From: "Kirill A. Shutemov" To: Andrea Arcangeli Cc: "Kirill A. Shutemov" , Andrew Morton , Hugh Dickins , Dave Hansen , Mel Gorman , Rik van Riel , Vlastimil Babka , Christoph Lameter , Naoya Horiguchi , Steve Capper , "Aneesh Kumar K.V" , Johannes Weiner , Michal Hocko , Jerome Marchand , Sasha Levin , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH 2/3] thp: change deferred_split_count() to return number of THP in queue Message-ID: <20160122152049.GA24420@node.shutemov.name> References: <20160121012237.GE7119@redhat.com> <1453378163-133609-1-git-send-email-kirill.shutemov@linux.intel.com> <1453378163-133609-3-git-send-email-kirill.shutemov@linux.intel.com> <20160122143127.GI7119@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160122143127.GI7119@redhat.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 22, 2016 at 03:31:27PM +0100, Andrea Arcangeli wrote: > On Thu, Jan 21, 2016 at 03:09:22PM +0300, Kirill A. Shutemov wrote: > > @@ -3511,7 +3506,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink, > > list_splice_tail(&list, &pgdata->split_queue); > > spin_unlock_irqrestore(&pgdata->split_queue_lock, flags); > > > > - return split * HPAGE_PMD_NR / 2; > > + return split; > > } > > Looking further at how the caller processes this "split" retval, if > the list has been fully shrunk by the page freeing, between the > split_count and split_scan, the caller seems to ignore a 0 value > returned above and it'll keep calling even if sc->nr_to_scan isn't > decreasing. The caller won't even check sc->nr_to_scan to notice that > it isn't decreasing anymore, it's write-only as far as the caller is > concerned. > > It's also weird we can't return the number of freed pages and break > the loop with just one invocation of the split_scan, but that's a > slight inefficiency in the caller interface. The caller also seems to > forget to set total_scan to 0 if SHRINK_STOP was returned but perhaps > that's on purpose, however for our purpose it'd be better off if it > did. > > The split_queue.next is going to be hot in the CPU cache anyway, so > unless we change the caller, it should be worth it to add a list_empty > check and return SHRINK_STOP if it was empty. Doing it at the start or > end doesn't make much difference, at the end lockless it'll deal with > the split failures too if any. > > return split ? : list_empty(&pgdat->split_queue) ? SPLIT_STOP : 0; Ughh. Shrinker interface is confusing. >>From ed85b527b2ea5c0b8ed93d9d212f9f0d25cae3ab Mon Sep 17 00:00:00 2001 From: "Kirill A. Shutemov" Date: Fri, 22 Jan 2016 18:10:59 +0300 Subject: [PATCH] thp: deferred_split_scan(): stop shrinker if the queue is empty If pages on queue were freed under us, deferred_split_scan() would return zero. It makes caller keep calling deferred_split_scan() without any result. Let's return SHRINK_STOP in this situation. Signed-off-by: Kirill A. Shutemov Suggested-by: Andrea Arcangeli --- mm/huge_memory.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 298dbc001b07..2ea5e26ce069 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -3508,6 +3508,12 @@ static unsigned long deferred_split_scan(struct shrinker *shrink, list_splice_tail(&list, &pgdata->split_queue); spin_unlock_irqrestore(&pgdata->split_queue_lock, flags); + /* + * Stop shrinker, if we didn't split any page, but the queue is empty. + * This can happen if pages were freed under us. + */ + if (!split && list_empty(&pgdata->split_queue)) + return SHRINK_STOP; return split; } -- Kirill A. Shutemov