Re: [patch 1/2] mm, compaction: kcompactd should not ignore pageblock skip

[Vlastimil Babka]

On 09/11/2017 03:07 AM, David Rientjes wrote:
> On Wed, 23 Aug 2017, Vlastimil Babka wrote:
> 
>> On 08/16/2017 01:39 AM, David Rientjes wrote:
>>> Kcompactd is needlessly ignoring pageblock skip information.  It is doing
>>> MIGRATE_SYNC_LIGHT compaction, which is no more powerful than
>>> MIGRATE_SYNC compaction.
>>>
>>> If compaction recently failed to isolate memory from a set of pageblocks,
>>> there is nothing to indicate that kcompactd will be able to do so, or
>>> that it is beneficial from attempting to isolate memory.
>>>
>>> Use the pageblock skip hint to avoid rescanning pageblocks needlessly
>>> until that information is reset.
>>>
>>> Signed-off-by: David Rientjes <rientjes@xxxxxxxxxx>
>>
>> It would be much better if patches like this were accompanied by some
>> numbers.
>>
> 
> The numbers were from https://marc.info/?l=linux-mm&m=150231232707999 
> where very large amounts (>90% of system RAM) were hugetlb pages.  We can 
> supplement this changelog with the following if it helps:
> 
> """
> Currently, kcompactd ignores pageblock skip information that can become 
> useful if it is known that memory should not be considered by both the 
> migration and freeing scanners.  Abundant hugetlb memory is a good example 
> of memory that is needlessly (and expensively) scanned since the hugepage 
> order normally matches the pageblock order.
> 
> For example, on a sysctl with very large amounts of memory reserved by the 
> hugetlb subsystem:
> 
> compact_migrate_scanned 2931254031294 
> compact_free_scanned    102707804816705 
> compact_isolated        1309145254 
> 
> Kcompactd ends up successfully isolating ~0.0012% of memory that is 
> scans (the above does not involve direct compaction).

Yeah it would be nice to have the numbers also after the patch and to
see also effect on the kcompactd success rate.

> A follow-up change will set the pageblock skip for this memory since it is 
> never useful for either scanner.
> """
> 
>> Also there's now a danger that in cases where there's no direct
>> compaction happening (just kcompactd), nothing will ever call
>> __reset_isolation_suitable().
>>
> 
> I'm not sure that is helpful in a context where no high-order memory can 
> call direct compaction that kcompactd needlessly scanning the same memory 
> over and over is beneficial.

The point is that if it becomes beneficial again, we won't know as there
will be still be skip bits.

>>> ---
>>>  mm/compaction.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -1927,9 +1927,8 @@ static void kcompactd_do_work(pg_data_t *pgdat)
>>>  		.total_free_scanned = 0,
>>>  		.classzone_idx = pgdat->kcompactd_classzone_idx,
>>>  		.mode = MIGRATE_SYNC_LIGHT,
>>> -		.ignore_skip_hint = true,
>>> +		.ignore_skip_hint = false,
>>>  		.gfp_mask = GFP_KERNEL,
>>> -
>>>  	};
>>>  	trace_mm_compaction_kcompactd_wake(pgdat->node_id, cc.order,
>>>  							cc.classzone_idx);
>>>
>>
>>