From: Mel Gorman <mel@csn.ul.ie>
To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>,
Christoph Lameter <cl@linux-foundation.org>,
Adam Litke <agl@us.ibm.com>, Avi Kivity <avi@redhat.com>,
David Rientjes <rientjes@google.com>,
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>,
Rik van Riel <riel@redhat.com>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 03/12] mm: Share the anon_vma ref counts between KSM and page migration
Date: Sat, 20 Feb 2010 09:32:51 +0000 [thread overview]
Message-ID: <20100220093251.GI1445@csn.ul.ie> (raw)
In-Reply-To: <20100220124847.0b441e76.kamezawa.hiroyu@jp.fujitsu.com>
On Sat, Feb 20, 2010 at 12:48:47PM +0900, KAMEZAWA Hiroyuki wrote:
> On Fri, 19 Feb 2010 14:05:00 +0000
> Mel Gorman <mel@csn.ul.ie> wrote:
>
> > On Fri, Feb 19, 2010 at 09:18:59AM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu, 18 Feb 2010 18:02:33 +0000
> > > Mel Gorman <mel@csn.ul.ie> wrote:
> > >
> > > > For clarity of review, KSM and page migration have separate refcounts on
> > > > the anon_vma. While clear, this is a waste of memory. This patch gets
> > > > KSM and page migration to share their toys in a spirit of harmony.
> > > >
> > > > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> > >
> > > Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > >
> > > Nitpick:
> > > I think this refcnt has something different characteristics than other
> > > usual refcnts. Even when refcnt goes down to 0, anon_vma will not be freed.
> > > So, I think some kind of name as temporal_reference_count is better than
> > > simple "refcnt". Then, it will be clearer what this refcnt is for.
> > >
> >
> > When I read this in a few years, I'll have no idea what "temporal" is
> > referring to. The holder of this account is by a process that does not
> > necessarily own the page or its mappings but "remote" has special
> > meaning as well. "external_count" ?
> >
> "external" seems good. My selection of word is tend to be bad ;)
>
Trust me, it's not my strong point either.
> Off topic:
> But as Christoph says, make this as real reference counter as
> "if coutner goes down to 0, it's freed." may be good.
> I'm not fully aware of how anon_vma is copiled after Rik's anon_vma split(?)
> work. So, it may be complicated than I'm thinking.
>
The complexity is one factor. Rik's patches make it different for sure
because it's less clear what needs to be done with the chains. Even if
that is worked around, I'd have concerns about the refcount becoming a
highly contended cache line in some circumstances. I have taken note to
research it as a separate patch.
> > > > mm/rmap.c | 6 ++----
> > > > 4 files changed, 24 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > > > index 6b5a1a9..55c0e9e 100644
> > > > --- a/include/linux/rmap.h
> > > > +++ b/include/linux/rmap.h
> > > > @@ -26,11 +26,17 @@
> > > > */
> > > > struct anon_vma {
> > > > spinlock_t lock; /* Serialize access to vma list */
> > > > -#ifdef CONFIG_KSM
> > > > - atomic_t ksm_refcount;
> > > > -#endif
> > > > -#ifdef CONFIG_MIGRATION
> > > > - atomic_t migrate_refcount;
> > > > +#if defined(CONFIG_KSM) || defined(CONFIG_MIGRATION)
> > > > +
> > > > + /*
> > > > + * The refcount is taken by either KSM or page migration
> > > > + * to take a reference to an anon_vma when there is no
> > > > + * guarantee that the vma of page tables will exist for
> > > > + * the duration of the operation. A caller that takes
> > > > + * the reference is responsible for clearing up the
> > > > + * anon_vma if they are the last user on release
> > > > + */
> > > > + atomic_t refcount;
> > > > #endif
> > > > /*
> > > > * NOTE: the LSB of the head.next is set by
> > > > @@ -44,46 +50,26 @@ struct anon_vma {
> > > > };
> > > >
> > > > #ifdef CONFIG_MMU
> > > > -#ifdef CONFIG_KSM
> > > > -static inline void ksm_refcount_init(struct anon_vma *anon_vma)
> > > > +#if defined(CONFIG_KSM) || defined(CONFIG_MIGRATION)
> > > > +static inline void anonvma_refcount_init(struct anon_vma *anon_vma)
> > > > {
> > > > - atomic_set(&anon_vma->ksm_refcount, 0);
> > > > + atomic_set(&anon_vma->refcount, 0);
> > > > }
> > > >
> > > > -static inline int ksm_refcount(struct anon_vma *anon_vma)
> > > > +static inline int anonvma_refcount(struct anon_vma *anon_vma)
> > > > {
> > > > - return atomic_read(&anon_vma->ksm_refcount);
> > > > + return atomic_read(&anon_vma->refcount);
> > > > }
> > > > #else
> > > > -static inline void ksm_refcount_init(struct anon_vma *anon_vma)
> > > > +static inline void anonvma_refcount_init(struct anon_vma *anon_vma)
> > > > {
> > > > }
> > > >
> > > > -static inline int ksm_refcount(struct anon_vma *anon_vma)
> > > > +static inline int anonvma_refcount(struct anon_vma *anon_vma)
> > > > {
> > > > return 0;
> > > > }
> > > > #endif /* CONFIG_KSM */
> > > > -#ifdef CONFIG_MIGRATION
> > > > -static inline void migrate_refcount_init(struct anon_vma *anon_vma)
> > > > -{
> > > > - atomic_set(&anon_vma->migrate_refcount, 0);
> > > > -}
> > > > -
> > > > -static inline int migrate_refcount(struct anon_vma *anon_vma)
> > > > -{
> > > > - return atomic_read(&anon_vma->migrate_refcount);
> > > > -}
> > > > -#else
> > > > -static inline void migrate_refcount_init(struct anon_vma *anon_vma)
> > > > -{
> > > > -}
> > > > -
> > > > -static inline int migrate_refcount(struct anon_vma *anon_vma)
> > > > -{
> > > > - return 0;
> > > > -}
> > > > -#endif /* CONFIG_MIGRATE */
> > > >
> > > > static inline struct anon_vma *page_anon_vma(struct page *page)
> > > > {
> > > > diff --git a/mm/ksm.c b/mm/ksm.c
> > > > index 56a0da1..7decf73 100644
> > > > --- a/mm/ksm.c
> > > > +++ b/mm/ksm.c
> > > > @@ -318,14 +318,14 @@ static void hold_anon_vma(struct rmap_item *rmap_item,
> > > > struct anon_vma *anon_vma)
> > > > {
> > > > rmap_item->anon_vma = anon_vma;
> > > > - atomic_inc(&anon_vma->ksm_refcount);
> > > > + atomic_inc(&anon_vma->refcount);
> > > > }
> > > >
> > > > static void drop_anon_vma(struct rmap_item *rmap_item)
> > > > {
> > > > struct anon_vma *anon_vma = rmap_item->anon_vma;
> > > >
> > > > - if (atomic_dec_and_lock(&anon_vma->ksm_refcount, &anon_vma->lock)) {
> > > > + if (atomic_dec_and_lock(&anon_vma->refcount, &anon_vma->lock)) {
> > > > int empty = list_empty(&anon_vma->head);
> > > > spin_unlock(&anon_vma->lock);
> > > > if (empty)
> > > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > > index 1ce6a2f..00777b0 100644
> > > > --- a/mm/migrate.c
> > > > +++ b/mm/migrate.c
> > > > @@ -619,7 +619,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
> > > > rcu_read_lock();
> > > > rcu_locked = 1;
> > > > anon_vma = page_anon_vma(page);
> > > > - atomic_inc(&anon_vma->migrate_refcount);
> > > > + atomic_inc(&anon_vma->refcount);
> > > > }
> > > >
> > > > /*
> > > > @@ -661,7 +661,7 @@ skip_unmap:
> > > > rcu_unlock:
> > > >
> > > > /* Drop an anon_vma reference if we took one */
> > > > - if (anon_vma && atomic_dec_and_lock(&anon_vma->migrate_refcount, &anon_vma->lock)) {
> > > > + if (anon_vma && atomic_dec_and_lock(&anon_vma->refcount, &anon_vma->lock)) {
> > > > int empty = list_empty(&anon_vma->head);
> > > > spin_unlock(&anon_vma->lock);
> > > > if (empty)
> > > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > > index 11ba74a..96b5905 100644
> > > > --- a/mm/rmap.c
> > > > +++ b/mm/rmap.c
> > > > @@ -172,8 +172,7 @@ void anon_vma_unlink(struct vm_area_struct *vma)
> > > > list_del(&vma->anon_vma_node);
> > > >
> > > > /* We must garbage collect the anon_vma if it's empty */
> > > > - empty = list_empty(&anon_vma->head) && !ksm_refcount(anon_vma) &&
> > > > - !migrate_refcount(anon_vma);
> > > > + empty = list_empty(&anon_vma->head) && !anonvma_refcount(anon_vma);
> > > > spin_unlock(&anon_vma->lock);
> > > >
> > > > if (empty)
> > > > @@ -185,8 +184,7 @@ static void anon_vma_ctor(void *data)
> > > > struct anon_vma *anon_vma = data;
> > > >
> > > > spin_lock_init(&anon_vma->lock);
> > > > - ksm_refcount_init(anon_vma);
> > > > - migrate_refcount_init(anon_vma);
> > > > + anonvma_refcount_init(anon_vma);
> > > > INIT_LIST_HEAD(&anon_vma->head);
> > > > }
> > > >
> > > > --
> > > > 1.6.5
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at http://www.tux.org/lkml/
> > > >
> > >
> >
> > --
> > Mel Gorman
> > Part-time Phd Student Linux Technology Center
> > University of Limerick IBM Dublin Software Lab
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2010-02-20 9:33 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-02-18 18:02 [PATCH 0/12] Memory Compaction v3 Mel Gorman
2010-02-18 18:02 ` [PATCH 01/12] mm,migration: Take a reference to the anon_vma before migrating Mel Gorman
2010-02-19 0:12 ` KAMEZAWA Hiroyuki
2010-02-19 13:59 ` Mel Gorman
2010-02-19 16:43 ` Rik van Riel
2010-02-18 18:02 ` [PATCH 02/12] mm,migration: Do not try to migrate unmapped anonymous pages Mel Gorman
2010-02-19 16:45 ` Rik van Riel
2010-02-18 18:02 ` [PATCH 03/12] mm: Share the anon_vma ref counts between KSM and page migration Mel Gorman
2010-02-19 0:18 ` KAMEZAWA Hiroyuki
2010-02-19 14:05 ` Mel Gorman
2010-02-19 15:01 ` Christoph Lameter
2010-02-20 3:48 ` KAMEZAWA Hiroyuki
2010-02-20 9:32 ` Mel Gorman [this message]
2010-02-19 5:09 ` Minchan Kim
2010-02-19 21:42 ` Rik van Riel
2010-02-19 21:58 ` Mel Gorman
2010-02-20 0:16 ` Rik van Riel
2010-02-20 9:29 ` Mel Gorman
2010-02-18 18:02 ` [PATCH 04/12] mm: Document /proc/pagetypeinfo Mel Gorman
2010-02-19 1:36 ` Minchan Kim
2010-02-19 21:42 ` Rik van Riel
2010-02-18 18:02 ` [PATCH 05/12] Allow CONFIG_MIGRATION to be set without CONFIG_NUMA or memory hot-remove Mel Gorman
2010-02-19 0:21 ` KAMEZAWA Hiroyuki
2010-02-19 14:09 ` Mel Gorman
2010-02-18 18:02 ` [PATCH 06/12] Export unusable free space index via /proc/pagetypeinfo Mel Gorman
2010-02-19 1:35 ` Minchan Kim
2010-02-19 21:46 ` Rik van Riel
2010-02-18 18:02 ` [PATCH 07/12] Export fragmentation " Mel Gorman
2010-02-19 1:59 ` Minchan Kim
2010-02-20 0:16 ` Rik van Riel
2010-02-18 18:02 ` [PATCH 08/12] Memory compaction core Mel Gorman
2010-02-19 0:37 ` KAMEZAWA Hiroyuki
2010-02-19 14:15 ` Mel Gorman
2010-02-18 18:02 ` [PATCH 09/12] Add /proc trigger for memory compaction Mel Gorman
2010-02-19 0:43 ` KAMEZAWA Hiroyuki
2010-02-19 14:16 ` Mel Gorman
2010-02-20 3:53 ` KAMEZAWA Hiroyuki
2010-02-19 2:26 ` Minchan Kim
2010-02-18 18:02 ` [PATCH 10/12] Add /sys trigger for per-node " Mel Gorman
2010-02-19 14:53 ` Greg KH
2010-02-19 15:28 ` Mel Gorman
2010-02-19 15:31 ` Greg KH
2010-02-19 15:51 ` Mel Gorman
2010-02-19 15:52 ` Mel Gorman
2010-02-19 16:02 ` Greg KH
2010-02-18 18:02 ` [PATCH 11/12] Direct compact when a high-order allocation fails Mel Gorman
2010-02-19 0:51 ` KAMEZAWA Hiroyuki
2010-02-19 14:19 ` Mel Gorman
2010-02-19 2:41 ` Minchan Kim
2010-02-19 14:25 ` Mel Gorman
2010-02-18 18:02 ` [PATCH 12/12] Do not compact within a preferred zone after a compaction failure Mel Gorman
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=20100220093251.GI1445@csn.ul.ie \
--to=mel@csn.ul.ie \
--cc=aarcange@redhat.com \
--cc=agl@us.ibm.com \
--cc=avi@redhat.com \
--cc=cl@linux-foundation.org \
--cc=kamezawa.hiroyu@jp.fujitsu.com \
--cc=kosaki.motohiro@jp.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=riel@redhat.com \
--cc=rientjes@google.com \
/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).