[lkml]   [2011]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages
On Sat, Mar 19, 2011 at 6:40 AM, Hugh Dickins <> wrote:
> On Fri, 18 Mar 2011, Nai Xia wrote:
>> >On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <>> wrote
>> > This patch obviously wasn't tested with CONFIG_KSM=n, which was a
>> > pretty basic patch-testing failure :(
>> Oops, I will be careful to avoid similar mistakes next time.
>> >
>> > I fixed up my tree with the below, but really the amount of ifdeffing
>> > is unacceptable - please find a cleaner way to fix up this patch.
>> Ok, I will have a try in my next patch submit.
> A couple of notes on that.
> akpm's fixup introduced an #ifdef CONFIG_KSM in mm/ksm.c: that should
> be, er, unnecessary - since ksm.c is only compiled when CONFIG_KSM=y.

This was lately pointed out by me and canceled by another patch in and CCed to your obsolete email
address: I think.

> And PageKsm(page) evaluates to 0 when CONFIG_KSM is not set, so the
> optimizer should eliminate code from most places without #ifdef:
> though you need to keep the #ifdef around display in /proc/meminfo
> itself, so as not to annoy non-KSM people with an always 0kB line.

This is just what I thought before I introduced NR_KSM_PAGES_SHARING,
which then did break the compiling. My mistake.

> But I am uncomfortable with the whole patch.
> Can you make a stronger case for it?  KSM is designed to have its own
> cycle, and to keep out of the way of the rest of mm as much as possible
> (not as much as originally hoped, I admit).  Do we really want to show
> its statistics in /proc/meminfo now?  And do we really care that they
> don't keep up with exiting processes when the scan rate is low?

OK, I have to explain, here.
This patch is actually a tiny part of a bunch of code I wrote to improve ksm
in several aspects(This is somewhat off the topic but if you are interested,
please take at look at, It's still on
very early

In my code, the inconsistency is amplified by non-uniform
scan speed for different VMAs and significantly improved max scan speed.
Then I think this patch may also be helpful to ksm itself. Just as you said,
I had thought it at least improves the accuracy.

> I am not asserting that we don't, nor am I nacking your patch:
> but I would like to hear more support for it, before it adds
> yet another line to our user interface in /proc/meminfo.

Then how about not touching the sexy meminfo and adding a new
interface file in /sys/kernel/mm/ksm/ ? OK, on condition that the bug
below can be properly solved.

> And there is an awkward little bug in your patch, which amplifies
> a more significant and shameful pair of bugs of mine in KSM itself -
> no wonder that I'm anxious about your patch going in!
> Your bug is precisely where akpm added the #ifdef in ksm.c.  The
> problem is that page_mapcount() is maintained atomically, generally
> without spinlock or pagelock: so the value of mapcount there, unless
> it is 1, can go up or down racily (as other processes sharing that
> anonymous page fork or unmap at the same time).

You are right, copy_one_pte does not take page lock. So it's definitely a
bug in my patch, although it did not appear in my tests. Actually, there is
another issue in my patch: It tries to count all the ptes, while actually only
those changed by ksmd really matter, those added by fork does not mean
memory savings. I had thought not taking the mapcount
, instead, only increase the count by one each time a pte is changed by ksmd,
but It seems also hard to tell a pte mapped to ksm page was previously
changed by ksmd or by fork when it gets unmapped.

So indeed, I have no idea to fix this bug for the time being.

> I could hardly complain about that, while suggesting above that more
> approximate numbers are good enough!  Except that, when KSM is turned
> off, there's a chance that you'd be left showing a non-0 kB in
> /proc/meminfo.  Then people will want a fix, and I don't yet know
> what that fix will be.
> My first bug is in the break_cow() technique used to get back to
> normal, when merging into a KSM page fails for one reason or another:
> that technique misses other mappings of the page.  I did have a patch
> in progress to fix that a few months ago, but it wasn't quite working,
> and then I realized the second bug: that even when successful, if
> VM_UNMERGEABLE has been used in forked processes, then we could end up
> with a KSM page in a VM_UNMERGEABLE area, which is against the spec.
> A solution to all three problems would be to revert to allocating a
> separate KSM page, instead of using one of the pages already there.
> But that feels like a regression, and I don't think anybody is really
> hurting from the current situation, so I've not jumped to fix it yet.
> Hugh

Yes, I agree on your point. Let's hope there is an efficient and
simple solution.
But for now, please drop this patch, Andrew.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at
Please read the FAQ at

 \ /
  Last update: 2011-03-19 15:59    [W:0.038 / U:5.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site