Re: [PATCH v8 4/8] mm: pghot: Precision mode for pghot

[Reply to Shashiko review]

On 28-Jul-26 11:13 AM, Bharata B Rao wrote:
> Default pghot stores hotness in a 1‑byte record per PFN, limiting
> frequency to 2 bits, time to a 5‑bit bucket, and preventing storage
> of per‑PFN toptier NID. This restricts time granularity and forces
> all promotions to use the global pghot_target_nid.
> 
> This patch adds an optional precision mode (CONFIG_PGHOT_PRECISE)
> that expands the hotness record to 4 bytes (u32) and provides:
> 
> - 10‑bit NID field for per‑PFN promotion target,
> - 3‑bit frequency field (freq_threshold range 1–7),
> - 14‑bit time field offering finer recency tracking,
> - MSB migrate‑ready bit.
> 
> Precision mode improves placement accuracy on systems with multiple
> toptier nodes and provides higher‑resolution hotness tracking, at
> the cost of increasing metadata to 4 bytes per PFN.
> 
> Documentation, tunables, and the record layout are updated accordingly.
> 
> Signed-off-by: Bharata B Rao <[email protected]>
> ---
>  Documentation/admin-guide/mm/pghot.rst |  4 +-
>  include/linux/pghot.h                  | 39 ++++++++++++-
>  mm/Kconfig                             | 11 ++++
>  mm/Makefile                            |  7 ++-
>  mm/pghot-precise.c                     | 77 ++++++++++++++++++++++++++
>  mm/pghot.c                             | 13 +++--
>  6 files changed, 143 insertions(+), 8 deletions(-)
>  create mode 100644 mm/pghot-precise.c
> 
> diff --git a/Documentation/admin-guide/mm/pghot.rst b/Documentation/admin-guide/mm/pghot.rst
> index 0edbe0082816..6d9b3d9522d1 100644
> --- a/Documentation/admin-guide/mm/pghot.rst
> +++ b/Documentation/admin-guide/mm/pghot.rst
> @@ -56,7 +56,7 @@ Path: /proc/sys/vm/pghot_target_nid
>  Path: /proc/sys/vm/pghot_freq_threshold
>  
>  - Minimum access frequency before a page is marked ready for promotion.
> -  Range is 1 to 3 in default mode.
> +  Range is 1 to 3 in default mode and 1 to 7 in precision mode.
>  - Default: 2
>  - Example:
>    # sysctl vm.pghot_freq_threshold=1
> @@ -68,7 +68,7 @@ Path: /proc/sys/vm/pghot_promote_freq_window_ms
>  - Controls the time window (in ms) for counting access frequency. A page is
>    considered hot only when **pghot_freq_threshold** number of accesses occur
>    with this time period.
> -- Default: 3000 (3 seconds)
> +- Default: 3000 (3 seconds) in default mode and 5000 (5s) in precision mode.
>  - Example:
>    # sysctl vm.pghot_promote_freq_window_ms=3000
>  
> diff --git a/include/linux/pghot.h b/include/linux/pghot.h
> index 7b85d717f410..313e5a6973e4 100644
> --- a/include/linux/pghot.h
> +++ b/include/linux/pghot.h
> @@ -37,8 +37,40 @@ DECLARE_STATIC_KEY_FALSE(pghot_src_hwhints);
>  
>  #define PGHOT_DEFAULT_NODE		0
>  
> +#if defined(CONFIG_PGHOT_PRECISE)
>  #define PGHOT_FREQ_WINDOW_MIN		(1 * MSEC_PER_SEC)
> -#define PGHOT_FREQ_WINDOW_DEFAULT	(3 * MSEC_PER_SEC)
> +#define PGHOT_FREQ_WINDOW_DEFAULT	(5 * MSEC_PER_SEC)
> +
> +/*
> + * Bits 0-26 are used to store nid, frequency and time.
> + * Bits 27-30 are unused now.
> + * Bit 31 is used to indicate the page is ready for migration.
> + */
> +#define PGHOT_MIGRATE_READY		31
> +
> +#define PGHOT_NID_WIDTH			10
> +#define PGHOT_FREQ_WIDTH		3
> +/* time is stored in 14 bits which can represent up to 16s with HZ=1000 */
> +#define PGHOT_TIME_WIDTH		14
> +
> +#define PGHOT_NID_SHIFT			0
> +#define PGHOT_FREQ_SHIFT		(PGHOT_NID_SHIFT + PGHOT_NID_WIDTH)
> +#define PGHOT_TIME_SHIFT		(PGHOT_FREQ_SHIFT + PGHOT_FREQ_WIDTH)
> +
> +#define PGHOT_NID_MASK			GENMASK(PGHOT_NID_WIDTH - 1, 0)
> +#define PGHOT_FREQ_MASK			GENMASK(PGHOT_FREQ_WIDTH - 1, 0)
> +#define PGHOT_TIME_MASK			GENMASK(PGHOT_TIME_WIDTH - 1, 0)
> +
> +#define PGHOT_NID_MAX			((1 << PGHOT_NID_WIDTH) - 1)
> +#define PGHOT_FREQ_MAX			((1 << PGHOT_FREQ_WIDTH) - 1)
> +#define PGHOT_TIME_MAX			((1 << PGHOT_TIME_WIDTH) - 1)
> +#define PGHOT_FREQ_WINDOW_MAX		PGHOT_TIME_MAX
> +
> +typedef u32 phi_t;

