From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f69.google.com (mail-pg0-f69.google.com [74.125.83.69]) by kanga.kvack.org (Postfix) with ESMTP id 0E7726B000C for ; Fri, 4 May 2018 06:33:30 -0400 (EDT) Received: by mail-pg0-f69.google.com with SMTP id s8-v6so13598687pgf.0 for ; Fri, 04 May 2018 03:33:30 -0700 (PDT) Received: from mx2.suse.de (mx2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id m2-v6si12739566pgq.221.2018.05.04.03.33.26 for (version=TLS1 cipher=AES128-SHA bits=128/128); Fri, 04 May 2018 03:33:27 -0700 (PDT) Date: Fri, 4 May 2018 11:33:22 +0100 From: Mel Gorman Subject: Re: [PATCH] mm/page_alloc: use ac->high_zoneidx for classzone_idx Message-ID: <20180504103322.2nbadmnehwdxxaso@suse.de> References: <1525408246-14768-1-git-send-email-iamjoonsoo.kim@lge.com> <8b06973c-ef82-17d2-a83d-454368de75e6@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <8b06973c-ef82-17d2-a83d-454368de75e6@suse.cz> Sender: owner-linux-mm@kvack.org List-ID: To: Vlastimil Babka Cc: js1304@gmail.com, Andrew Morton , Michal Hocko , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Johannes Weiner , Minchan Kim , Ye Xiaolong , Joonsoo Kim On Fri, May 04, 2018 at 09:03:02AM +0200, Vlastimil Babka wrote: > > min watermark for NORMAL zone on node 0 > > allocation initiated on node 0: 750 + 4096 = 4846 > > allocation initiated on node 1: 750 + 0 = 750 > > > > This watermark difference could cause too many numa_miss allocation > > in some situation and then performance could be downgraded. > > > > Recently, there was a regression report about this problem on CMA patches > > since CMA memory are placed in ZONE_MOVABLE by those patches. I checked > > that problem is disappeared with this fix that uses high_zoneidx > > for classzone_idx. > > > > http://lkml.kernel.org/r/20180102063528.GG30397@yexl-desktop > > > > Using high_zoneidx for classzone_idx is more consistent way than previous > > approach because system's memory layout doesn't affect anything to it. > > So to summarize; > - ac->high_zoneidx is computed via the arcane gfp_zone(gfp_mask) and > represents the highest zone the allocation can use It's arcane but it was simply a fast-path calculation. A much older definition would be easier to understand but it was slower. > - classzone_idx was supposed to be the highest zone that the allocation > can use, that is actually available in the system. Somehow that became > the highest zone that is available on the preferred node (in the default > node-order zonelist), which causes the watermark inconsistencies you > mention. > I think it *always* was the index of the first preferred zone of a zonelist. The treatment of classzone has changed a lot over the years and I didn't do a historical check but the general intent was always "protect some pages in lower zones". This was particularly important for 32-bit and highmem albeit that is less of a concern today. When it transferred to NUMA, I don't think it ever was seriously considered if it should change as the critical node was likely to be node 0 with all the zones and the remote nodes all used the highest zone. CMA/MOVABLE changed that slightly by allowing the possibility of node0 having a "higher" zone than every other node. When MOVABLE was introduced, it wasn't much of a problem as the purpose of MOVABLE was for systems that dynamically needed to allocate hugetlbfs later in the runtime but for CMA, it was a lot more critical for ordinary usage so this is primarily a CMA thing. > I don't see a problem with your change. I would be worried about > inflated reserves when e.g. ZONE_MOVABLE doesn't exist, but that doesn't > seem to be the case. My laptop has empty ZONE_MOVABLE and the > ZONE_NORMAL protection for movable is 0. > > But there had to be some reason for classzone_idx to be like this and > not simple high_zoneidx. Maybe Mel remembers? Maybe it was important > then, but is not anymore? Sigh, it seems to be pre-git. > classzone predates my involvement with Linux but I would be less concerneed about what the original intent was and instead ensure that classzone index is consistent, sane and potentially renamed while preserving the intent of "reserve pages in lower zones when an allocation request can use higher zones". While historically the critical intent was to preserve Normal and to a lesser extent DMA on 32-bit systems, there still should be some care of DMA32 so we should not lose that. With the patch, the allocator looks like it would be fine as just reservations change. I think it's unlikely that CMA usage will result in lowmem starvation. Compaction becomes a bit weird as classzone index has no special meaning versis highmem and I think it'll be very easy to forget. Similarly, vmscan can reclaim pages from remote nodes and zones that are higher than the original request. That is not likely to be a problem but it's a change in behaviour and easy to miss. Fundamentally, I find it extremely weird we now have two variables that are essentially the same thing. They should be collapsed into one variable, renamed and documented on what the index means for page allocator, compaction, vmscan and the special casing around CMA. -- Mel Gorman SUSE Labs