Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v8] mm: Proactive compaction
To:     Andrew Morton <akpm@linux-foundation.org>,
        Nitin Gupta <nigupta@nvidia.com>
Cc:     Vlastimil Babka <vbabka@suse.cz>,
        Khalid Aziz <khalid.aziz@oracle.com>,
        Oleksandr Natalenko <oleksandr@redhat.com>,
        Michal Hocko <mhocko@suse.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        Matthew Wilcox <willy@infradead.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        David Rientjes <rientjes@google.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-mm <linux-mm@kvack.org>,
        Linux API <linux-api@vger.kernel.org>
References: <20200616204527.19185-1-nigupta@nvidia.com>
 <20200617135312.4f395479454c55a8d021b023@linux-foundation.org>
From:   Nitin Gupta <ngupta@nitingupta.dev>
Message-ID: <10f8a7c1-dc1f-532b-7ef7-d37b04334fbe@nitingupta.dev>
Date:   Wed, 17 Jun 2020 15:02:59 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <20200617135312.4f395479454c55a8d021b023@linux-foundation.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 6/17/20 1:53 PM, Andrew Morton wrote:
> On Tue, 16 Jun 2020 13:45:27 -0700 Nitin Gupta <nigupta@nvidia.com> wrote:
> 
>> For some applications, we need to allocate almost all memory as
>> hugepages. However, on a running system, higher-order allocations can
>> fail if the memory is fragmented. Linux kernel currently does on-demand
>> compaction as we request more hugepages, but this style of compaction
>> incurs very high latency. Experiments with one-time full memory
>> compaction (followed by hugepage allocations) show that kernel is able
>> to restore a highly fragmented memory state to a fairly compacted memory
>> state within <1 sec for a 32G system. Such data suggests that a more
>> proactive compaction can help us allocate a large fraction of memory as
>> hugepages keeping allocation latencies low.
>>
>> ...
>>
> 
> All looks straightforward to me and easy to disable if it goes wrong.
> 
> All the hard-coded magic numbers are a worry, but such is life.
> 
> One teeny complaint:
> 
>>
>> ...
>>
>> @@ -2650,12 +2801,34 @@ static int kcompactd(void *p)
>>   		unsigned long pflags;
>>   
>>   		trace_mm_compaction_kcompactd_sleep(pgdat->node_id);
>> -		wait_event_freezable(pgdat->kcompactd_wait,
>> -				kcompactd_work_requested(pgdat));
>> +		if (wait_event_freezable_timeout(pgdat->kcompactd_wait,
>> +			kcompactd_work_requested(pgdat),
>> +			msecs_to_jiffies(HPAGE_FRAG_CHECK_INTERVAL_MSEC))) {
>> +
>> +			psi_memstall_enter(&pflags);
>> +			kcompactd_do_work(pgdat);
>> +			psi_memstall_leave(&pflags);
>> +			continue;
>> +		}
>>   
>> -		psi_memstall_enter(&pflags);
>> -		kcompactd_do_work(pgdat);
>> -		psi_memstall_leave(&pflags);
>> +		/* kcompactd wait timeout */
>> +		if (should_proactive_compact_node(pgdat)) {
>> +			unsigned int prev_score, score;
> 
> Everywhere else, scores have type `int'.  Here they are unsigned.  How come?
> 
> Would it be better to make these unsigned throughout?  I don't think a
> score can ever be negative?
> 

The score is always in [0, 100], so yes, it should be unsigned.
I will send another patch which fixes this.

Thanks,
Nitin