From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755750AbcGEVhm (ORCPT ); Tue, 5 Jul 2016 17:37:42 -0400 Received: from mx2.suse.de ([195.135.220.15]:35882 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755560AbcGEVhi (ORCPT ); Tue, 5 Jul 2016 17:37:38 -0400 Subject: Re: [patch for-4.7] mm, compaction: prevent VM_BUG_ON when terminating freeing scanner To: David Rientjes References: <7ecb4f2d-724f-463f-961f-efba1bdb63d2@suse.cz> Cc: Joonsoo Kim , Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, hughd@google.com, mgorman@techsingularity.net, minchan@kernel.org, stable@vger.kernel.org From: Vlastimil Babka Message-ID: <577C289E.9020403@suse.cz> Date: Tue, 5 Jul 2016 23:37:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/05/2016 11:01 PM, David Rientjes wrote: > On Thu, 30 Jun 2016, Vlastimil Babka wrote: > >>> Note: I really dislike the low watermark check in split_free_page() and >>> consider it poor software engineering. The function should split a free >>> page, nothing more. Terminating memory compaction because of a low >>> watermark check when we're simply trying to migrate memory seems like an >>> arbitrary heuristic. There was an objection to removing it in the first >>> proposed patch, but I think we should really consider removing that >>> check so this is simpler. >> >> There's a patch changing it to min watermark (you were CC'd on the series). We >> could argue whether it belongs to split_free_page() or some wrapper of it, but >> I don't think removing it completely should be done. If zone is struggling >> with order-0 pages, a functionality for making higher-order pages shouldn't >> make it even worse. It's also not that arbitrary, even if we succeeded the >> migration and created a high-order page, the higher-order allocation would >> still fail due to watermark checks. Worse, __compact_finished() would keep >> telling the compaction to continue, creating an even longer lag, which is also >> against your recent patches. >> > > I'm suggesting we shouldn't check any zone watermark in split_free_page(): > that function should just split the free page. > > I don't find our current watermark checks to determine if compaction is > worthwhile to be invalid, but I do think that we should avoid checking or > acting on any watermark in isolate_freepages() itself. We could do more > effective checking in __compact_finished() to determine if we should > terminate compaction, but the freeing scanner feels like the wrong place > to do it -- it's also expensive to check while gathering free pages for > memory that we have already successfully isolated as part of the > iteration. > > Do you have any objection to this fix for 4.7? No. Acked-by: Vlastimil Babka > Joonson and/or Minchan, does this address the issue that you reported? >