linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Petr Holasek <pholasek@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Hugh Dickins <hughd@google.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Chris Wright <chrisw@sous-sol.org>,
	Izik Eidus <izik.eidus@ravellosystems.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Anton Arapov <anton@redhat.com>
Subject: Re: KSM: numa awareness sysfs knob
Date: Wed, 25 Jan 2012 17:40:45 +0100	[thread overview]
Message-ID: <20120125164037.GA21071@dhcp-27-244.brq.redhat.com> (raw)
In-Reply-To: <20120124160350.17b6e92b.akpm@linux-foundation.org>

On Tue, 24 Jan 2012, Andrew Morton wrote:
> On Mon, 23 Jan 2012 11:29:28 +0100
> Petr Holasek <pholasek@redhat.com> wrote:
> 
> > This patch is based on RFC
> > 
> > https://lkml.org/lkml/2011/11/30/91
> > 
> > Introduces new sysfs binary knob /sys/kernel/mm/ksm/merge_nodes
> 
> It's not binary - it's ascii text!  "boolean" is a better term here ;)
> 

Of course, I'll fix it :)

> > which control merging pages across different numa nodes.
> > When it is set to zero only pages from the same node are merged,
> > otherwise pages from all nodes can be merged together (default behavior).
> > 
> > Typical use-case could be a lot of KVM guests on NUMA machine
> > where cpus from more distant nodes would have significant increase
> > of access latency to the merged ksm page. Switching merge_nodes
> > to 1 will result into these steps:
> > 
> > 	1) unmerging all ksm pages
> > 	2) re-merging all pages from VM_MERGEABLE vmas only within
> > 		their NUMA nodes.
> > 	3) lower average access latency to merged pages at the
> > 	   expense of higher memory usage.
> > 
> > Every numa node has its own stable & unstable trees because
> > of faster searching and inserting. Changing of merge_nodes
> > value breaks COW on all current ksm pages.
> > 
> 
> How useful is this code?  Do you have any performance testing results
> to help make the case for merging it?

I didn't any no performance testing, but number of nodes is still the same, 
the only difference is that they are distributed among more trees, 
so searching is faster within specified numa node. Every node includes pointer
to the tree's root, but I assume it is quite small payload for faster searching.
Or not?

> 
> Should the unmerged case be made permanent and not configurable?  IOW,
> what is the argument for continuing to permit the user to merge across
> nodes?
> 
> Should the code bother doing this unmerge when
> /sys/kernel/mm/ksm/merge_nodes is written to?  It would be simpler to
> expect the user to configure /sys/kernel/mm/ksm/merge_nodes prior to
> using KSM at all?

The only reason for this feature is being more user-friendly. But if 
we find some issue in doing merging/unmerging interactively, forcing user
to set merge_nodes value before first ksm run will be more safe.

> 
> > @@ -58,6 +58,9 @@ sleep_millisecs  - how many milliseconds ksmd should sleep before next scan
> >                     e.g. "echo 20 > /sys/kernel/mm/ksm/sleep_millisecs"
> >                     Default: 20 (chosen for demonstration purposes)
> >  
> > +merge_nodes      - specifies if pages from different numa nodes can be merged
> > +                   Default: 1
> 
> This documentation would be better if it informed the user about how to
> use merge_nodes.  What are the effects of altering it and why might
> they wish to do this?
> 
> >
> > ...
> >
> > +static ssize_t merge_nodes_store(struct kobject *kobj,
> > +				   struct kobj_attribute *attr,
> > +				   const char *buf, size_t count)
> > +{
> > +	int err;
> > +	long unsigned int knob;
> 
> Plain old "unsigned long" is more usual.
> 
> Better would be to make this "unsigned", to match ksm_merge_nodes.  Use
> kstrtouint() below.
> 
> >
> > ...
> >
> > @@ -1987,6 +2070,9 @@ static struct attribute *ksm_attrs[] = {
> >  	&pages_unshared_attr.attr,
> >  	&pages_volatile_attr.attr,
> >  	&full_scans_attr.attr,
> > +#ifdef CONFIG_NUMA
> > +	&merge_nodes_attr.attr,
> > +#endif
> 
> One might think that with CONFIG_NUMA=n, we just added a pile of
> useless codebloat to vmlinux.  But gcc is sneaky and removes the
> unreferenced functions.
> 
> However while doing so, gcc shows that it reads
> Documentation/SubmitChecklist, section 2:
> 
> mm/ksm.c:2017: warning: 'merge_nodes_attr' defined but not used
> 
> So...
> 
> diff -puN mm/ksm.c~ksm-numa-awareness-sysfs-knob-fix mm/ksm.c
> --- a/mm/ksm.c~ksm-numa-awareness-sysfs-knob-fix
> +++ a/mm/ksm.c
> @@ -1973,6 +1973,7 @@ static ssize_t run_store(struct kobject 
>  }
>  KSM_ATTR(run);
>  
> +#ifdef CONFIG_NUMA
>  static ssize_t merge_nodes_show(struct kobject *kobj,
>  				struct kobj_attribute *attr, char *buf)
>  {
> @@ -2015,6 +2016,7 @@ static ssize_t merge_nodes_store(struct 
>  	return count;
>  }
>  KSM_ATTR(merge_nodes);
> +#endif
>  
>  static ssize_t pages_shared_show(struct kobject *kobj,
>  				 struct kobj_attribute *attr, char *buf)
> _
> 

Apologize, I overlooked that. I'll fix it and other issues you pointed out
above in next version. 

Many thanks for reviewing!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2012-01-25 16:40 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-01-23 10:29 [PATCH] KSM: numa awareness sysfs knob Petr Holasek
2012-01-25  0:03 ` Andrew Morton
2012-01-25 16:40   ` Petr Holasek [this message]
  -- strict thread matches above, loose matches on Subject: below --
2011-11-30 10:37 [PATCH] [RFC] " Petr Holasek
2011-11-30 23:47 ` Andrew Morton
2011-12-01 10:16   ` Petr Holasek
2011-12-01 11:40     ` Nai Xia
2011-12-01 13:04       ` Petr Holasek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120125164037.GA21071@dhcp-27-244.brq.redhat.com \
    --to=pholasek@redhat.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=anton@redhat.com \
    --cc=chrisw@sous-sol.org \
    --cc=hughd@google.com \
    --cc=izik.eidus@ravellosystems.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).