Re: [PATCH 5/11] ksm: get_ksm_page locked
On Thu, Feb 07, 2013 at 04:33:58PM -0800, Hugh Dickins wrote:
> > >
> > > --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
> > > +++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
> > > @@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
> > > * but this is different - made simpler by ksm_thread_mutex being held,
> > > but
> > > * interesting for assuming that no other use of the struct page could
> > > ever
> > > * put our expected_mapping into page->mapping (or a field of the union
> > > which
> > > - * coincides with page->mapping). The RCU calls are not for KSM at all,
> > > but
> > > - * to keep the page_count protocol described with
> > > page_cache_get_speculative.
> > > + * coincides with page->mapping).
> > > *
> > > * Note: it is possible that get_ksm_page() will return NULL one moment,
> > > * then page the next, if the page is in between page_freeze_refs() and
> > > * page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
> > > * is on its way to being freed; but it is an anomaly to bear in mind.
> > > */
> > > -static struct page *get_ksm_page(struct stable_node *stable_node)
> > > +static struct page *get_ksm_page(struct stable_node *stable_node, bool
> > > locked)
> > > {
> >
> > The naming is unhelpful :(
> >
> > Because the second parameter is called "locked", it implies that the
> > caller of this function holds the page lock (which is obviously very
> > silly). ret_locked maybe?
>
> I'd prefer "lock_it": I'll make that change unless you've a better.
>
I don't.
> >
> > As the function is akin to find_lock_page I would prefer if there was
> > a new get_lock_ksm_page() instead of locking depending on the value of a
> > parameter.
>
> I demur. If it were a global interface rather than a function static
> to ksm.c, yes, I'm sure Linus would side very strongly with you, and I'd
> be providing a pair of wrappers to get_ksm_page() to hide the bool arg.
>
> But this is a private function (you're invited :) which doesn't need
> that level of hand-holding.
>
> And I'm a firm believer in having one, difficult, function where all
> the heavy thought is focussed, which does the nasty work and spares
> everywhere else from having to worry about the difficulties.
>
Ok, I'm convinced. As you say, the case for having one function is a lot
strong later in the series when this function becomes quite complex. Thanks.
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Thu, Feb 07, 2013 at 04:33:58PM -0800, Hugh Dickins wrote:
SNIP
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
+++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
@@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
* but this is different - made simpler by ksm_thread_mutex being held,
but
* interesting for assuming that no other use of the struct page could
ever
* put our expected_mapping into page-mapping (or a field of the union
which
- * coincides with page-mapping). The RCU calls are not for KSM at all,
but
- * to keep the page_count protocol described with
page_cache_get_speculative.
+ * coincides with page-mapping).
*
* Note: it is possible that get_ksm_page() will return NULL one moment,
* then page the next, if the page is in between page_freeze_refs() and
* page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
* is on its way to being freed; but it is an anomaly to bear in mind.
*/
-static struct page *get_ksm_page(struct stable_node *stable_node)
+static struct page *get_ksm_page(struct stable_node *stable_node, bool
locked)
{
The naming is unhelpful :(
Because the second parameter is called locked, it implies that the
caller of this function holds the page lock (which is obviously very
silly). ret_locked maybe?
I'd prefer lock_it: I'll make that change unless you've a better.
I don't.
As the function is akin to find_lock_page I would prefer if there was
a new get_lock_ksm_page() instead of locking depending on the value of a
parameter.
I demur. If it were a global interface rather than a function static
to ksm.c, yes, I'm sure Linus would side very strongly with you, and I'd
be providing a pair of wrappers to get_ksm_page() to hide the bool arg.
But this is a private function (you're invited :) which doesn't need
that level of hand-holding.
And I'm a firm believer in having one, difficult, function where all
the heavy thought is focussed, which does the nasty work and spares
everywhere else from having to worry about the difficulties.
Ok, I'm convinced. As you say, the case for having one function is a lot
strong later in the series when this function becomes quite complex. Thanks.
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Tue, 5 Feb 2013, Mel Gorman wrote:
> On Fri, Jan 25, 2013 at 06:00:50PM -0800, Hugh Dickins wrote:
> > In some places where get_ksm_page() is used, we need the page to be locked.
> >
> > When KSM migration is fully enabled, we shall want that to make sure that
> > the page just acquired cannot be migrated beneath us (raised page count is
> > only effective when there is serialization to make sure migration notices).
> > Whereas when navigating through the stable tree, we certainly do not want
> > to lock each node (raised page count is enough to guarantee the memcmps,
> > even if page is migrated to another node).
> >
> > Since we're about to add another use case, add the locked argument to
> > get_ksm_page() now.
> >
> > Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
> > really got the wrong end of the stick on that! There's a configuration
> > in which page_cache_get_speculative() can do something cheaper than
> > get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
> > disabled preemption for it. There's no need for rcu_read_lock() around
> > get_page_unless_zero() (and mapping checks) here. Cut out that
> > silliness before making this any harder to understand.
> >
> > Signed-off-by: Hugh Dickins
> > ---
> > mm/ksm.c | 23 +--
> > 1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
> > +++ mmotm/mm/ksm.c 2013-01-25 14:36:58.856206099 -0800
> > @@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
> > * but this is different - made simpler by ksm_thread_mutex being held, but
> > * interesting for assuming that no other use of the struct page could ever
> > * put our expected_mapping into page->mapping (or a field of the union
> > which
> > - * coincides with page->mapping). The RCU calls are not for KSM at all,
> > but
> > - * to keep the page_count protocol described with
> > page_cache_get_speculative.
> > + * coincides with page->mapping).
> > *
> > * Note: it is possible that get_ksm_page() will return NULL one moment,
> > * then page the next, if the page is in between page_freeze_refs() and
> > * page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
> > * is on its way to being freed; but it is an anomaly to bear in mind.
> > */
> > -static struct page *get_ksm_page(struct stable_node *stable_node)
> > +static struct page *get_ksm_page(struct stable_node *stable_node, bool
> > locked)
> > {
>
> The naming is unhelpful :(
>
> Because the second parameter is called "locked", it implies that the
> caller of this function holds the page lock (which is obviously very
> silly). ret_locked maybe?
I'd prefer "lock_it": I'll make that change unless you've a better.
>
> As the function is akin to find_lock_page I would prefer if there was
> a new get_lock_ksm_page() instead of locking depending on the value of a
> parameter.
I demur. If it were a global interface rather than a function static
to ksm.c, yes, I'm sure Linus would side very strongly with you, and I'd
be providing a pair of wrappers to get_ksm_page() to hide the bool arg.
But this is a private function (you're invited :) which doesn't need
that level of hand-holding.
And I'm a firm believer in having one, difficult, function where all
the heavy thought is focussed, which does the nasty work and spares
everywhere else from having to worry about the difficulties.
You'll shiver with horror as I recite shmem_getpage(_gfp),
page_lock_anon_vma(_read), page_relock_lruvec (well, that one did
not yet get beyond its posting): get_ksm_page is one of those.
> We can do this because expected_mapping is recorded by the
> stable_node and we only need to recalculate it if the page has been
> successfully pinned. We calculate the expected value twice but that's
> not earth shattering. It'd look something like;
>
> /*
> * get_lock_ksm_page: Similar to get_ksm_page except returns with page
> * locked and pinned
> */
> static struct page *get_lock_ksm_page(struct stable_node *stable_node)
> {
> struct page *page = get_ksm_page(stable_node);
>
> if (page) {
> expected_mapping = (void *)stable_node +
> (PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
> lock_page(page);
> if (page->mapping != expected_mapping) {
> unlock_page(page);
>
> /* release pin taken by get_ksm_page() */
> put_page(page);
> page = NULL;
> }
> }
>
> return page;
> }
Something like; but would also need the remove_node_from_stable_tree.
>
> Up to you, I'm not going to make a big deal of it.
Phew! Probably my insistence springs from knowing what this function
develops into a few patches later, rather than the simpler version
that appears at this stage of the series.
>
> FWIW, I agree that
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Tue, 5 Feb 2013, Mel Gorman wrote:
On Fri, Jan 25, 2013 at 06:00:50PM -0800, Hugh Dickins wrote:
In some places where get_ksm_page() is used, we need the page to be locked.
When KSM migration is fully enabled, we shall want that to make sure that
the page just acquired cannot be migrated beneath us (raised page count is
only effective when there is serialization to make sure migration notices).
Whereas when navigating through the stable tree, we certainly do not want
to lock each node (raised page count is enough to guarantee the memcmps,
even if page is migrated to another node).
Since we're about to add another use case, add the locked argument to
get_ksm_page() now.
Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
really got the wrong end of the stick on that! There's a configuration
in which page_cache_get_speculative() can do something cheaper than
get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
disabled preemption for it. There's no need for rcu_read_lock() around
get_page_unless_zero() (and mapping checks) here. Cut out that
silliness before making this any harder to understand.
Signed-off-by: Hugh Dickins hu...@google.com
---
mm/ksm.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
+++ mmotm/mm/ksm.c 2013-01-25 14:36:58.856206099 -0800
@@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
* but this is different - made simpler by ksm_thread_mutex being held, but
* interesting for assuming that no other use of the struct page could ever
* put our expected_mapping into page-mapping (or a field of the union
which
- * coincides with page-mapping). The RCU calls are not for KSM at all,
but
- * to keep the page_count protocol described with
page_cache_get_speculative.
+ * coincides with page-mapping).
*
* Note: it is possible that get_ksm_page() will return NULL one moment,
* then page the next, if the page is in between page_freeze_refs() and
* page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
* is on its way to being freed; but it is an anomaly to bear in mind.
*/
-static struct page *get_ksm_page(struct stable_node *stable_node)
+static struct page *get_ksm_page(struct stable_node *stable_node, bool
locked)
{
The naming is unhelpful :(
Because the second parameter is called locked, it implies that the
caller of this function holds the page lock (which is obviously very
silly). ret_locked maybe?
I'd prefer lock_it: I'll make that change unless you've a better.
As the function is akin to find_lock_page I would prefer if there was
a new get_lock_ksm_page() instead of locking depending on the value of a
parameter.
I demur. If it were a global interface rather than a function static
to ksm.c, yes, I'm sure Linus would side very strongly with you, and I'd
be providing a pair of wrappers to get_ksm_page() to hide the bool arg.
But this is a private function (you're invited :) which doesn't need
that level of hand-holding.
And I'm a firm believer in having one, difficult, function where all
the heavy thought is focussed, which does the nasty work and spares
everywhere else from having to worry about the difficulties.
You'll shiver with horror as I recite shmem_getpage(_gfp),
page_lock_anon_vma(_read), page_relock_lruvec (well, that one did
not yet get beyond its posting): get_ksm_page is one of those.
We can do this because expected_mapping is recorded by the
stable_node and we only need to recalculate it if the page has been
successfully pinned. We calculate the expected value twice but that's
not earth shattering. It'd look something like;
/*
* get_lock_ksm_page: Similar to get_ksm_page except returns with page
* locked and pinned
*/
static struct page *get_lock_ksm_page(struct stable_node *stable_node)
{
struct page *page = get_ksm_page(stable_node);
if (page) {
expected_mapping = (void *)stable_node +
(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
lock_page(page);
if (page-mapping != expected_mapping) {
unlock_page(page);
/* release pin taken by get_ksm_page() */
put_page(page);
page = NULL;
}
}
return page;
}
Something like; but would also need the remove_node_from_stable_tree.
Up to you, I'm not going to make a big deal of it.
Phew! Probably my insistence springs from knowing what this function
develops into a few patches later, rather than the simpler version
that appears at this stage of the series.
FWIW, I agree that removing rcu_read_lock() is fine.
Good, thanks, I was rather embarrassed by my misunderstanding there.
Hugh
--
To unsubscribe from
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Fri, Jan 25, 2013 at 06:00:50PM -0800, Hugh Dickins wrote:
> In some places where get_ksm_page() is used, we need the page to be locked.
>
> When KSM migration is fully enabled, we shall want that to make sure that
> the page just acquired cannot be migrated beneath us (raised page count is
> only effective when there is serialization to make sure migration notices).
> Whereas when navigating through the stable tree, we certainly do not want
> to lock each node (raised page count is enough to guarantee the memcmps,
> even if page is migrated to another node).
>
> Since we're about to add another use case, add the locked argument to
> get_ksm_page() now.
>
> Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
> really got the wrong end of the stick on that! There's a configuration
> in which page_cache_get_speculative() can do something cheaper than
> get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
> disabled preemption for it. There's no need for rcu_read_lock() around
> get_page_unless_zero() (and mapping checks) here. Cut out that
> silliness before making this any harder to understand.
>
> Signed-off-by: Hugh Dickins
> ---
> mm/ksm.c | 23 +--
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
> +++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
> @@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
> * but this is different - made simpler by ksm_thread_mutex being held, but
> * interesting for assuming that no other use of the struct page could ever
> * put our expected_mapping into page->mapping (or a field of the union which
> - * coincides with page->mapping). The RCU calls are not for KSM at all, but
> - * to keep the page_count protocol described with page_cache_get_speculative.
> + * coincides with page->mapping).
> *
> * Note: it is possible that get_ksm_page() will return NULL one moment,
> * then page the next, if the page is in between page_freeze_refs() and
> * page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
> * is on its way to being freed; but it is an anomaly to bear in mind.
> */
> -static struct page *get_ksm_page(struct stable_node *stable_node)
> +static struct page *get_ksm_page(struct stable_node *stable_node, bool
> locked)
> {
The naming is unhelpful :(
Because the second parameter is called "locked", it implies that the
caller of this function holds the page lock (which is obviously very
silly). ret_locked maybe?
As the function is akin to find_lock_page I would prefer if there was
a new get_lock_ksm_page() instead of locking depending on the value of a
parameter. We can do this because expected_mapping is recorded by the
stable_node and we only need to recalculate it if the page has been
successfully pinned. We calculate the expected value twice but that's
not earth shattering. It'd look something like;
/*
* get_lock_ksm_page: Similar to get_ksm_page except returns with page
* locked and pinned
*/
static struct page *get_lock_ksm_page(struct stable_node *stable_node)
{
struct page *page = get_ksm_page(stable_node);
if (page) {
expected_mapping = (void *)stable_node +
(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
lock_page(page);
if (page->mapping != expected_mapping) {
unlock_page(page);
/* release pin taken by get_ksm_page() */
put_page(page);
page = NULL;
}
}
return page;
}
Up to you, I'm not going to make a big deal of it.
FWIW, I agree that removing rcu_read_lock() is fine.
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Fri, Jan 25, 2013 at 06:00:50PM -0800, Hugh Dickins wrote:
In some places where get_ksm_page() is used, we need the page to be locked.
When KSM migration is fully enabled, we shall want that to make sure that
the page just acquired cannot be migrated beneath us (raised page count is
only effective when there is serialization to make sure migration notices).
Whereas when navigating through the stable tree, we certainly do not want
to lock each node (raised page count is enough to guarantee the memcmps,
even if page is migrated to another node).
Since we're about to add another use case, add the locked argument to
get_ksm_page() now.
Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
really got the wrong end of the stick on that! There's a configuration
in which page_cache_get_speculative() can do something cheaper than
get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
disabled preemption for it. There's no need for rcu_read_lock() around
get_page_unless_zero() (and mapping checks) here. Cut out that
silliness before making this any harder to understand.
Signed-off-by: Hugh Dickins hu...@google.com
---
mm/ksm.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
+++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
@@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
* but this is different - made simpler by ksm_thread_mutex being held, but
* interesting for assuming that no other use of the struct page could ever
* put our expected_mapping into page-mapping (or a field of the union which
- * coincides with page-mapping). The RCU calls are not for KSM at all, but
- * to keep the page_count protocol described with page_cache_get_speculative.
+ * coincides with page-mapping).
*
* Note: it is possible that get_ksm_page() will return NULL one moment,
* then page the next, if the page is in between page_freeze_refs() and
* page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
* is on its way to being freed; but it is an anomaly to bear in mind.
*/
-static struct page *get_ksm_page(struct stable_node *stable_node)
+static struct page *get_ksm_page(struct stable_node *stable_node, bool
locked)
{
The naming is unhelpful :(
Because the second parameter is called locked, it implies that the
caller of this function holds the page lock (which is obviously very
silly). ret_locked maybe?
As the function is akin to find_lock_page I would prefer if there was
a new get_lock_ksm_page() instead of locking depending on the value of a
parameter. We can do this because expected_mapping is recorded by the
stable_node and we only need to recalculate it if the page has been
successfully pinned. We calculate the expected value twice but that's
not earth shattering. It'd look something like;
/*
* get_lock_ksm_page: Similar to get_ksm_page except returns with page
* locked and pinned
*/
static struct page *get_lock_ksm_page(struct stable_node *stable_node)
{
struct page *page = get_ksm_page(stable_node);
if (page) {
expected_mapping = (void *)stable_node +
(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
lock_page(page);
if (page-mapping != expected_mapping) {
unlock_page(page);
/* release pin taken by get_ksm_page() */
put_page(page);
page = NULL;
}
}
return page;
}
Up to you, I'm not going to make a big deal of it.
FWIW, I agree that removing rcu_read_lock() is fine.
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sun, 27 Jan 2013, Simon Jeons wrote: > On Sun, 2013-01-27 at 14:08 -0800, Hugh Dickins wrote: > > On Sat, 26 Jan 2013, Simon Jeons wrote: > > > > > > Why the parameter lock passed from stable_tree_search/insert is true, > > > but remove_rmap_item_from_tree is false? > > > > The other way round? remove_rmap_item_from_tree needs the page locked, > > because it's about to modify the list: that's secured (e.g. against > > concurrent KSM page reclaim) by the page lock. > > How can KSM page reclaim path call remove_rmap_item_from_tree? I have > already track every callsites but can't find it. It doesn't. Please read what I said above again. > BTW, I'm curious about > KSM page reclaim, it seems that there're no special handle in vmscan.c > for KSM page reclaim, is it will be reclaimed similiar with normal > page? Look for PageKsm in mm/rmap.c. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sun, 2013-01-27 at 14:08 -0800, Hugh Dickins wrote: > On Sat, 26 Jan 2013, Simon Jeons wrote: > > On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote: > > > In some places where get_ksm_page() is used, we need the page to be > > > locked. > > > > > > > In function get_ksm_page, why check page->mapping => > > get_page_unless_zero => check page->mapping instead of > > get_page_unless_zero => check page->mapping, because > > get_page_unless_zero is expensive? > > Yes, it's more expensive. > > > > > > When KSM migration is fully enabled, we shall want that to make sure that > > > the page just acquired cannot be migrated beneath us (raised page count is > > > only effective when there is serialization to make sure migration > > > notices). > > > Whereas when navigating through the stable tree, we certainly do not want > > > > What's the meaning of "navigating through the stable tree"? > > Finding the right place in the stable tree, > as stable_tree_search() and stable_tree_insert() do. > > > > > > to lock each node (raised page count is enough to guarantee the memcmps, > > > even if page is migrated to another node). > > > > > > Since we're about to add another use case, add the locked argument to > > > get_ksm_page() now. > > > > Why the parameter lock passed from stable_tree_search/insert is true, > > but remove_rmap_item_from_tree is false? > > The other way round? remove_rmap_item_from_tree needs the page locked, > because it's about to modify the list: that's secured (e.g. against > concurrent KSM page reclaim) by the page lock. How can KSM page reclaim path call remove_rmap_item_from_tree? I have already track every callsites but can't find it. BTW, I'm curious about KSM page reclaim, it seems that there're no special handle in vmscan.c for KSM page reclaim, is it will be reclaimed similiar with normal page? > > stable_tree_search and stable_tree_insert do not need intermediate nodes > to be locked: get_page is enough to secure the page contents for memcmp, > and we don't want a pointless wait for exclusive page lock on every > intermediate node. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sat, 26 Jan 2013, Simon Jeons wrote: > > BTW, what's the meaning of ksm page forked? A ksm page is mapped into a process's mm, then that process calls fork(): the ksm page then appears in the child's mm, before ksmd has tracked it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sat, 26 Jan 2013, Simon Jeons wrote: > On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote: > > In some places where get_ksm_page() is used, we need the page to be locked. > > > > In function get_ksm_page, why check page->mapping => > get_page_unless_zero => check page->mapping instead of > get_page_unless_zero => check page->mapping, because > get_page_unless_zero is expensive? Yes, it's more expensive. > > > When KSM migration is fully enabled, we shall want that to make sure that > > the page just acquired cannot be migrated beneath us (raised page count is > > only effective when there is serialization to make sure migration notices). > > Whereas when navigating through the stable tree, we certainly do not want > > What's the meaning of "navigating through the stable tree"? Finding the right place in the stable tree, as stable_tree_search() and stable_tree_insert() do. > > > to lock each node (raised page count is enough to guarantee the memcmps, > > even if page is migrated to another node). > > > > Since we're about to add another use case, add the locked argument to > > get_ksm_page() now. > > Why the parameter lock passed from stable_tree_search/insert is true, > but remove_rmap_item_from_tree is false? The other way round? remove_rmap_item_from_tree needs the page locked, because it's about to modify the list: that's secured (e.g. against concurrent KSM page reclaim) by the page lock. stable_tree_search and stable_tree_insert do not need intermediate nodes to be locked: get_page is enough to secure the page contents for memcmp, and we don't want a pointless wait for exclusive page lock on every intermediate node. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sat, 26 Jan 2013, Simon Jeons wrote: On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote: In some places where get_ksm_page() is used, we need the page to be locked. In function get_ksm_page, why check page-mapping = get_page_unless_zero = check page-mapping instead of get_page_unless_zero = check page-mapping, because get_page_unless_zero is expensive? Yes, it's more expensive. When KSM migration is fully enabled, we shall want that to make sure that the page just acquired cannot be migrated beneath us (raised page count is only effective when there is serialization to make sure migration notices). Whereas when navigating through the stable tree, we certainly do not want What's the meaning of navigating through the stable tree? Finding the right place in the stable tree, as stable_tree_search() and stable_tree_insert() do. to lock each node (raised page count is enough to guarantee the memcmps, even if page is migrated to another node). Since we're about to add another use case, add the locked argument to get_ksm_page() now. Why the parameter lock passed from stable_tree_search/insert is true, but remove_rmap_item_from_tree is false? The other way round? remove_rmap_item_from_tree needs the page locked, because it's about to modify the list: that's secured (e.g. against concurrent KSM page reclaim) by the page lock. stable_tree_search and stable_tree_insert do not need intermediate nodes to be locked: get_page is enough to secure the page contents for memcmp, and we don't want a pointless wait for exclusive page lock on every intermediate node. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sat, 26 Jan 2013, Simon Jeons wrote: BTW, what's the meaning of ksm page forked? A ksm page is mapped into a process's mm, then that process calls fork(): the ksm page then appears in the child's mm, before ksmd has tracked it. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sun, 2013-01-27 at 14:08 -0800, Hugh Dickins wrote: On Sat, 26 Jan 2013, Simon Jeons wrote: On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote: In some places where get_ksm_page() is used, we need the page to be locked. In function get_ksm_page, why check page-mapping = get_page_unless_zero = check page-mapping instead of get_page_unless_zero = check page-mapping, because get_page_unless_zero is expensive? Yes, it's more expensive. When KSM migration is fully enabled, we shall want that to make sure that the page just acquired cannot be migrated beneath us (raised page count is only effective when there is serialization to make sure migration notices). Whereas when navigating through the stable tree, we certainly do not want What's the meaning of navigating through the stable tree? Finding the right place in the stable tree, as stable_tree_search() and stable_tree_insert() do. to lock each node (raised page count is enough to guarantee the memcmps, even if page is migrated to another node). Since we're about to add another use case, add the locked argument to get_ksm_page() now. Why the parameter lock passed from stable_tree_search/insert is true, but remove_rmap_item_from_tree is false? The other way round? remove_rmap_item_from_tree needs the page locked, because it's about to modify the list: that's secured (e.g. against concurrent KSM page reclaim) by the page lock. How can KSM page reclaim path call remove_rmap_item_from_tree? I have already track every callsites but can't find it. BTW, I'm curious about KSM page reclaim, it seems that there're no special handle in vmscan.c for KSM page reclaim, is it will be reclaimed similiar with normal page? stable_tree_search and stable_tree_insert do not need intermediate nodes to be locked: get_page is enough to secure the page contents for memcmp, and we don't want a pointless wait for exclusive page lock on every intermediate node. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Sun, 27 Jan 2013, Simon Jeons wrote: On Sun, 2013-01-27 at 14:08 -0800, Hugh Dickins wrote: On Sat, 26 Jan 2013, Simon Jeons wrote: Why the parameter lock passed from stable_tree_search/insert is true, but remove_rmap_item_from_tree is false? The other way round? remove_rmap_item_from_tree needs the page locked, because it's about to modify the list: that's secured (e.g. against concurrent KSM page reclaim) by the page lock. How can KSM page reclaim path call remove_rmap_item_from_tree? I have already track every callsites but can't find it. It doesn't. Please read what I said above again. BTW, I'm curious about KSM page reclaim, it seems that there're no special handle in vmscan.c for KSM page reclaim, is it will be reclaimed similiar with normal page? Look for PageKsm in mm/rmap.c. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote:
> In some places where get_ksm_page() is used, we need the page to be locked.
>
> When KSM migration is fully enabled, we shall want that to make sure that
> the page just acquired cannot be migrated beneath us (raised page count is
> only effective when there is serialization to make sure migration notices).
> Whereas when navigating through the stable tree, we certainly do not want
> to lock each node (raised page count is enough to guarantee the memcmps,
> even if page is migrated to another node).
>
> Since we're about to add another use case, add the locked argument to
> get_ksm_page() now.
>
> Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
> really got the wrong end of the stick on that! There's a configuration
> in which page_cache_get_speculative() can do something cheaper than
> get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
> disabled preemption for it. There's no need for rcu_read_lock() around
> get_page_unless_zero() (and mapping checks) here. Cut out that
> silliness before making this any harder to understand.
BTW, what's the meaning of ksm page forked?
>
> Signed-off-by: Hugh Dickins
> ---
> mm/ksm.c | 23 +--
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
> +++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
> @@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
> * but this is different - made simpler by ksm_thread_mutex being held, but
> * interesting for assuming that no other use of the struct page could ever
> * put our expected_mapping into page->mapping (or a field of the union which
> - * coincides with page->mapping). The RCU calls are not for KSM at all, but
> - * to keep the page_count protocol described with page_cache_get_speculative.
> + * coincides with page->mapping).
> *
> * Note: it is possible that get_ksm_page() will return NULL one moment,
> * then page the next, if the page is in between page_freeze_refs() and
> * page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
> * is on its way to being freed; but it is an anomaly to bear in mind.
> */
> -static struct page *get_ksm_page(struct stable_node *stable_node)
> +static struct page *get_ksm_page(struct stable_node *stable_node, bool
> locked)
> {
> struct page *page;
> void *expected_mapping;
> @@ -530,7 +529,6 @@ static struct page *get_ksm_page(struct
> page = pfn_to_page(stable_node->kpfn);
> expected_mapping = (void *)stable_node +
> (PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
> - rcu_read_lock();
> if (page->mapping != expected_mapping)
> goto stale;
> if (!get_page_unless_zero(page))
> @@ -539,10 +537,16 @@ static struct page *get_ksm_page(struct
> put_page(page);
> goto stale;
> }
> - rcu_read_unlock();
> + if (locked) {
> + lock_page(page);
> + if (page->mapping != expected_mapping) {
> + unlock_page(page);
> + put_page(page);
> + goto stale;
> + }
> + }
> return page;
> stale:
> - rcu_read_unlock();
> remove_node_from_stable_tree(stable_node);
> return NULL;
> }
> @@ -558,11 +562,10 @@ static void remove_rmap_item_from_tree(s
> struct page *page;
>
> stable_node = rmap_item->head;
> - page = get_ksm_page(stable_node);
> + page = get_ksm_page(stable_node, true);
> if (!page)
> goto out;
>
> - lock_page(page);
> hlist_del(_item->hlist);
> unlock_page(page);
> put_page(page);
> @@ -1042,7 +1045,7 @@ static struct page *stable_tree_search(s
>
> cond_resched();
> stable_node = rb_entry(node, struct stable_node, node);
> - tree_page = get_ksm_page(stable_node);
> + tree_page = get_ksm_page(stable_node, false);
> if (!tree_page)
> return NULL;
>
> @@ -1086,7 +1089,7 @@ static struct stable_node *stable_tree_i
>
> cond_resched();
> stable_node = rb_entry(*new, struct stable_node, node);
> - tree_page = get_ksm_page(stable_node);
> + tree_page = get_ksm_page(stable_node, false);
> if (!tree_page)
> return NULL;
>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH 5/11] ksm: get_ksm_page locked
Hi Hugh,
On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote:
> In some places where get_ksm_page() is used, we need the page to be locked.
>
In function get_ksm_page, why check page->mapping =>
get_page_unless_zero => check page->mapping instead of
get_page_unless_zero => check page->mapping, because
get_page_unless_zero is expensive?
> When KSM migration is fully enabled, we shall want that to make sure that
> the page just acquired cannot be migrated beneath us (raised page count is
> only effective when there is serialization to make sure migration notices).
> Whereas when navigating through the stable tree, we certainly do not want
What's the meaning of "navigating through the stable tree"?
> to lock each node (raised page count is enough to guarantee the memcmps,
> even if page is migrated to another node).
>
> Since we're about to add another use case, add the locked argument to
> get_ksm_page() now.
Why the parameter lock passed from stable_tree_search/insert is true,
but remove_rmap_item_from_tree is false?
>
> Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
> really got the wrong end of the stick on that! There's a configuration
> in which page_cache_get_speculative() can do something cheaper than
> get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
> disabled preemption for it. There's no need for rcu_read_lock() around
> get_page_unless_zero() (and mapping checks) here. Cut out that
> silliness before making this any harder to understand.
>
> Signed-off-by: Hugh Dickins
> ---
> mm/ksm.c | 23 +--
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
> +++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
> @@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
> * but this is different - made simpler by ksm_thread_mutex being held, but
> * interesting for assuming that no other use of the struct page could ever
> * put our expected_mapping into page->mapping (or a field of the union which
> - * coincides with page->mapping). The RCU calls are not for KSM at all, but
> - * to keep the page_count protocol described with page_cache_get_speculative.
> + * coincides with page->mapping).
> *
> * Note: it is possible that get_ksm_page() will return NULL one moment,
> * then page the next, if the page is in between page_freeze_refs() and
> * page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
> * is on its way to being freed; but it is an anomaly to bear in mind.
> */
> -static struct page *get_ksm_page(struct stable_node *stable_node)
> +static struct page *get_ksm_page(struct stable_node *stable_node, bool
> locked)
> {
> struct page *page;
> void *expected_mapping;
> @@ -530,7 +529,6 @@ static struct page *get_ksm_page(struct
> page = pfn_to_page(stable_node->kpfn);
> expected_mapping = (void *)stable_node +
> (PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
> - rcu_read_lock();
> if (page->mapping != expected_mapping)
> goto stale;
> if (!get_page_unless_zero(page))
> @@ -539,10 +537,16 @@ static struct page *get_ksm_page(struct
> put_page(page);
> goto stale;
> }
> - rcu_read_unlock();
> + if (locked) {
> + lock_page(page);
> + if (page->mapping != expected_mapping) {
> + unlock_page(page);
> + put_page(page);
> + goto stale;
> + }
> + }
> return page;
> stale:
> - rcu_read_unlock();
> remove_node_from_stable_tree(stable_node);
> return NULL;
> }
> @@ -558,11 +562,10 @@ static void remove_rmap_item_from_tree(s
> struct page *page;
>
> stable_node = rmap_item->head;
> - page = get_ksm_page(stable_node);
> + page = get_ksm_page(stable_node, true);
> if (!page)
> goto out;
>
> - lock_page(page);
> hlist_del(_item->hlist);
> unlock_page(page);
> put_page(page);
> @@ -1042,7 +1045,7 @@ static struct page *stable_tree_search(s
>
> cond_resched();
> stable_node = rb_entry(node, struct stable_node, node);
> - tree_page = get_ksm_page(stable_node);
> + tree_page = get_ksm_page(stable_node, false);
> if (!tree_page)
> return NULL;
>
> @@ -1086,7 +1089,7 @@ static struct stable_node *stable_tree_i
>
> cond_resched();
> stable_node = rb_entry(*new, struct stable_node, node);
> - tree_page = get_ksm_page(stable_node);
> + tree_page = get_ksm_page(stable_node, false);
> if (!tree_page)
> return NULL;
>
>
> --
> To unsubscribe, send a message with
Re: [PATCH 5/11] ksm: get_ksm_page locked
Hi Hugh,
On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote:
In some places where get_ksm_page() is used, we need the page to be locked.
In function get_ksm_page, why check page-mapping =
get_page_unless_zero = check page-mapping instead of
get_page_unless_zero = check page-mapping, because
get_page_unless_zero is expensive?
When KSM migration is fully enabled, we shall want that to make sure that
the page just acquired cannot be migrated beneath us (raised page count is
only effective when there is serialization to make sure migration notices).
Whereas when navigating through the stable tree, we certainly do not want
What's the meaning of navigating through the stable tree?
to lock each node (raised page count is enough to guarantee the memcmps,
even if page is migrated to another node).
Since we're about to add another use case, add the locked argument to
get_ksm_page() now.
Why the parameter lock passed from stable_tree_search/insert is true,
but remove_rmap_item_from_tree is false?
Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
really got the wrong end of the stick on that! There's a configuration
in which page_cache_get_speculative() can do something cheaper than
get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
disabled preemption for it. There's no need for rcu_read_lock() around
get_page_unless_zero() (and mapping checks) here. Cut out that
silliness before making this any harder to understand.
Signed-off-by: Hugh Dickins hu...@google.com
---
mm/ksm.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
+++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
@@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
* but this is different - made simpler by ksm_thread_mutex being held, but
* interesting for assuming that no other use of the struct page could ever
* put our expected_mapping into page-mapping (or a field of the union which
- * coincides with page-mapping). The RCU calls are not for KSM at all, but
- * to keep the page_count protocol described with page_cache_get_speculative.
+ * coincides with page-mapping).
*
* Note: it is possible that get_ksm_page() will return NULL one moment,
* then page the next, if the page is in between page_freeze_refs() and
* page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
* is on its way to being freed; but it is an anomaly to bear in mind.
*/
-static struct page *get_ksm_page(struct stable_node *stable_node)
+static struct page *get_ksm_page(struct stable_node *stable_node, bool
locked)
{
struct page *page;
void *expected_mapping;
@@ -530,7 +529,6 @@ static struct page *get_ksm_page(struct
page = pfn_to_page(stable_node-kpfn);
expected_mapping = (void *)stable_node +
(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
- rcu_read_lock();
if (page-mapping != expected_mapping)
goto stale;
if (!get_page_unless_zero(page))
@@ -539,10 +537,16 @@ static struct page *get_ksm_page(struct
put_page(page);
goto stale;
}
- rcu_read_unlock();
+ if (locked) {
+ lock_page(page);
+ if (page-mapping != expected_mapping) {
+ unlock_page(page);
+ put_page(page);
+ goto stale;
+ }
+ }
return page;
stale:
- rcu_read_unlock();
remove_node_from_stable_tree(stable_node);
return NULL;
}
@@ -558,11 +562,10 @@ static void remove_rmap_item_from_tree(s
struct page *page;
stable_node = rmap_item-head;
- page = get_ksm_page(stable_node);
+ page = get_ksm_page(stable_node, true);
if (!page)
goto out;
- lock_page(page);
hlist_del(rmap_item-hlist);
unlock_page(page);
put_page(page);
@@ -1042,7 +1045,7 @@ static struct page *stable_tree_search(s
cond_resched();
stable_node = rb_entry(node, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
@@ -1086,7 +1089,7 @@ static struct stable_node *stable_tree_i
cond_resched();
stable_node = rb_entry(*new, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majord...@kvack.org. For more info on Linux MM,
see:
Re: [PATCH 5/11] ksm: get_ksm_page locked
On Fri, 2013-01-25 at 18:00 -0800, Hugh Dickins wrote:
In some places where get_ksm_page() is used, we need the page to be locked.
When KSM migration is fully enabled, we shall want that to make sure that
the page just acquired cannot be migrated beneath us (raised page count is
only effective when there is serialization to make sure migration notices).
Whereas when navigating through the stable tree, we certainly do not want
to lock each node (raised page count is enough to guarantee the memcmps,
even if page is migrated to another node).
Since we're about to add another use case, add the locked argument to
get_ksm_page() now.
Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
really got the wrong end of the stick on that! There's a configuration
in which page_cache_get_speculative() can do something cheaper than
get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
disabled preemption for it. There's no need for rcu_read_lock() around
get_page_unless_zero() (and mapping checks) here. Cut out that
silliness before making this any harder to understand.
BTW, what's the meaning of ksm page forked?
Signed-off-by: Hugh Dickins hu...@google.com
---
mm/ksm.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
+++ mmotm/mm/ksm.c2013-01-25 14:36:58.856206099 -0800
@@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
* but this is different - made simpler by ksm_thread_mutex being held, but
* interesting for assuming that no other use of the struct page could ever
* put our expected_mapping into page-mapping (or a field of the union which
- * coincides with page-mapping). The RCU calls are not for KSM at all, but
- * to keep the page_count protocol described with page_cache_get_speculative.
+ * coincides with page-mapping).
*
* Note: it is possible that get_ksm_page() will return NULL one moment,
* then page the next, if the page is in between page_freeze_refs() and
* page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
* is on its way to being freed; but it is an anomaly to bear in mind.
*/
-static struct page *get_ksm_page(struct stable_node *stable_node)
+static struct page *get_ksm_page(struct stable_node *stable_node, bool
locked)
{
struct page *page;
void *expected_mapping;
@@ -530,7 +529,6 @@ static struct page *get_ksm_page(struct
page = pfn_to_page(stable_node-kpfn);
expected_mapping = (void *)stable_node +
(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
- rcu_read_lock();
if (page-mapping != expected_mapping)
goto stale;
if (!get_page_unless_zero(page))
@@ -539,10 +537,16 @@ static struct page *get_ksm_page(struct
put_page(page);
goto stale;
}
- rcu_read_unlock();
+ if (locked) {
+ lock_page(page);
+ if (page-mapping != expected_mapping) {
+ unlock_page(page);
+ put_page(page);
+ goto stale;
+ }
+ }
return page;
stale:
- rcu_read_unlock();
remove_node_from_stable_tree(stable_node);
return NULL;
}
@@ -558,11 +562,10 @@ static void remove_rmap_item_from_tree(s
struct page *page;
stable_node = rmap_item-head;
- page = get_ksm_page(stable_node);
+ page = get_ksm_page(stable_node, true);
if (!page)
goto out;
- lock_page(page);
hlist_del(rmap_item-hlist);
unlock_page(page);
put_page(page);
@@ -1042,7 +1045,7 @@ static struct page *stable_tree_search(s
cond_resched();
stable_node = rb_entry(node, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
@@ -1086,7 +1089,7 @@ static struct stable_node *stable_tree_i
cond_resched();
stable_node = rb_entry(*new, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majord...@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 5/11] ksm: get_ksm_page locked
In some places where get_ksm_page() is used, we need the page to be locked.
When KSM migration is fully enabled, we shall want that to make sure that
the page just acquired cannot be migrated beneath us (raised page count is
only effective when there is serialization to make sure migration notices).
Whereas when navigating through the stable tree, we certainly do not want
to lock each node (raised page count is enough to guarantee the memcmps,
even if page is migrated to another node).
Since we're about to add another use case, add the locked argument to
get_ksm_page() now.
Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
really got the wrong end of the stick on that! There's a configuration
in which page_cache_get_speculative() can do something cheaper than
get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
disabled preemption for it. There's no need for rcu_read_lock() around
get_page_unless_zero() (and mapping checks) here. Cut out that
silliness before making this any harder to understand.
Signed-off-by: Hugh Dickins
---
mm/ksm.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
+++ mmotm/mm/ksm.c 2013-01-25 14:36:58.856206099 -0800
@@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
* but this is different - made simpler by ksm_thread_mutex being held, but
* interesting for assuming that no other use of the struct page could ever
* put our expected_mapping into page->mapping (or a field of the union which
- * coincides with page->mapping). The RCU calls are not for KSM at all, but
- * to keep the page_count protocol described with page_cache_get_speculative.
+ * coincides with page->mapping).
*
* Note: it is possible that get_ksm_page() will return NULL one moment,
* then page the next, if the page is in between page_freeze_refs() and
* page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
* is on its way to being freed; but it is an anomaly to bear in mind.
*/
-static struct page *get_ksm_page(struct stable_node *stable_node)
+static struct page *get_ksm_page(struct stable_node *stable_node, bool locked)
{
struct page *page;
void *expected_mapping;
@@ -530,7 +529,6 @@ static struct page *get_ksm_page(struct
page = pfn_to_page(stable_node->kpfn);
expected_mapping = (void *)stable_node +
(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
- rcu_read_lock();
if (page->mapping != expected_mapping)
goto stale;
if (!get_page_unless_zero(page))
@@ -539,10 +537,16 @@ static struct page *get_ksm_page(struct
put_page(page);
goto stale;
}
- rcu_read_unlock();
+ if (locked) {
+ lock_page(page);
+ if (page->mapping != expected_mapping) {
+ unlock_page(page);
+ put_page(page);
+ goto stale;
+ }
+ }
return page;
stale:
- rcu_read_unlock();
remove_node_from_stable_tree(stable_node);
return NULL;
}
@@ -558,11 +562,10 @@ static void remove_rmap_item_from_tree(s
struct page *page;
stable_node = rmap_item->head;
- page = get_ksm_page(stable_node);
+ page = get_ksm_page(stable_node, true);
if (!page)
goto out;
- lock_page(page);
hlist_del(_item->hlist);
unlock_page(page);
put_page(page);
@@ -1042,7 +1045,7 @@ static struct page *stable_tree_search(s
cond_resched();
stable_node = rb_entry(node, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
@@ -1086,7 +1089,7 @@ static struct stable_node *stable_tree_i
cond_resched();
stable_node = rb_entry(*new, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 5/11] ksm: get_ksm_page locked
In some places where get_ksm_page() is used, we need the page to be locked.
When KSM migration is fully enabled, we shall want that to make sure that
the page just acquired cannot be migrated beneath us (raised page count is
only effective when there is serialization to make sure migration notices).
Whereas when navigating through the stable tree, we certainly do not want
to lock each node (raised page count is enough to guarantee the memcmps,
even if page is migrated to another node).
Since we're about to add another use case, add the locked argument to
get_ksm_page() now.
Hmm, what's that rcu_read_lock() about? Complete misunderstanding, I
really got the wrong end of the stick on that! There's a configuration
in which page_cache_get_speculative() can do something cheaper than
get_page_unless_zero(), relying on its caller's rcu_read_lock() to have
disabled preemption for it. There's no need for rcu_read_lock() around
get_page_unless_zero() (and mapping checks) here. Cut out that
silliness before making this any harder to understand.
Signed-off-by: Hugh Dickins hu...@google.com
---
mm/ksm.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
+++ mmotm/mm/ksm.c 2013-01-25 14:36:58.856206099 -0800
@@ -514,15 +514,14 @@ static void remove_node_from_stable_tree
* but this is different - made simpler by ksm_thread_mutex being held, but
* interesting for assuming that no other use of the struct page could ever
* put our expected_mapping into page-mapping (or a field of the union which
- * coincides with page-mapping). The RCU calls are not for KSM at all, but
- * to keep the page_count protocol described with page_cache_get_speculative.
+ * coincides with page-mapping).
*
* Note: it is possible that get_ksm_page() will return NULL one moment,
* then page the next, if the page is in between page_freeze_refs() and
* page_unfreeze_refs(): this shouldn't be a problem anywhere, the page
* is on its way to being freed; but it is an anomaly to bear in mind.
*/
-static struct page *get_ksm_page(struct stable_node *stable_node)
+static struct page *get_ksm_page(struct stable_node *stable_node, bool locked)
{
struct page *page;
void *expected_mapping;
@@ -530,7 +529,6 @@ static struct page *get_ksm_page(struct
page = pfn_to_page(stable_node-kpfn);
expected_mapping = (void *)stable_node +
(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
- rcu_read_lock();
if (page-mapping != expected_mapping)
goto stale;
if (!get_page_unless_zero(page))
@@ -539,10 +537,16 @@ static struct page *get_ksm_page(struct
put_page(page);
goto stale;
}
- rcu_read_unlock();
+ if (locked) {
+ lock_page(page);
+ if (page-mapping != expected_mapping) {
+ unlock_page(page);
+ put_page(page);
+ goto stale;
+ }
+ }
return page;
stale:
- rcu_read_unlock();
remove_node_from_stable_tree(stable_node);
return NULL;
}
@@ -558,11 +562,10 @@ static void remove_rmap_item_from_tree(s
struct page *page;
stable_node = rmap_item-head;
- page = get_ksm_page(stable_node);
+ page = get_ksm_page(stable_node, true);
if (!page)
goto out;
- lock_page(page);
hlist_del(rmap_item-hlist);
unlock_page(page);
put_page(page);
@@ -1042,7 +1045,7 @@ static struct page *stable_tree_search(s
cond_resched();
stable_node = rb_entry(node, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
@@ -1086,7 +1089,7 @@ static struct stable_node *stable_tree_i
cond_resched();
stable_node = rb_entry(*new, struct stable_node, node);
- tree_page = get_ksm_page(stable_node);
+ tree_page = get_ksm_page(stable_node, false);
if (!tree_page)
return NULL;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
