Re: [PATCH v7] mm/ksm: introduce ksm_force for each process

[Andrew Morton]

On Thu, 12 May 2022 07:03:47 +0000 cgel.zte@xxxxxxxxx wrote:

> From: xu xin <xu.xin16@xxxxxxxxxx>
> 
> To use KSM, we have to explicitly call madvise() in application code,
> which means installed apps on OS needs to be uninstall and source code
> needs to be modified. It is inconvenient.
> 
> In order to change this situation, We add a new proc file ksm_force
> under /proc/<pid>/ to support turning on/off KSM scanning of a
> process's mm dynamically.
> 
> If ksm_force is set to 1, force all anonymous and 'qualified' VMAs
> of this mm to be involved in KSM scanning without explicitly calling
> madvise to mark VMA as MADV_MERGEABLE. But It is effective only when
> the klob of /sys/kernel/mm/ksm/run is set as 1.
> 
> If ksm_force is set to 0, cancel the feature of ksm_force of this
> process and unmerge those merged pages belonging to VMAs which is not
> madvised as MADV_MERGEABLE of this process, but leave MADV_MERGEABLE
> areas merged.

It certainly seems like a useful feature.

> Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
> Reviewed-by: Yang Yang <yang.yang29@xxxxxxxxxx>
> Reviewed-by: Ran Xiaokai <ran.xiaokai@xxxxxxxxxx>
> Reviewed-by: wangyong <wang.yong12@xxxxxxxxxx>
> Reviewed-by: Yunkai Zhang <zhang.yunkai@xxxxxxxxxx>
> Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Suggested-by: Ammar Faizi <ammarfaizi2@xxxxxxxxxxx>

This patch doesn't have your Signed-off-by:.  It should, because you
were on the delivery path.  This is described in
Documentation/process/submitting-patches.rst, "Developer's Certificate
of Origin".

I'll queue it for some testing but please do resend with that tag.


> +/* Check if vma is qualified for ksmd scanning */
> +static bool ksm_vma_check(struct vm_area_struct *vma)

I have trouble with "check" names, because the name doesn't convey what
is being checked, nor does the name convey whether it's checking for
truth or for falsity.

I suggest that "vma_scannable" is a more informative name.  It doesn't
need the "ksm_" prefix as this is a static file-local function.

See, with the name "vma_scannable", that comment which you added is
barely needed.

--- a/mm/ksm.c~mm-ksm-introduce-ksm_force-for-each-process-fix
+++ a/mm/ksm.c
@@ -335,7 +335,7 @@ static void __init ksm_slab_free(void)
 }
 
 /* Check if vma is qualified for ksmd scanning */
-static bool ksm_vma_check(struct vm_area_struct *vma)
+static bool vma_scannable(struct vm_area_struct *vma)
 {
 	unsigned long vm_flags = vma->vm_flags;
 
@@ -551,7 +551,7 @@ static struct vm_area_struct *find_merge
 	if (ksm_test_exit(mm))
 		return NULL;
 	vma = vma_lookup(mm, addr);
-	if (!vma || !ksm_vma_check(vma) || !vma->anon_vma)
+	if (!vma || !vma_scannable(vma) || !vma->anon_vma)
 		return NULL;
 	return vma;
 }
@@ -2328,7 +2328,7 @@ next_mm:
 		goto no_vmas;
 
 	for_each_vma(vmi, vma) {
-		if (!ksm_vma_check(vma))
+		if (!vma_scannable(vma))
 			continue;
 		if (ksm_scan.address < vma->vm_start)
 			ksm_scan.address = vma->vm_start;
_