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

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