linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave McCracken <dmccr@us.ibm.com>
To: Hugh Dickins <hugh@veritas.com>
Cc: Andrew Morton <akpm@osdl.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>,
	Linux Memory Management <linux-mm@kvack.org>
Subject: Re: [PATCH 1/1] Implement shared page tables
Date: Wed, 31 Aug 2005 11:40:28 -0500	[thread overview]
Message-ID: <6E5E4C275EEB99E7C5D096D9@[10.1.1.4]> (raw)
In-Reply-To: <Pine.LNX.4.61.0508311143340.15467@goblin.wat.veritas.com>


--On Wednesday, August 31, 2005 12:44:24 +0100 Hugh Dickins
<hugh@veritas.com> wrote:

> So you don't have Nick's test at the start of copy_page_range():
> 	if (!(vma->vm_flags & (VM_HUGETLB|VM_NONLINEAR|VM_RESERVED))) {
> 		if (!vma->anon_vma)
> 			return 0;
> 	}
> Experimental, yes, but Linus likes it enough to have fast-tracked it into
> his tree for 2.6.14.  My guess is that that patch (if its downsides prove
> manageable) takes away a lot of the point of shared page tables -
> I wonder how much of your "3% improvement".

Very little, actually.  The test does not create new processes as part of
the run.  The improvement is due to sharing of existing areas.

> I was going to say, doesn't randomize_va_space take away the rest of
> the point?  But no, it appears "randomize_va_space", as it currently
> appears in mainline anyway, is somewhat an exaggeration: it just shifts
> the stack a little, with no effect on the rest of the va space.
> But if it is to do more later, it may conflict with your interest.

I've been considering a future enhancement to my patch where it could share
page tables of any areas that share alignment, not just the same virtual
address.  That might allow sharing with randomization if the randomization
aligns things properly.

> The pud sharing and pmd sharing: perhaps they complicate the patch for
> negligible benefit?

The pmd sharing is necessary for ppc64 since it has to share at segment
size, plus it will be useful for very large regions.  I did pud for
completeness but you may be right that it's not useful.  It's all
configurable in any event.

>> +		if ((vma->vm_start <= base) &&
>> +	    (vma->vm_end >= end))
>> +		return 1;
>> 
> New Adventures in Coding Style ;)

New Adventures in Typos, actually :)  I'll fix.

> But most seriously: search the patch for the string "lock" and I find
> no change whatever to locking.  You're introducing page tables shared
> between different mms yet relying on the old mm->page_table_lock?
> You're searching a prio_tree for suitable matches to share, but
> taking no lock on that?  You're counting shares in an atomic,
> but not detecting when the count falls to 0 atomically?
> 
> And allied with that point on locking mms: there's no change to rmap.c,
> so how is its TLB flushing and cache flushing now supposed to work?
> page_referenced_one and try_to_unmap_one will visit all the vmas
> sharing the page table, yes, but (usually) only the first will
> satisfy the conditions and get flushed.

I'll go over the locking again.

> I'm not sure if it's worth pursuing shared page tables again or not.

The immediate clear benefits I see are a reduction in the number of page
table pages and a reduction in minor faults.  Keep in mind that faulting a
page into a shared page table makes it available to all other processes
sharing that area, eliminating the need for them to also take faults on it.

> You certainly need to sort the locking out to do so.  Wait a couple
> of weeks and I should have sent all the per-page-table-page locking
> in to -mm (to replace the pte xchging currently there): that should
> give what you need for locking pts independent of the mm.

I'll look things over in more detail.  I thought I had the locking issues
settled, but you raised some points I should revisit.

Dave McCracken


  parent reply	other threads:[~2005-08-31 16:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-08-30 22:13 [PATCH 1/1] Implement shared page tables Dave McCracken
2005-08-31 11:44 ` Hugh Dickins
2005-08-31 11:51   ` Arjan van de Ven
2005-08-31 13:42     ` Hugh Dickins
2005-08-31 14:31       ` Martin J. Bligh
2005-08-31 14:41         ` Arjan van de Ven
2005-08-31 15:06         ` Hugh Dickins
2005-08-31 15:39           ` Martin J. Bligh
2005-08-31 16:40   ` Dave McCracken [this message]
2005-09-02  1:58 ` Chen, Kenneth W
2005-09-02 16:40   ` Dave McCracken
2005-09-02  4:26 ` Chen, Kenneth W

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='6E5E4C275EEB99E7C5D096D9@[10.1.1.4]' \
    --to=dmccr@us.ibm.com \
    --cc=akpm@osdl.org \
    --cc=hugh@veritas.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).