Re: [PATCH v8 3/8] mm: Hot page tracking and promotion - pghot

[Bharata B Rao]

[Reply to Shashiko review]

On 28-Jul-26 11:13 AM, Bharata B Rao wrote:
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 9e0ca4824905..0a5bcd5d45ed 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -1509,6 +1509,20 @@ config LAZY_MMU_MODE_KUNIT_TEST
>  
>  	  If unsure, say N.
>  
> +config PGHOT
> +	bool "Hot page tracking and promotion"
> +	default n
> +	depends on NUMA_MIGRATION && SPARSEMEM

> Will this fail to compile if CONFIG_PGHOT is enabled but CONFIG_NUMA_BALANCING
> is disabled? The kmigrated_promotion_rate_limit() function unconditionally
> accesses fields like pgdat->nbp_rl_start which are gated by
> CONFIG_NUMA_BALANCING.

kmigrated_promotion_rate_limit() and the pgdat->nbp_* fields aren't part
of this patch (3/8) -- they're introduced in 5/8. That same patch moves the
nbp_* fields (and the PGPROMOTE_* node stats) from #ifdef
CONFIG_NUMA_BALANCING to #ifdef CONFIG_PGHOT.

So with CONFIG_PGHOT=y the fields exist regardless of
CONFIG_NUMA_BALANCING. PGHOT depends only on NUMA_MIGRATION && SPARSEMEM,
so PGHOT=y with NUMA_BALANCING=n is a supported configuration and builds
cleanly.

> diff --git a/mm/pghot-default.c b/mm/pghot-default.c
> new file mode 100644
> index 000000000000..ed66a867a644
> --- /dev/null
> +++ b/mm/pghot-default.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * pghot: Default mode
> + *
> + * 1 byte hotness record per PFN.
> + * Bucketed time and frequency tracked as part of the record.
> + * Promotion to @sysctl_pghot_target_nid by default.
> + */
> +
> +#include <linux/pghot.h>
> +#include <linux/jiffies.h>
> +
> +/* pghot-default doesn't store and hence no NID validation is required */
> +bool pghot_nid_valid(int nid)
> +{
> +	return true;
> +}
> +
> +/*
> + * @time is regular time, @old_time is bucketed time.
> + */
> +unsigned long pghot_access_latency(unsigned long old_time, unsigned long time)
> +{
> +	time &= PGHOT_TIME_BUCKETS_MASK;
> +	old_time <<= PGHOT_TIME_BUCKETS_SHIFT;
> +
> +	return jiffies_to_msecs((time - old_time) & PGHOT_TIME_BUCKETS_MASK);
> +}

> Does this time calculation allow cold pages to be falsely promoted? Since the
> time bucket uses 5 bits, PGHOT_TIME_BUCKETS_MASK wraps around approximately
> every 4 seconds at HZ=1000. If an access occurs hours later, the modulo
> arithmetic might yield a small random latency, tricking the threshold check
> and causing widespread false promotions.

This is the same scheme that the existing NUMAB=2 has always used:
numa_hint_fault_latency() computes (time - last_time) & PAGE_ACCESS_TIME_MASK.
pghot's default (1-byte) record simply keeps that accuracy-vs-space tradeoff;
precise mode (CONFIG_PGHOT_PRECISE) widens the window to ~16s.

This is not the only check that allows promotion. There are others as well like
frequency threshold (by default 2 accesses), pgdat_free_space_enough() check,
the dynamic hot-threshold adjustment, and pghot_promote_rate_limit.

A genuinely cold page may get promoted (and will eventually get demoted too
under pressure) but I don't think it will lead to widespread false promotions.
I'll add a comment to pghot_access_latency() documenting the bounded-window
tradeoff so it's explicit.

