linux-kernel - Re: [PATCH v7] mm/ksm: introduce ksm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20220512134111.cfd6c9ee639fe81d4f55a1f3@linux-foundation.org>
Date:   Thu, 12 May 2022 13:41:11 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     cgel.zte@...il.com
Cc:     ammarfaizi2@...weeb.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        ran.xiaokai@....com.cn, wang.yong12@....com.cn,
        willy@...radead.org, xu.xin16@....com.cn, yang.yang29@....com.cn,
        zhang.yunkai@....com.cn
Subject: Re: [PATCH v7] mm/ksm: introduce ksm_force for each process

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

> From: xu xin <xu.xin16@....com.cn>
> 
> 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@....com.cn>
> Reviewed-by: Yang Yang <yang.yang29@....com.cn>
> Reviewed-by: Ran Xiaokai <ran.xiaokai@....com.cn>
> Reviewed-by: wangyong <wang.yong12@....com.cn>
> Reviewed-by: Yunkai Zhang <zhang.yunkai@....com.cn>
> Suggested-by: Matthew Wilcox <willy@...radead.org>
> Suggested-by: Ammar Faizi <ammarfaizi2@...weeb.org>

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;
_