Re: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
Dave Hansen wrote:
> On 06/07/2013 08:10 AM, Kirill A. Shutemov wrote:
> > + /*
> > +* When we add a huge page to page cache we take only reference to head
> > +* page, but on split we need to take addition reference to all tail
> > +* pages since they are still in page cache after splitting.
> > +*/
> > + init_tail_refcount = PageAnon(page) ? 0 : 1;
>
> What's the "init" for in the name?
initial_tail_refcount?
> In add_to_page_cache_locked() in patch 12/39, you do
> > + spin_lock_irq(>tree_lock);
> > + for (i = 0; i < nr; i++) {
> > + page_cache_get(page + i);
>
> That looks to me to be taking references to the tail pages. What gives? :)
The point is to drop this from add_to_page_cache_locked() and make distribution
on split.
--
Kirill A. Shutemov
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
On 06/07/2013 08:10 AM, Kirill A. Shutemov wrote:
> + /*
> + * When we add a huge page to page cache we take only reference to head
> + * page, but on split we need to take addition reference to all tail
> + * pages since they are still in page cache after splitting.
> + */
> + init_tail_refcount = PageAnon(page) ? 0 : 1;
What's the "init" for in the name?
In add_to_page_cache_locked() in patch 12/39, you do
> + spin_lock_irq(>tree_lock);
> + for (i = 0; i < nr; i++) {
> + page_cache_get(page + i);
That looks to me to be taking references to the tail pages. What gives? :)
> for (i = HPAGE_PMD_NR - 1; i >= 1; i--) {
> struct page *page_tail = page + i;
>
> @@ -1587,8 +1595,9 @@ static void __split_huge_page_refcount(struct page
> *page,
>* atomic_set() here would be safe on all archs (and
>* not only on x86), it's safer to use atomic_add().
>*/
> - atomic_add(page_mapcount(page) + page_mapcount(page_tail) + 1,
> -_tail->_count);
> + atomic_add(init_tail_refcount + page_mapcount(page) +
> + page_mapcount(page_tail) + 1,
> + _tail->_count);
>
> /* after clearing PageTail the gup refcount can be released */
> smp_mb();
This does look much better in general, though.
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
On 06/07/2013 08:10 AM, Kirill A. Shutemov wrote:
+ /*
+ * When we add a huge page to page cache we take only reference to head
+ * page, but on split we need to take addition reference to all tail
+ * pages since they are still in page cache after splitting.
+ */
+ init_tail_refcount = PageAnon(page) ? 0 : 1;
What's the init for in the name?
In add_to_page_cache_locked() in patch 12/39, you do
+ spin_lock_irq(mapping-tree_lock);
+ for (i = 0; i nr; i++) {
+ page_cache_get(page + i);
That looks to me to be taking references to the tail pages. What gives? :)
for (i = HPAGE_PMD_NR - 1; i = 1; i--) {
struct page *page_tail = page + i;
@@ -1587,8 +1595,9 @@ static void __split_huge_page_refcount(struct page
*page,
* atomic_set() here would be safe on all archs (and
* not only on x86), it's safer to use atomic_add().
*/
- atomic_add(page_mapcount(page) + page_mapcount(page_tail) + 1,
-page_tail-_count);
+ atomic_add(init_tail_refcount + page_mapcount(page) +
+ page_mapcount(page_tail) + 1,
+ page_tail-_count);
/* after clearing PageTail the gup refcount can be released */
smp_mb();
This does look much better in general, though.
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
Dave Hansen wrote:
On 06/07/2013 08:10 AM, Kirill A. Shutemov wrote:
+ /*
+* When we add a huge page to page cache we take only reference to head
+* page, but on split we need to take addition reference to all tail
+* pages since they are still in page cache after splitting.
+*/
+ init_tail_refcount = PageAnon(page) ? 0 : 1;
What's the init for in the name?
initial_tail_refcount?
In add_to_page_cache_locked() in patch 12/39, you do
+ spin_lock_irq(mapping-tree_lock);
+ for (i = 0; i nr; i++) {
+ page_cache_get(page + i);
That looks to me to be taking references to the tail pages. What gives? :)
The point is to drop this from add_to_page_cache_locked() and make distribution
on split.
--
Kirill A. Shutemov
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
Kirill A. Shutemov wrote:
> Dave Hansen wrote:
> > Which reminds me... Why do we handle their reference counts differently? :)
> >
> > It seems like we could easily put a for loop in delete_from_page_cache()
> > that will release their reference counts along with the head page.
> > Wouldn't that make the code less special-cased for tail pages?
>
> delete_from_page_cache() is not the only user of
> __delete_from_page_cache()...
>
> It seems I did it wrong in add_to_page_cache_locked(). We shouldn't take
> references on tail pages there, only one on head. On split it will be
> distributed properly.
This way:
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index b267859..c2c0df2 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1556,6 +1556,7 @@ static void __split_huge_page_refcount(struct page *page,
struct zone *zone = page_zone(page);
struct lruvec *lruvec;
int tail_count = 0;
+ int init_tail_refcount;
/* prevent PageLRU to go away from under us, and freeze lru stats */
spin_lock_irq(>lru_lock);
@@ -1565,6 +1566,13 @@ static void __split_huge_page_refcount(struct page *page,
/* complete memcg works before add pages to LRU */
mem_cgroup_split_huge_fixup(page);
+ /*
+* When we add a huge page to page cache we take only reference to head
+* page, but on split we need to take addition reference to all tail
+* pages since they are still in page cache after splitting.
+*/
+ init_tail_refcount = PageAnon(page) ? 0 : 1;
+
for (i = HPAGE_PMD_NR - 1; i >= 1; i--) {
struct page *page_tail = page + i;
@@ -1587,8 +1595,9 @@ static void __split_huge_page_refcount(struct page *page,
* atomic_set() here would be safe on all archs (and
* not only on x86), it's safer to use atomic_add().
*/
- atomic_add(page_mapcount(page) + page_mapcount(page_tail) + 1,
- _tail->_count);
+ atomic_add(init_tail_refcount + page_mapcount(page) +
+ page_mapcount(page_tail) + 1,
+ _tail->_count);
/* after clearing PageTail the gup refcount can be released */
smp_mb();
--
Kirill A. Shutemov
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
Kirill A. Shutemov wrote:
Dave Hansen wrote:
Which reminds me... Why do we handle their reference counts differently? :)
It seems like we could easily put a for loop in delete_from_page_cache()
that will release their reference counts along with the head page.
Wouldn't that make the code less special-cased for tail pages?
delete_from_page_cache() is not the only user of
__delete_from_page_cache()...
It seems I did it wrong in add_to_page_cache_locked(). We shouldn't take
references on tail pages there, only one on head. On split it will be
distributed properly.
This way:
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index b267859..c2c0df2 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1556,6 +1556,7 @@ static void __split_huge_page_refcount(struct page *page,
struct zone *zone = page_zone(page);
struct lruvec *lruvec;
int tail_count = 0;
+ int init_tail_refcount;
/* prevent PageLRU to go away from under us, and freeze lru stats */
spin_lock_irq(zone-lru_lock);
@@ -1565,6 +1566,13 @@ static void __split_huge_page_refcount(struct page *page,
/* complete memcg works before add pages to LRU */
mem_cgroup_split_huge_fixup(page);
+ /*
+* When we add a huge page to page cache we take only reference to head
+* page, but on split we need to take addition reference to all tail
+* pages since they are still in page cache after splitting.
+*/
+ init_tail_refcount = PageAnon(page) ? 0 : 1;
+
for (i = HPAGE_PMD_NR - 1; i = 1; i--) {
struct page *page_tail = page + i;
@@ -1587,8 +1595,9 @@ static void __split_huge_page_refcount(struct page *page,
* atomic_set() here would be safe on all archs (and
* not only on x86), it's safer to use atomic_add().
*/
- atomic_add(page_mapcount(page) + page_mapcount(page_tail) + 1,
- page_tail-_count);
+ atomic_add(init_tail_refcount + page_mapcount(page) +
+ page_mapcount(page_tail) + 1,
+ page_tail-_count);
/* after clearing PageTail the gup refcount can be released */
smp_mb();
--
Kirill A. Shutemov
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
Dave Hansen wrote:
> On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote:
> > From: "Kirill A. Shutemov"
> >
> > As with add_to_page_cache_locked() we handle HPAGE_CACHE_NR pages a
> > time.
> >
> > Signed-off-by: Kirill A. Shutemov
> > ---
> > mm/filemap.c | 31 +--
> > 1 file changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index b0c7c8c..657ce82 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -115,6 +115,9 @@
> > void __delete_from_page_cache(struct page *page)
> > {
> > struct address_space *mapping = page->mapping;
> > + bool thp = PageTransHuge(page) &&
> > + IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE_PAGECACHE);
> > + int nr;
>
> Is that check for the config option really necessary? How would we get
> a page with PageTransHuge() set without it being enabled?
I'll drop it and use hpagecache_nr_page() instead.
> I like to rewrite your code. :)
It's nice. Thanks.
> Which reminds me... Why do we handle their reference counts differently? :)
>
> It seems like we could easily put a for loop in delete_from_page_cache()
> that will release their reference counts along with the head page.
> Wouldn't that make the code less special-cased for tail pages?
delete_from_page_cache() is not the only user of
__delete_from_page_cache()...
It seems I did it wrong in add_to_page_cache_locked(). We shouldn't take
references on tail pages there, only one on head. On split it will be
distributed properly.
> > /* Leave page->index set: truncation lookup relies upon it */
> > - mapping->nrpages--;
> > - __dec_zone_page_state(page, NR_FILE_PAGES);
> > + mapping->nrpages -= nr;
> > + __mod_zone_page_state(page_zone(page), NR_FILE_PAGES, -nr);
> > if (PageSwapBacked(page))
> > - __dec_zone_page_state(page, NR_SHMEM);
> > + __mod_zone_page_state(page_zone(page), NR_SHMEM, -nr);
> > BUG_ON(page_mapped(page));
>
> Man, we suck:
>
> __dec_zone_page_state()
> and
> __mod_zone_page_state()
>
> take a differently-typed first argument.
>
> Would there be any good to making __dec_zone_page_state() check to see
> if the page we passed in _is_ a compound page, and adjusting its
> behaviour accordingly?
Yeah, it would be better but I think it outside the scope of the patchset.
Probably, later.
--
Kirill A. Shutemov
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
Dave Hansen wrote:
On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote:
From: Kirill A. Shutemov kirill.shute...@linux.intel.com
As with add_to_page_cache_locked() we handle HPAGE_CACHE_NR pages a
time.
Signed-off-by: Kirill A. Shutemov kirill.shute...@linux.intel.com
---
mm/filemap.c | 31 +--
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index b0c7c8c..657ce82 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -115,6 +115,9 @@
void __delete_from_page_cache(struct page *page)
{
struct address_space *mapping = page-mapping;
+ bool thp = PageTransHuge(page)
+ IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE_PAGECACHE);
+ int nr;
Is that check for the config option really necessary? How would we get
a page with PageTransHuge() set without it being enabled?
I'll drop it and use hpagecache_nr_page() instead.
I like to rewrite your code. :)
It's nice. Thanks.
Which reminds me... Why do we handle their reference counts differently? :)
It seems like we could easily put a for loop in delete_from_page_cache()
that will release their reference counts along with the head page.
Wouldn't that make the code less special-cased for tail pages?
delete_from_page_cache() is not the only user of
__delete_from_page_cache()...
It seems I did it wrong in add_to_page_cache_locked(). We shouldn't take
references on tail pages there, only one on head. On split it will be
distributed properly.
/* Leave page-index set: truncation lookup relies upon it */
- mapping-nrpages--;
- __dec_zone_page_state(page, NR_FILE_PAGES);
+ mapping-nrpages -= nr;
+ __mod_zone_page_state(page_zone(page), NR_FILE_PAGES, -nr);
if (PageSwapBacked(page))
- __dec_zone_page_state(page, NR_SHMEM);
+ __mod_zone_page_state(page_zone(page), NR_SHMEM, -nr);
BUG_ON(page_mapped(page));
Man, we suck:
__dec_zone_page_state()
and
__mod_zone_page_state()
take a differently-typed first argument. sigh
Would there be any good to making __dec_zone_page_state() check to see
if the page we passed in _is_ a compound page, and adjusting its
behaviour accordingly?
Yeah, it would be better but I think it outside the scope of the patchset.
Probably, later.
--
Kirill A. Shutemov
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov"
>
> As with add_to_page_cache_locked() we handle HPAGE_CACHE_NR pages a
> time.
>
> Signed-off-by: Kirill A. Shutemov
> ---
> mm/filemap.c | 31 +--
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index b0c7c8c..657ce82 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -115,6 +115,9 @@
> void __delete_from_page_cache(struct page *page)
> {
> struct address_space *mapping = page->mapping;
> + bool thp = PageTransHuge(page) &&
> + IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE_PAGECACHE);
> + int nr;
Is that check for the config option really necessary? How would we get
a page with PageTransHuge() set without it being enabled?
> trace_mm_filemap_delete_from_page_cache(page);
> /*
> @@ -127,13 +130,29 @@ void __delete_from_page_cache(struct page *page)
> else
> cleancache_invalidate_page(mapping, page);
>
> - radix_tree_delete(>page_tree, page->index);
> + if (thp) {
> + int i;
> +
> + nr = HPAGE_CACHE_NR;
> + radix_tree_delete(>page_tree, page->index);
> + for (i = 1; i < HPAGE_CACHE_NR; i++) {
> + radix_tree_delete(>page_tree, page->index + i);
> + page[i].mapping = NULL;
> + page_cache_release(page + i);
> + }
> + __dec_zone_page_state(page, NR_FILE_TRANSPARENT_HUGEPAGES);
> + } else {
> + BUG_ON(PageTransHuge(page));
> + nr = 1;
> + radix_tree_delete(>page_tree, page->index);
> + }
> page->mapping = NULL;
I like to rewrite your code. :)
nr = hpage_nr_pages(page);
for (i = 0; i < nr; i++) {
page[i].mapping = NULL;
radix_tree_delete(>page_tree, page->index + i);
/* tail pages: */
if (i)
page_cache_release(page + i);
}
if (thp)
__dec_zone_page_state(page, NR_FILE_TRANSPARENT_HUGEPAGES);
I like this because it explicitly calls out the logic that tail pages
are different from head pages. We handle their reference counts
differently.
Which reminds me... Why do we handle their reference counts differently? :)
It seems like we could easily put a for loop in delete_from_page_cache()
that will release their reference counts along with the head page.
Wouldn't that make the code less special-cased for tail pages?
> /* Leave page->index set: truncation lookup relies upon it */
> - mapping->nrpages--;
> - __dec_zone_page_state(page, NR_FILE_PAGES);
> + mapping->nrpages -= nr;
> + __mod_zone_page_state(page_zone(page), NR_FILE_PAGES, -nr);
> if (PageSwapBacked(page))
> - __dec_zone_page_state(page, NR_SHMEM);
> + __mod_zone_page_state(page_zone(page), NR_SHMEM, -nr);
> BUG_ON(page_mapped(page));
Man, we suck:
__dec_zone_page_state()
and
__mod_zone_page_state()
take a differently-typed first argument.
Would there be any good to making __dec_zone_page_state() check to see
if the page we passed in _is_ a compound page, and adjusting its
behaviour accordingly?
> /*
> @@ -144,8 +163,8 @@ void __delete_from_page_cache(struct page *page)
>* having removed the page entirely.
>*/
> if (PageDirty(page) && mapping_cap_account_dirty(mapping)) {
> - dec_zone_page_state(page, NR_FILE_DIRTY);
> - dec_bdi_stat(mapping->backing_dev_info, BDI_RECLAIMABLE);
> + mod_zone_page_state(page_zone(page), NR_FILE_DIRTY, -nr);
> + add_bdi_stat(mapping->backing_dev_info, BDI_RECLAIMABLE, -nr);
> }
> }
Ahh, I see now why you didn't need a dec_bdi_stat(). Oh well...
--
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: [PATCHv4 14/39] thp, mm: rewrite delete_from_page_cache() to support huge pages
On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote:
From: Kirill A. Shutemov kirill.shute...@linux.intel.com
As with add_to_page_cache_locked() we handle HPAGE_CACHE_NR pages a
time.
Signed-off-by: Kirill A. Shutemov kirill.shute...@linux.intel.com
---
mm/filemap.c | 31 +--
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index b0c7c8c..657ce82 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -115,6 +115,9 @@
void __delete_from_page_cache(struct page *page)
{
struct address_space *mapping = page-mapping;
+ bool thp = PageTransHuge(page)
+ IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE_PAGECACHE);
+ int nr;
Is that check for the config option really necessary? How would we get
a page with PageTransHuge() set without it being enabled?
trace_mm_filemap_delete_from_page_cache(page);
/*
@@ -127,13 +130,29 @@ void __delete_from_page_cache(struct page *page)
else
cleancache_invalidate_page(mapping, page);
- radix_tree_delete(mapping-page_tree, page-index);
+ if (thp) {
+ int i;
+
+ nr = HPAGE_CACHE_NR;
+ radix_tree_delete(mapping-page_tree, page-index);
+ for (i = 1; i HPAGE_CACHE_NR; i++) {
+ radix_tree_delete(mapping-page_tree, page-index + i);
+ page[i].mapping = NULL;
+ page_cache_release(page + i);
+ }
+ __dec_zone_page_state(page, NR_FILE_TRANSPARENT_HUGEPAGES);
+ } else {
+ BUG_ON(PageTransHuge(page));
+ nr = 1;
+ radix_tree_delete(mapping-page_tree, page-index);
+ }
page-mapping = NULL;
I like to rewrite your code. :)
nr = hpage_nr_pages(page);
for (i = 0; i nr; i++) {
page[i].mapping = NULL;
radix_tree_delete(mapping-page_tree, page-index + i);
/* tail pages: */
if (i)
page_cache_release(page + i);
}
if (thp)
__dec_zone_page_state(page, NR_FILE_TRANSPARENT_HUGEPAGES);
I like this because it explicitly calls out the logic that tail pages
are different from head pages. We handle their reference counts
differently.
Which reminds me... Why do we handle their reference counts differently? :)
It seems like we could easily put a for loop in delete_from_page_cache()
that will release their reference counts along with the head page.
Wouldn't that make the code less special-cased for tail pages?
/* Leave page-index set: truncation lookup relies upon it */
- mapping-nrpages--;
- __dec_zone_page_state(page, NR_FILE_PAGES);
+ mapping-nrpages -= nr;
+ __mod_zone_page_state(page_zone(page), NR_FILE_PAGES, -nr);
if (PageSwapBacked(page))
- __dec_zone_page_state(page, NR_SHMEM);
+ __mod_zone_page_state(page_zone(page), NR_SHMEM, -nr);
BUG_ON(page_mapped(page));
Man, we suck:
__dec_zone_page_state()
and
__mod_zone_page_state()
take a differently-typed first argument. sigh
Would there be any good to making __dec_zone_page_state() check to see
if the page we passed in _is_ a compound page, and adjusting its
behaviour accordingly?
/*
@@ -144,8 +163,8 @@ void __delete_from_page_cache(struct page *page)
* having removed the page entirely.
*/
if (PageDirty(page) mapping_cap_account_dirty(mapping)) {
- dec_zone_page_state(page, NR_FILE_DIRTY);
- dec_bdi_stat(mapping-backing_dev_info, BDI_RECLAIMABLE);
+ mod_zone_page_state(page_zone(page), NR_FILE_DIRTY, -nr);
+ add_bdi_stat(mapping-backing_dev_info, BDI_RECLAIMABLE, -nr);
}
}
Ahh, I see now why you didn't need a dec_bdi_stat(). Oh well...
--
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/