> +}
> diff --git a/mm/pghot.c b/mm/pghot.c
> +/*
> + * Walks the PFNs of the zone, isolates and migrates them in batches.
> + */
> +static void kmigrated_walk_zone(unsigned long start_pfn, unsigned long end_pfn,
> +				int src_nid)
> +{
> +	struct mem_cgroup *cur_memcg = NULL;
> +	int cur_nid = NUMA_NO_NODE;
> +	LIST_HEAD(migrate_list);
> +	int batch_count = 0;
> +	struct folio *folio;
> +	struct page *page;
> +	unsigned long pfn;
> +
> +	pfn = start_pfn;
> +	do {
> +		int nid = NUMA_NO_NODE, nr = 1;
> +		struct mem_cgroup *memcg;
> +		unsigned long time = 0;
> +		int freq = 0;
> +
> +		if (!pfn_valid(pfn))
> +			goto out_next;
> +
> +		page = pfn_to_online_page(pfn);
> +		if (!page)
> +			goto out_next;
> +
> +		folio = page_folio(page);
> +		if (!folio_try_get(folio))
> +			goto out_next;
> +
> +		if (unlikely(page_folio(page) != folio)) {
> +			folio_put(folio);
> +			goto out_next;
> +		}
> +
> +		nr = folio_nr_pages(folio);
> +		if (folio_nid(folio) != src_nid) {
> +			folio_put(folio);
> +			goto out_next;
> +		}
> +
> +		if (!folio_test_lru(folio)) {
> +			folio_put(folio);
> +			goto out_next;
> +		}
> +
> +		if (pghot_get_hotness(pfn, &nid, &freq, &time)) {
> +			folio_put(folio);
> +			goto out_next;
> +		}
> +
> +		if (folio_nid(folio) == nid) {
> +			folio_put(folio);
> +			goto out_next;
> +		}
> +
> +		if (migrate_misplaced_folio_prepare(folio, NULL, nid)) {
> +			folio_put(folio);
> +			goto out_next;
> +		}
> +
> +		rcu_read_lock();
> +		memcg = folio_memcg(folio);
> +		rcu_read_unlock();
> +		if (cur_nid == NUMA_NO_NODE) {
> +			cur_nid = nid;
> +			cur_memcg = memcg;
> +		}
> +
> +		/* If NID or memcg changed, flush the previous batch first */
> +		if (cur_nid != nid || cur_memcg != memcg) {
> +			if (!list_empty(&migrate_list))
> +				promote_misplaced_memcg_folios(&migrate_list, cur_nid);
> +			cur_nid = nid;
> +			cur_memcg = memcg;
> +			batch_count = 0;
> +			cond_resched();
> +		}
> +
> +		list_add(&folio->lru, &migrate_list);
> +		folio_put(folio);
> +
> +		if (++batch_count > READ_ONCE(kmigrated_batch_nr)) {
> +			promote_misplaced_memcg_folios(&migrate_list, cur_nid);
> +			batch_count = 0;
> +			cond_resched();
> +		}
> +out_next:
> +		pfn += nr;
> +	} while (pfn < end_pfn);

> Is there a risk of unbounded PFN scanning causing soft lockups here?
> cond_resched() is only called when a batch is successfully flushed. If a
> section's hot bit is set but no pages qualify for migration and the code takes
> the goto out_next path, the loop might spin through millions of PFNs without
> ever yielding the CPU.

> Is there a risk of unbounded PFN scanning causing soft lockups here?
> ... the loop might spin through millions of PFNs without ever yielding

kmigrated_walk_zone() is called per-section, so a single walk scans at most
PAGES_PER_SECTION PFNs (32768 on x86_64) not millions. The worst case can occur
if kmigrated_do_work()'s outer loop walks every hot section of the node
back-to-back without resulting in anything to migrate.

Hence let me add a resched point once per section in that outer loop.

> Could this PFN advancement skip valid folios if the loop starts on a tail page?
> If folio_try_get() fails on a head page, nr remains 1 and pfn advances to a
> tail page. On the next iteration, resolving the head folio succeeds, setting nr
> to folio_nr_pages(folio). Adding this full length to the tail page's PFN will
> overshoot the actual end of the folio and skip subsequent pages.

Theoretically yes, but not sure if that can happen for standard THPs. And
gigantic (1G) folios are anyway skipped as they aren't part of LRU.

But then I will fix it to ensure that it won't ever happen


> +	if (!list_empty(&migrate_list))
> +		promote_misplaced_memcg_folios(&migrate_list, cur_nid);
> +}
> +
> +static void kmigrated_do_work(pg_data_t *pgdat)
> +{
> +	unsigned long section_nr, s_begin, start_pfn;
> +	struct pghot_hot_map *hot_map;
> +	struct mem_section *ms;
> +	bool hot;
> +	int nid;
> +
> +	clear_bit(PGDAT_KMIGRATED_ACTIVATE, &pgdat->flags);
> +	s_begin = next_present_section_nr(-1);
> +	for_each_present_section_nr(s_begin, section_nr) {

> Does this skip the very first present memory section? The macro
> for_each_present_section_nr() internally calls next_present_section_nr() on
> its start parameter, which means it will immediately advance past s_begin
> and never process the first section for promotion.

No. next_present_section_nr() is called with (start - 1) which guarantees that
the first present section is considered in the loop.

> +		start_pfn = section_nr_to_pfn(section_nr);
> +		ms = __nr_to_section(section_nr);
> +
> +		if (!pfn_valid(start_pfn))
> +			continue;

> What happens if a memory section is only partially populated and starts with a
> memory hole? This check appears to skip the entire section based solely on the
> first PFN, which would ignore any valid hot folios located further inside the
> section.

Hmm what kind of sections are those which start with a hole? Device memory
sections? Those are already excluded from pghot's tracking. pghot tracks only
on-LRU RAM folios whose sections won't get skipped with this check, I believe.

> +
> +		nid = pfn_to_nid(start_pfn);
> +		if (node_is_toptier(nid) || nid != pgdat->node_id)
> +			continue;
> +
> +		rcu_read_lock();
> +		hot_map = rcu_dereference(ms->hot_map);
> +		hot = hot_map &&
> +		      test_and_clear_bit(PGHOT_SECTION_HOT_BIT, &hot_map->flags);
> +		rcu_read_unlock();
> +
> +		if (!hot)
> +			continue;
> +
> +		kmigrated_walk_zone(start_pfn, start_pfn + PAGES_PER_SECTION,
> +				    pgdat->node_id);
> +	}
> +}
> +
> 
> +
> +static void pghot_free_hot_map(struct mem_section *ms)
> +{
> +	struct pghot_hot_map *hot_map;
> +
> +	hot_map = rcu_dereference_protected(ms->hot_map, 1);
> +	if (!hot_map)
> +		return;
> +
> +	RCU_INIT_POINTER(ms->hot_map, NULL);
> +	kfree_rcu(hot_map, rcu);

> Can two racing threads double-free the hot_map pointer here? Because
> rcu_dereference_protected() is used without a lock, concurrent calls might both
> read the same non-NULL pointer and call kfree_rcu() on it.

pghot_free_hot_map() is called for three places:

1. pghot_offline_sec_hotmap() — from the notifier on MEM_OFFLINE /
MEM_CANCEL_ONLINE.
2. pghot_online_sec_hotmap() rollback — from the notifier on MEM_GOING_ONLINE.
3. pghot_destroy_hot_map() — only from pghot_init()'s failure path.

First two paths are already serialized by mem_hotplug_lock.
Regarding the third one, pghot_destroy_hot_map() needs to run under
get_online_mems() to address your next two comments. That would close the window
for path 3 as well.

> +}
> +
> +static int pghot_alloc_hot_map(struct mem_section *ms, int nid)
> +{
> +	struct pghot_hot_map *hot_map;
> +
> +	hot_map = kzalloc_node(struct_size(hot_map, phi, PAGES_PER_SECTION),
> +			       GFP_KERNEL, nid);
> +	if (!hot_map)
> +		return -ENOMEM;
> +
> +	rcu_assign_pointer(ms->hot_map, hot_map);
> +	return 0;
> +}
> +
> +static void pghot_offline_sec_hotmap(unsigned long start_pfn,
> +				     unsigned long nr_pages)
> +{
> +	unsigned long start, end, pfn;
> +	struct mem_section *ms;
> +
> +	start = SECTION_ALIGN_DOWN(start_pfn);
> +	end = SECTION_ALIGN_UP(start_pfn + nr_pages);
> +
> +	for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
> +		ms = __pfn_to_section(pfn);
> +		if (!ms)
> +			continue;
> +
> +		pghot_free_hot_map(ms);
> +	}
> +}
> +
> +static int pghot_online_sec_hotmap(unsigned long start_pfn,
> +				   unsigned long nr_pages)
> +{
> +	int nid = pfn_to_nid(start_pfn);
> +	unsigned long start, end, pfn;
> +	struct mem_section *ms;
> +	int fail = 0;
> +
> +	/*
> +	 * No section hot maps for toptier sections.
> +	 */
> +	if (node_is_toptier(nid))
> +		return 0;
> +
> +	start = SECTION_ALIGN_DOWN(start_pfn);
> +	end = SECTION_ALIGN_UP(start_pfn + nr_pages);
> +
> +	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION) {
> +		ms = __pfn_to_section(pfn);
> +		if (!ms || rcu_access_pointer(ms->hot_map))
> +			continue;
> +
> +		fail = pghot_alloc_hot_map(ms, nid);
> +	}
> +
> +	if (!fail)
> +		return 0;
> +
> +	/* rollback: free the sections allocated before the failure */
> +	end = pfn - PAGES_PER_SECTION;
> +	for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
> +		ms = __pfn_to_section(pfn);
> +		if (ms)
> +			pghot_free_hot_map(ms);
> +	}
> +	return -ENOMEM;
> +}

