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

[Matthew Wilcox <willy@infradead.org>]

On Mon, May 09, 2022 at 06:57:33AM +0000, CGEL wrote:
> 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.

No, you misunderstand.  Is it useful to have the "force KSM off"
functionality?  ie code which has been modified to allow KSM, but
then overridden by an admin?

> 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'