Re: [PATCH] mm: thp: default defrag mode to defer+madvise

[Ferran Duarri <ferran.duarri@me.com>]

Please drop this one, and not for the reason you asked about. Your mail
sent me back to read the code properly, and my commit message has the
mechanism backwards.

I claimed:

> Paired with transparent_hugepage=always every anonymous fault
> becomes eligible, and under memory pressure the faulting thread can
> stall in compaction.

That doesn't happen. In vma_thp_gfp_mask(), the current default:

	/* Only do synchronous compaction if madvised */
	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, ...))
		return GFP_TRANSHUGE_LIGHT |
		       (vma_madvised ? __GFP_DIRECT_RECLAIM : 0);

A non-madvised fault gets GFP_TRANSHUGE_LIGHT with no reclaim flag at all,
so it fails fast and cannot stall in direct compaction. The stall I
described is only reachable from a MADV_HUGEPAGE region, and defer+madvise
keeps __GFP_DIRECT_RECLAIM for exactly those regions:

	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, ...))
		return GFP_TRANSHUGE_LIGHT |
			(vma_madvised ? __GFP_DIRECT_RECLAIM :
					__GFP_KSWAPD_RECLAIM);

So the patch removes no stall. What it actually changes is the other
branch: non-madvised faults gain __GFP_KSWAPD_RECLAIM, which they did not
have. That is strictly more background work, waking kswapd and kcompactd
on failed THP allocations across every anonymous fault under THP=always.
The patch does close to the opposite of what it claims, and on a
fragmented machine it is a plausible regression rather than an
improvement.

transhuge.rst says the same thing I should have read before writing the
commit message: madvise "will enter direct reclaim like always but only
for regions that are have used madvise(MADV_HUGEPAGE)".

Zi Yan, that also answers your question, and you were right to ask it: the
extra kswapd and kcompactd work you identified is the real effect of the
patch, not a side cost of it.

One correction to my own patch while I am here. I wrote that the machine
could not testify: thp_fault_fallback 0 across 60682 faults, compact_stall
0. That was true when I sent it and is not true now. Same box, THP=always,
64 GB, after a few hours with a 27B model resident:

	thp_fault_alloc     213690
	thp_fault_fallback   22145
	compact_stall         3762
	compact_fail          1926

So it does reach the fallback path, it just had not yet. I am not offering
that as evidence for anything: it was collected with defer+madvise already
in effect, so it says nothing about what madvise would have done, and
defrag is writable at runtime, so the A/B costs nothing. If I get
something worth showing, it will be a fresh patch with numbers in it, not
this one.

Lorenzo, no argument on the patch. It is wrong for the reason above and I
would rather have found that before sending than after.

On "distros can set as needed", that is the one part I would push back on,
and it is the same thing David asked. They cannot, other than by writing
to sysfs after boot. There is no Kconfig symbol for defrag; mm/Kconfig
offers only the ALWAYS/MADVISE/NEVER enablement axis. There is no boot
parameter either: setup_transparent_hugepage() sets
TRANSPARENT_HUGEPAGE_FLAG and TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG only, and
thp_anon= is a different axis again. Grepping for what sets the DEFRAG
bits at all outside the initialiser, it is defrag_store() and nothing
else. So a distro that wants a different defrag default ships a sysfs
unit, and anything faulting between subsys_initcall(hugepage_init) and
that unit gets the compiled-in value.

That may well be deliberate and sufficient. If it is not, a boot parameter
is the cheap fix and I am happy to write it. Either answer is useful to
me, and I would rather be told it is a non-problem than guess.

Some context, offered as an explanation and not as an excuse. These
patches come out of running large models locally: I maintain a custom
kernel tree for my own inference workstation, and the changes in it
accumulated there first, against a real workload rather than as ideas. I
have started sending them upstream to put that pile in order and to find
out which of them are actually correct rather than merely useful to me.
This one is a fair sample of why that is worth doing, and of why the order
I did it in was wrong: I posted while still testing, instead of testing
and then posting. I am slowing the pace down and the rest stays local
until it has had more than this one got.

Thanks for the review. It caught a real error.

Ferran