> Could this rollback loop inadvertently destroy hotness metadata for pre-existing
> online sections? The loop iterates from start to the failure point and frees
> every hot_map without checking if this specific function invocation was the one
> that originally allocated it.

Though there is a check to skip allocation if the hot_map already exists, it is
not really possible to hit that as I think memory hotplugs are section aligned
and we free hot_maps of those sections (ranging from start to failure pfn) for
which allocation succeded during this onlining.

The only way the section being onlined could have a map is due to the
setup-vs-notifier race, which I will fix as explained in the previous and the
next review comments.

> +
> +static int pghot_memhp_callback(struct notifier_block *self,
> +				unsigned long action, void *arg)
> +{
> +	struct memory_notify *mn = arg;
> +	int ret = 0;
> +
> +	switch (action) {
> +	case MEM_GOING_ONLINE:
> +		ret = pghot_online_sec_hotmap(mn->start_pfn, mn->nr_pages);
> +		break;
> +	case MEM_OFFLINE:
> +	case MEM_CANCEL_ONLINE:
> +		pghot_offline_sec_hotmap(mn->start_pfn, mn->nr_pages);
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +static struct notifier_block pghot_mem_notifier = {
> +	.notifier_call = pghot_memhp_callback,
> +	.priority = DEFAULT_CALLBACK_PRI,
> +};
> +
> +static void pghot_destroy_hot_map(void)
> +{
> +	unsigned long section_nr, s_begin;
> +	struct mem_section *ms;
> +
> +	s_begin = next_present_section_nr(-1);
> +	for_each_present_section_nr(s_begin, section_nr) {
> +		ms = __nr_to_section(section_nr);
> +		pghot_free_hot_map(ms);
> +	}
> +
> +	unregister_memory_notifier(&pghot_mem_notifier);

> Is there a race condition with the memory hotplug notifier during teardown?
> Because the maps are freed before the notifier is unregistered, a concurrent
> memory offline event could trigger the callback and attempt to free the same
> map again.

See the reply to next comment.


> +}
> +
> +static int pghot_setup_hot_map(void)
> +{
> +	unsigned long section_nr, s_begin, start_pfn;
> +	struct mem_section *ms;
> +	int nid, ret;
> +
> +	ret = register_memory_notifier(&pghot_mem_notifier);
> +	if (ret)
> +		return ret;
> +
> +	s_begin = next_present_section_nr(-1);
> +	for_each_present_section_nr(s_begin, section_nr) {
> +		ms = __nr_to_section(section_nr);
> +		start_pfn = section_nr_to_pfn(section_nr);
> +		nid = pfn_to_nid(start_pfn);
> +
> +		if (node_is_toptier(nid) || !pfn_valid(start_pfn))
> +			continue;
> +
> +		if (pghot_alloc_hot_map(ms, nid))
> +			goto out_free_hot_map;
> +	}
> +	return 0;

> Can a concurrent memory hotplug event leak hot_map structures during setup?
> The notifier is registered before the loop allocates maps for pre-existing
> sections. If a memory online event triggers the callback concurrently, it could
> allocate a map that this loop subsequently overwrites and leaks.

Both the above are locking issues. They can be fixed by holding
get_online_mems() in both setup and destroy paths.

Regards,
Bharata.