> This is a pre-existing issue, but expanding the size of phi_t to 4 bytes
> significantly worsens the reliability of hot_map allocations.
> In mm/pghot.c, pghot_alloc_hot_map() allocates the tracking structure:
>    hot_map = kzalloc_node(struct_size(hot_map, phi, PAGES_PER_SECTION),
>                           GFP_KERNEL, nid);
>With 4 bytes per PFN, the allocation size for a standard section grows to
>over 131KB, requiring an order-6 contiguous physical allocation. Since
>high-order GFP_KERNEL allocations frequently fail under memory fragmentation,
>could this cause memory hotplug operations to silently fail to track hot
>pages for newly onlined memory sections?
>Would it be better to convert this to use kvzalloc_node() and kvfree_rcu()
>to avoid the contiguous physical memory requirement?

Agreed. Will switch to kvzalloc_node() and kvfree_rcu().

> +
> +#else	/* !CONFIG_PGHOT_PRECISE */
> +#define PGHOT_FREQ_WINDOW_MIN		(1 * MSEC_PER_SEC)
> +#define PGHOT_FREQ_WINDOW_DEFAULT       (3 * MSEC_PER_SEC)
>  
>  /*
>   * Bits 0-6 are used to store frequency and time.
> @@ -66,6 +98,11 @@ DECLARE_STATIC_KEY_FALSE(pghot_src_hwhints);
>  
>  typedef u8 phi_t;
>  
> +static_assert(MAX_NUMNODES <= (1 << PGHOT_NID_WIDTH),
> +	      "pghot precise nid field too narrow for MAX_NUMNODES");

> Is this static_assert placed in the correct block?
> It appears to be inside the #else block for CONFIG_PGHOT_PRECISE, which means
> it will be disabled precisely when precision mode is enabled.
> If a kernel is compiled with CONFIG_PGHOT_PRECISE=y and MAX_NUMNODES > 1024,
> the compiler won't catch the narrow NID field. Could this lead to NIDs being
> silently truncated during runtime, causing pages to be migrated to incorrect
> NUMA nodes?

Yes, I got the placement wrong, it should have been within CONFIG_PGHOT_PRECISE.

> +
> +#endif /* CONFIG_PGHOT_PRECISE */
> +
>  #define PGHOT_RECORD_SIZE		sizeof(phi_t)
>  
>  #define PGHOT_SECTION_HOT_BIT		0
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 0a5bcd5d45ed..955a826ecfe9 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -1523,6 +1523,17 @@ config PGHOT
>  	  This adds 1 byte of metadata overhead per page in lower-tier
>  	  memory nodes.
>  
> +config PGHOT_PRECISE
> +	bool "Hot page tracking precision mode"
> +	default n
> +	depends on PGHOT
> +	help
> +	  Enables precision mode for tracking hot pages with pghot sub-system.
> +	  Adds fine-grained access time tracking and explicit toptier target
> +	  NID tracking. Precise hot page tracking comes at the cost of using
> +	  4 bytes per page against the default one byte per page. Preferable
> +	  to enable this on systems with multiple nodes in toptier.
> +
>  source "mm/damon/Kconfig"
>  
>  endmenu
> diff --git a/mm/Makefile b/mm/Makefile
> index 4939a1a74c1d..83bac64cab26 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -147,4 +147,9 @@ obj-$(CONFIG_SHRINKER_DEBUG) += shrinker_debug.o
>  obj-$(CONFIG_EXECMEM) += execmem.o
>  obj-$(CONFIG_TMPFS_QUOTA) += shmem_quota.o
>  obj-$(CONFIG_LAZY_MMU_MODE_KUNIT_TEST) += tests/lazy_mmu_mode_kunit.o
> -obj-$(CONFIG_PGHOT) += pghot.o pghot-default.o
> +obj-$(CONFIG_PGHOT) += pghot.o
> +ifdef CONFIG_PGHOT_PRECISE
> +obj-$(CONFIG_PGHOT) += pghot-precise.o
> +else
> +obj-$(CONFIG_PGHOT) += pghot-default.o
> +endif
> diff --git a/mm/pghot-precise.c b/mm/pghot-precise.c
> new file mode 100644
> index 000000000000..df82f2068a99
> --- /dev/null
> +++ b/mm/pghot-precise.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * pghot: Precision mode
> + *
> + * 4 byte hotness record per PFN (u32)
> + * NID, time and frequency tracked as part of the record.
> + */
> +
> +#include <linux/pghot.h>
> +#include <linux/jiffies.h>
> +#include <linux/memory-tiers.h>
> +
> +bool pghot_nid_valid(int nid)
> +{
> +	if (nid != NUMA_NO_NODE &&
> +	    (!numa_valid_node(nid) || nid > PGHOT_NID_MAX ||
> +	     !node_online(nid) || !node_is_toptier(nid)))
> +		return false;
> +
> +	return true;
> +}
> +
> +unsigned long pghot_access_latency(unsigned long old_time, unsigned long time)
> +{
> +	return jiffies_to_msecs((time - old_time) & PGHOT_TIME_MASK);
> +}
> +
> +bool pghot_update_record(phi_t *phi, int nid, unsigned long now)
> +{
> +	phi_t freq, old_freq, hotness, old_hotness, old_time;
> +	phi_t time = now & PGHOT_TIME_MASK;
> +
> +	nid = (nid == NUMA_NO_NODE) ? sysctl_pghot_target_nid : nid;
> +	old_hotness = READ_ONCE(*phi);
> +
> +	do {
> +		bool new_window = false;
> +
> +		old_freq = (old_hotness >> PGHOT_FREQ_SHIFT) & PGHOT_FREQ_MASK;
> +		old_time = (old_hotness >> PGHOT_TIME_SHIFT) & PGHOT_TIME_MASK;
> +
> +		if (pghot_access_latency(old_time, time) > sysctl_pghot_freq_window)
> +			new_window = true;

> Can the 14-bit time mask cause cold pages to be incorrectly evaluated as hot?
> Since PGHOT_TIME_MASK is 14 bits, the tracked time value wraps around roughly
> every 16.38 seconds (assuming HZ=1000). 
> If a cold page is accessed once, left completely idle for a long period, and
> then accessed again, is it possible that the elapsed time calculated using the
> wrapped mask randomly falls within the default 5-second sysctl window?
> Could this lead to spurious hotness promotions and unnecessary memory
> migrations for pages that aren't actually accessed frequently?

For the reasons explained in the similar path in pghot-default, this is okay.
This is in fact better than pghot-default as 14bits give a better range of
~16s.

Regards,
Bharata.