Re: [PATCH 1/1] mm/ksm: add ksm_pages_sharing for each process to calculate profit more accurately

[David Hildenbrand <david@redhat.com>]

On 06.06.25 09:03, Longlong Xia wrote:

CCing Stefan.

> The general_profit_show() function only considers ksm_pages_sharing,
> while ksm_process_profit() includes both ksm_pages_sharing and
> ksm_pages_shared for each process. This leads to a mismatch between
> the total profits from ksm_process_profit() and general_profit_show().
> 
> Based on my tests, the sum of ksm_process_profit() for all processes
> can be up to 20% higher than general_profit_show(), depending on
> the size of page_shared. For individual processes, the ratio of
> ksm_pages_sharing to ksm_merging_pages is usually not equal to 1.
> 
> To resolve this, we suggest introducing ksm_pages_sharing for each
> process to accurately calculate its pages_sharing, ensuring
> ksm_process_profit() reflects shared memory benefits more accurately.
> 
> Add a new proc file named as ksm_pages_sharing both under /proc/<pid>/

It's an entry in the file, not a new file.

> and /proc/self/ksm_stat/ to indicate the saved pages of this process.
> (not including ksm_zero_pages)

Curious, why is updating ksm_process_profit() insufficient and we also 
have to expose ksm_pages_sharing?

> 
> Suggested-by: Xu Xin <xu.xin16@zte.com.cn>
> Signed-off-by: Longlong Xia <xialonglong@kylinos.cn>
> ---
>   Documentation/admin-guide/mm/ksm.rst |  5 +++--
>   Documentation/filesystems/proc.rst   |  8 ++++++++
>   fs/proc/base.c                       | 18 ++++++++++++++++++
>   include/linux/mm_types.h             |  5 +++++
>   mm/ksm.c                             | 12 ++++++++----
>   5 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/admin-guide/mm/ksm.rst b/Documentation/admin-guide/mm/ksm.rst
> index ad8e7a41f3b5..0b33ef98930f 100644
> --- a/Documentation/admin-guide/mm/ksm.rst
> +++ b/Documentation/admin-guide/mm/ksm.rst
> @@ -256,9 +256,10 @@ several times, which are unprofitable memory consumed.
>   	process_profit =~ ksm_saved_pages * sizeof(page) -
>   			  ksm_rmap_items * sizeof(rmap_item).
>   
> -   where ksm_saved_pages equals to the sum of ``ksm_merging_pages`` and
> +   where ksm_saved_pages equals to the sum of ``ksm_pages_sharing`` and
>      ``ksm_zero_pages``, both of which are shown under the directory
> -   ``/proc/<pid>/ksm_stat``, and ksm_rmap_items is also shown in
> +   ``/proc/<pid>/ksm_stat``, ksm_merging_pages and ksm_rmap_items are
> +   also shown in
>      ``/proc/<pid>/ksm_stat``. The process profit is also shown in
>      ``/proc/<pid>/ksm_stat`` as ksm_process_profit.
>   
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 2a17865dfe39..e14ea8389500 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -2290,6 +2290,7 @@ Example
>       / # cat /proc/self/ksm_stat
>       ksm_rmap_items 0
>       ksm_zero_pages 0
> +    ksm_pages_sharing 0
>       ksm_merging_pages 0
>       ksm_process_profit 0
>       ksm_merge_any: no
> @@ -2312,6 +2313,13 @@ ksm_zero_pages
>   When /sys/kernel/mm/ksm/use_zero_pages is enabled, it represent how many
>   empty pages are merged with kernel zero pages by KSM.
>   
> +ksm_pages_sharing
> +^^^^^^^^^^^^^^^^^
> +
> +It represents how many pages saved of this process.
> +(not including ksm_zero_pages). It is the same with what
> +/proc/<pid>/ksm_pages_sharing shows.
 > +>   ksm_merging_pages
