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

[CGEL <cgel.zte@gmail.com>]

On Sun, May 08, 2022 at 07:03:36PM +0100, Matthew Wilcox wrote:
> On Sun, May 08, 2022 at 09:27:10AM +0000, cgel.zte@gmail.com wrote:
> > 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.
> 
> Is that actually a useful feature?  Otherwise, we could simply turn
> on/off the existing MMF_VM_MERGEABLE flag instead of introducing this
> new bool.
> 
I think this will be very useful for those apps which are very likely to
cause Same Pages in memory and users and operators are not willing to
modified the source codes for any reasons.

Besides, simply turning of/off the existing MMF_VM_MERGEABLE flag may be
not feasible because madvise will also turn on the MMF_VM_MERGEABLE
flag.

I think the following suggestions is good, and I will resend a patch.
> > +Controlling KSM with procfs
> > +===========================
> > +
> > +KSM can also operate on anonymous areas of address space of those processes's
> > +knob ``/proc/<pid>/ksm_force`` is on, even if app codes doesn't call madvise()
> > +explicitly to advise specific areas as MADV_MERGEABLE.
> > +
> > +You can set ksm_force to 1 to force all anonymous and qualified VMAs of
> > +this process to be involved in KSM scanning. But It is effective only when the
> > +klob of ``/sys/kernel/mm/ksm/run`` is set as 1.
> 
> I think that last sentence doesn't really add any value.
> 
> > +	memset(buffer, 0, sizeof(buffer));
> > +	if (count > sizeof(buffer) - 1)
> > +		count = sizeof(buffer) - 1;
> > +	if (copy_from_user(buffer, buf, count)) {
> > +		err = -EFAULT;
> > +		goto out_return;
> 
> This feels a bit unnecessary.  Just 'return -EFAULT' here.
> 
> > +	}
> > +
> > +	err = kstrtoint(strstrip(buffer), 0, &force);
> > +
> > +	if (err)
> > +		goto out_return;
> 
> 'return err'
> 
> > +	if (force != 0 && force != 1) {
> > +		err = -EINVAL;
> > +		goto out_return;
> 
> 'return -EINVAL'
> 
> > +	}
> > +
> > +	task = get_proc_task(file_inode(file));
> > +	if (!task) {
> > +		err = -ESRCH;
> > +		goto out_return;
> 
> 'return -ESRCH'