From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933018AbaKMMrO (ORCPT ); Thu, 13 Nov 2014 07:47:14 -0500 Received: from cantor2.suse.de ([195.135.220.15]:34695 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932807AbaKMMrM (ORCPT ); Thu, 13 Nov 2014 07:47:12 -0500 Message-ID: <5464A84C.1040903@suse.cz> Date: Thu, 13 Nov 2014 13:47:08 +0100 From: Vlastimil Babka User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Joonsoo Kim CC: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Minchan Kim , Mel Gorman , Michal Nazarewicz , Naoya Horiguchi , Christoph Lameter , Rik van Riel , David Rientjes Subject: Re: [PATCH 5/5] mm, compaction: more focused lru and pcplists draining References: <1412696019-21761-1-git-send-email-vbabka@suse.cz> <1412696019-21761-6-git-send-email-vbabka@suse.cz> <20141027074112.GC23379@js1304-P5Q-DELUXE> <545738F1.4010307@suse.cz> <20141104003733.GB8412@js1304-P5Q-DELUXE> In-Reply-To: <20141104003733.GB8412@js1304-P5Q-DELUXE> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/04/2014 01:37 AM, Joonsoo Kim wrote: > On Mon, Nov 03, 2014 at 09:12:33AM +0100, Vlastimil Babka wrote: >> On 10/27/2014 08:41 AM, Joonsoo Kim wrote: >>> On Tue, Oct 07, 2014 at 05:33:39PM +0200, Vlastimil Babka wrote: >>> >>> And, I wonder why last_migrated_pfn is set after isolate_migratepages(). >> >> Not sure I understand your question. With the mistake above, it >> cannot currently be set at the point isolate_migratepages() is >> called, so you might question the goto check_drain in the >> ISOLATE_NONE case, if that's what you are wondering about. >> >> When I correct that, it might be set when COMPACT_CLUSTER_MAX pages >> are isolated and migrated the middle of a pageblock, and then the >> rest of the pageblock contains no pages that could be isolated, so >> the last isolate_migratepages() attempt in the pageblock returns >> with ISOLATE_NONE. Still there were some migrations that produced >> free pages that should be drained at that point. > > To clarify my question, I attach psuedo code that I thought correct. Sorry for the late reply. > static int compact_zone() > { > unsigned long last_migrated_pfn = 0; > > ... > > compaction_suitable(); > > ... > > while (compact_finished()) { > if (!last_migrated_pfn) > last_migrated_pfn = cc->migrate_pfn - 1; > > isolate_migratepages(); > switch case > migrate_pages(); > ... > > check_drain: (at the end of loop) > do flush and reset last_migrated_pfn if needed > } > } > > We should record last_migrated_pfn before isolate_migratepages() and > then compare it with cc->migrate_pfn after isolate_migratepages() to > know if we moved away from the previous cc->order aligned block. > Am I missing something? What about this scenario, with pageblock order: - record cc->migrate_pfn pointing to pageblock X - isolate_migratepages() skips the pageblock due to e.g. skip bit, or the pageblock being a THP already... - loop to pageblock X+1, last_migrated_pfn is still set to pfn of pageblock X (more precisely the pfn is (X << pageblock_order) - 1 per your code, but doesn't matter) - isolate_migratepages isolates something, but ends up somewhere in the middle of pageblock due to COMPACT_CLUSTER_MAX - cc->migrate_pfn points to pageblock X+1 (plus some pages it scanned) - so it will decide that it has fully migrated pageblock X and it's time to drain. But the drain is most likely useless - we didn't migrate anything in pageblock X, we skipped it. And in X+1 we didn't migrate everything yet, so we should drain only after finishing the other part of the pageblock. In short, "last_migrated_pfn" is not "last position of migrate scanner" but "last block where we *actually* migrated". I think if you would try to fix the scenario above, you would end up with something like my patch :) Vlastimil > Thanks. >