>   ^^^^^^^^^^^^^^^^^
>   
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c667702dc69b..327bf82acf54 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3262,6 +3262,21 @@ static int proc_pid_ksm_merging_pages(struct seq_file *m, struct pid_namespace *
>   
>   	return 0;
>   }
> +
> +static int proc_pid_ksm_pages_sharing(struct seq_file *m, struct pid_namespace *ns,
> +				struct pid *pid, struct task_struct *task)
> +{
> +	struct mm_struct *mm;
> +
> +	mm = get_task_mm(task);
> +	if (mm) {
> +		seq_printf(m, "%lu\n", mm->ksm_pages_sharing);
> +		mmput(mm);
> +	}
> +
> +	return 0;
> +}
> +
>   static int proc_pid_ksm_stat(struct seq_file *m, struct pid_namespace *ns,
>   				struct pid *pid, struct task_struct *task)
>   {
> @@ -3272,6 +3287,7 @@ static int proc_pid_ksm_stat(struct seq_file *m, struct pid_namespace *ns,
>   	if (mm) {
>   		seq_printf(m, "ksm_rmap_items %lu\n", mm->ksm_rmap_items);
>   		seq_printf(m, "ksm_zero_pages %ld\n", mm_ksm_zero_pages(mm));
> +		seq_printf(m, "ksm_pages_sharing %lu\n", mm->ksm_pages_sharing);
>   		seq_printf(m, "ksm_merging_pages %lu\n", mm->ksm_merging_pages);
>   		seq_printf(m, "ksm_process_profit %ld\n", ksm_process_profit(mm));
>   		seq_printf(m, "ksm_merge_any: %s\n",
> @@ -3421,6 +3437,7 @@ static const struct pid_entry tgid_base_stuff[] = {
>   	ONE("seccomp_cache", S_IRUSR, proc_pid_seccomp_cache),
>   #endif
>   #ifdef CONFIG_KSM
> +	ONE("ksm_pages_sharing",  S_IRUSR, proc_pid_ksm_pages_sharing),
>   	ONE("ksm_merging_pages",  S_IRUSR, proc_pid_ksm_merging_pages),
>   	ONE("ksm_stat",  S_IRUSR, proc_pid_ksm_stat),
>   #endif
> @@ -3758,6 +3775,7 @@ static const struct pid_entry tid_base_stuff[] = {
>   	ONE("seccomp_cache", S_IRUSR, proc_pid_seccomp_cache),
>   #endif
>   #ifdef CONFIG_KSM
> +	ONE("ksm_pages_sharing",  S_IRUSR, proc_pid_ksm_pages_sharing),
>   	ONE("ksm_merging_pages",  S_IRUSR, proc_pid_ksm_merging_pages),
>   	ONE("ksm_stat",  S_IRUSR, proc_pid_ksm_stat),
>   #endif
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index d6b91e8a66d6..d260cb09c10a 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1176,6 +1176,11 @@ struct mm_struct {
>   		 * merging (not including ksm_zero_pages).
>   		 */
>   		unsigned long ksm_merging_pages;
> +		/*
> +		 * Represents how many pages saved of this process.
> +		 * (not including ksm_zero_pages).
> +		 */
> +		unsigned long ksm_pages_sharing;
>   		/*
>   		 * Represent how many pages are checked for ksm merging
>   		 * including merged and not merged.
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 8583fb91ef13..c2d85ea07b1c 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -824,6 +824,7 @@ static void remove_node_from_stable_tree(struct ksm_stable_node *stable_node)
>   	hlist_for_each_entry(rmap_item, &stable_node->hlist, hlist) {
>   		if (rmap_item->hlist.next) {
>   			ksm_pages_sharing--;
> +			rmap_item->mm->ksm_pages_sharing--;
>   			trace_ksm_remove_rmap_item(stable_node->kpfn, rmap_item, rmap_item->mm);
>   		} else {
>   			ksm_pages_shared--;
> @@ -976,8 +977,10 @@ static void remove_rmap_item_from_tree(struct ksm_rmap_item *rmap_item)
>   		folio_unlock(folio);
>   		folio_put(folio);
>   
> -		if (!hlist_empty(&stable_node->hlist))
> +		if (!hlist_empty(&stable_node->hlist)) {
>   			ksm_pages_sharing--;
> +			rmap_item->mm->ksm_pages_sharing--;
> +		}


Hm, I am wondering if that works. Stable nodes are not per MM, so can't 
we create an accounting imbalance for one MM somehow?

(did not look into all the details, just something that came to mind)

-- 
Cheers,

David / dhildenb