Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1313801 > unrolled thread

Re: [PATCHv12 34/37] thp: introduce deferred_split_huge_page()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv12 34/37] thp: introduce deferred_split_huge_page() Andrea Arcangeli <aarcange@redhat.com> - 2016-01-21 02:30 +0100
    [PATCH 1/3] thp: make split_queue per-node "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-01-21 13:10 +0100
    [PATCH 2/3] thp: change deferred_split_count() to return number of THP in queue "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-01-21 13:10 +0100
      Re: [PATCH 2/3] thp: change deferred_split_count() to return number  of THP in queue Andrea Arcangeli <aarcange@redhat.com> - 2016-01-22 15:40 +0100
        Re: [PATCH 2/3] thp: change deferred_split_count() to return number  of THP in queue "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-01-22 16:30 +0100
    [PATCH 3/3] thp: limit number of object to scan on deferred_split_scan() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-01-21 13:10 +0100
    [PATCH 0/3] Couple of fixes for deferred_split_huge_page() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-01-21 13:20 +0100
      Re: [PATCH 0/3] Couple of fixes for deferred_split_huge_page() Andrea Arcangeli <aarcange@redhat.com> - 2016-01-22 00:00 +0100

#1313801 — Re: [PATCHv12 34/37] thp: introduce deferred_split_huge_page()

Hello Kirill,

On Tue, Oct 06, 2015 at 06:24:01PM +0300, Kirill A. Shutemov wrote:
> +static unsigned long deferred_split_scan(struct shrinker *shrink,
> +		struct shrink_control *sc)
> +{
> +	unsigned long flags;
> +	LIST_HEAD(list), *pos, *next;
> +	struct page *page;
> +	int split = 0;
> +
> +	spin_lock_irqsave(&split_queue_lock, flags);
> +	list_splice_init(&split_queue, &list);
> +
> +	/* Take pin on all head pages to avoid freeing them under us */
> +	list_for_each_safe(pos, next, &list) {
> +		page = list_entry((void *)pos, struct page, mapping);
> +		page = compound_head(page);
> +		/* race with put_compound_page() */
> +		if (!get_page_unless_zero(page)) {
> +			list_del_init(page_deferred_list(page));
> +			split_queue_len--;
> +		}
> +	}
> +	spin_unlock_irqrestore(&split_queue_lock, flags);

While rebasing I noticed this loop looks a bit too heavy. There's no
lockbreak and no cap on the list size, and million of THP pages could
have been partially unmapped but not be entirely freed yet, and sit
there for a while (there are other scenarios but this is the one that
could more realistically happen with certain allocators). Then as
result of random memory pressure we'd be calling millions of
get_page_unless_zero across multiple NUMA nodes thrashing cachelines
at every list entry, with irq disabled too for the whole period.

I haven't verified it, but I guess that in some large NUMA (i.e. 4TiB)
system that could take down a CPU for a second or more with irq
disabled.

I think it needs to isolate a certain number of pages, not splice
(userland programs can invoke the shrinker through direct reclaim too
and they can't stuck there for too long) and perhaps use
sc->nr_to_scan to achieve that.

The split_queue can also be moved from global to the "struct
pglist_data" and then you can do NODE_DATA(sc->nid)->split_queue, same
for the spinlock. That will make it more scalable for the lock and
more efficient in freeing memory so we don't split THP from nodes
reclaim isn't currently interested about (reclaim will later try again
on the zones in the other nodes by itself if needed).

Thanks,
Andrea

[toc] | [next] | [standalone]

#1314139 — [PATCH 1/3] thp: make split_queue per-node

Andrea Arcangeli suggested to make split queue per-node to improve
scalability. Let's do it.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
---
 include/linux/mmzone.h |  6 ++++++
 mm/huge_memory.c       | 49 ++++++++++++++++++++++++++-----------------------
 mm/page_alloc.c        |  5 +++++
 3 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 33bb1b19273e..7b6c2cfee390 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -682,6 +682,12 @@ typedef struct pglist_data {
 	 */
 	unsigned long first_deferred_pfn;
 #endif /* CONFIG_DEFERRED_STRUCT_PAGE_INIT */
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+	spinlock_t split_queue_lock;
+	struct list_head split_queue;
+	unsigned long split_queue_len;
+#endif
 } pg_data_t;
 
 #define node_present_pages(nid)	(NODE_DATA(nid)->node_present_pages)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2d1ffe9d0e26..769ea8db5771 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -138,9 +138,6 @@ static struct khugepaged_scan khugepaged_scan = {
 	.mm_head = LIST_HEAD_INIT(khugepaged_scan.mm_head),
 };
 
-static DEFINE_SPINLOCK(split_queue_lock);
-static LIST_HEAD(split_queue);
-static unsigned long split_queue_len;
 static struct shrinker deferred_split_shrinker;
 
 static void set_recommended_min_free_kbytes(void)
@@ -3358,6 +3355,7 @@ int total_mapcount(struct page *page)
 int split_huge_page_to_list(struct page *page, struct list_head *list)
 {
 	struct page *head = compound_head(page);
+	struct pglist_data *pgdata = NODE_DATA(page_to_nid(head));
 	struct anon_vma *anon_vma;
 	int count, mapcount, ret;
 	bool mlocked;
@@ -3401,19 +3399,19 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		lru_add_drain();
 
 	/* Prevent deferred_split_scan() touching ->_count */
-	spin_lock_irqsave(&split_queue_lock, flags);
+	spin_lock_irqsave(&pgdata->split_queue_lock, flags);
 	count = page_count(head);
 	mapcount = total_mapcount(head);
 	if (!mapcount && count == 1) {
 		if (!list_empty(page_deferred_list(head))) {
-			split_queue_len--;
+			pgdata->split_queue_len--;
 			list_del(page_deferred_list(head));
 		}
-		spin_unlock_irqrestore(&split_queue_lock, flags);
+		spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 		__split_huge_page(page, list);
 		ret = 0;
 	} else if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
-		spin_unlock_irqrestore(&split_queue_lock, flags);
+		spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 		pr_alert("total_mapcount: %u, page_count(): %u\n",
 				mapcount, count);
 		if (PageTail(page))
@@ -3421,7 +3419,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		dump_page(page, "total_mapcount(head) > 0");
 		BUG();
 	} else {
-		spin_unlock_irqrestore(&split_queue_lock, flags);
+		spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 		unfreeze_page(anon_vma, head);
 		ret = -EBUSY;
 	}
@@ -3436,52 +3434,56 @@ out:
 
 void free_transhuge_page(struct page *page)
 {
+	struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
 	unsigned long flags;
 
-	spin_lock_irqsave(&split_queue_lock, flags);
+	spin_lock_irqsave(&pgdata->split_queue_lock, flags);
 	if (!list_empty(page_deferred_list(page))) {
-		split_queue_len--;
+		pgdata->split_queue_len--;
 		list_del(page_deferred_list(page));
 	}
-	spin_unlock_irqrestore(&split_queue_lock, flags);
+	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 	free_compound_page(page);
 }
 
 void deferred_split_huge_page(struct page *page)
 {
+	struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
 	unsigned long flags;
 
 	VM_BUG_ON_PAGE(!PageTransHuge(page), page);
 
-	spin_lock_irqsave(&split_queue_lock, flags);
+	spin_lock_irqsave(&pgdata->split_queue_lock, flags);
 	if (list_empty(page_deferred_list(page))) {
-		list_add_tail(page_deferred_list(page), &split_queue);
-		split_queue_len++;
+		list_add_tail(page_deferred_list(page), &pgdata->split_queue);
+		pgdata->split_queue_len++;
 	}
-	spin_unlock_irqrestore(&split_queue_lock, flags);
+	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 }
 
 static unsigned long deferred_split_count(struct shrinker *shrink,
 		struct shrink_control *sc)
 {
+	struct pglist_data *pgdata = NODE_DATA(sc->nid);
 	/*
 	 * Split a page from split_queue will free up at least one page,
 	 * at most HPAGE_PMD_NR - 1. We don't track exact number.
 	 * Let's use HPAGE_PMD_NR / 2 as ballpark.
 	 */
-	return ACCESS_ONCE(split_queue_len) * HPAGE_PMD_NR / 2;
+	return ACCESS_ONCE(pgdata->split_queue_len) * HPAGE_PMD_NR / 2;
 }
 
 static unsigned long deferred_split_scan(struct shrinker *shrink,
 		struct shrink_control *sc)
 {
+	struct pglist_data *pgdata = NODE_DATA(sc->nid);
 	unsigned long flags;
 	LIST_HEAD(list), *pos, *next;
 	struct page *page;
 	int split = 0;
 
-	spin_lock_irqsave(&split_queue_lock, flags);
-	list_splice_init(&split_queue, &list);
+	spin_lock_irqsave(&pgdata->split_queue_lock, flags);
+	list_splice_init(&pgdata->split_queue, &list);
 
 	/* Take pin on all head pages to avoid freeing them under us */
 	list_for_each_safe(pos, next, &list) {
@@ -3490,10 +3492,10 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 		/* race with put_compound_page() */
 		if (!get_page_unless_zero(page)) {
 			list_del_init(page_deferred_list(page));
-			split_queue_len--;
+			pgdata->split_queue_len--;
 		}
 	}
-	spin_unlock_irqrestore(&split_queue_lock, flags);
+	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 
 	list_for_each_safe(pos, next, &list) {
 		page = list_entry((void *)pos, struct page, mapping);
@@ -3505,9 +3507,9 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 		put_page(page);
 	}
 
-	spin_lock_irqsave(&split_queue_lock, flags);
-	list_splice_tail(&list, &split_queue);
-	spin_unlock_irqrestore(&split_queue_lock, flags);
+	spin_lock_irqsave(&pgdata->split_queue_lock, flags);
+	list_splice_tail(&list, &pgdata->split_queue);
+	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 
 	return split * HPAGE_PMD_NR / 2;
 }
@@ -3516,6 +3518,7 @@ static struct shrinker deferred_split_shrinker = {
 	.count_objects = deferred_split_count,
 	.scan_objects = deferred_split_scan,
 	.seeks = DEFAULT_SEEKS,
+	.flags = SHRINKER_NUMA_AWARE,
 };
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 63358d9f9aa9..ea2c4d3e0c03 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5210,6 +5210,11 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat)
 	pgdat->numabalancing_migrate_nr_pages = 0;
 	pgdat->numabalancing_migrate_next_window = jiffies;
 #endif
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+	spin_lock_init(&pgdat->split_queue_lock);
+	INIT_LIST_HEAD(&pgdat->split_queue);
+	pgdat->split_queue_len = 0;
+#endif
 	init_waitqueue_head(&pgdat->kswapd_wait);
 	init_waitqueue_head(&pgdat->pfmemalloc_wait);
 	pgdat_page_ext_init(pgdat);
-- 
2.7.0.rc3

[toc] | [prev] | [next] | [standalone]

#1314142 — [PATCH 2/3] thp: change deferred_split_count() to return number of THP in queue

I've got meaning of shrinker::count_objects() wrong: it should return
number of potentially freeable objects, which is not necessary correlate
with freeable memory.

Returning 256 per THP in queue is not reasonable:
shrinker::scan_objects() never called with nr_to_scan > 128 in my setup.

Let's return 1 per THP and correct scan_object accordingly.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/huge_memory.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 769ea8db5771..36f98459f854 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3465,12 +3465,7 @@ static unsigned long deferred_split_count(struct shrinker *shrink,
 		struct shrink_control *sc)
 {
 	struct pglist_data *pgdata = NODE_DATA(sc->nid);
-	/*
-	 * Split a page from split_queue will free up at least one page,
-	 * at most HPAGE_PMD_NR - 1. We don't track exact number.
-	 * Let's use HPAGE_PMD_NR / 2 as ballpark.
-	 */
-	return ACCESS_ONCE(pgdata->split_queue_len) * HPAGE_PMD_NR / 2;
+	return ACCESS_ONCE(pgdata->split_queue_len);
 }
 
 static unsigned long deferred_split_scan(struct shrinker *shrink,
@@ -3511,7 +3506,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 	list_splice_tail(&list, &pgdata->split_queue);
 	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 
-	return split * HPAGE_PMD_NR / 2;
+	return split;
 }
 
 static struct shrinker deferred_split_shrinker = {
-- 
2.7.0.rc3

[toc] | [prev] | [next] | [standalone]

#1315044 — Re: [PATCH 2/3] thp: change deferred_split_count() to return number of THP in queue

On Thu, Jan 21, 2016 at 03:09:22PM +0300, Kirill A. Shutemov wrote:
> @@ -3511,7 +3506,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>  	list_splice_tail(&list, &pgdata->split_queue);
>  	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>  
> -	return split * HPAGE_PMD_NR / 2;
> +	return split;
>  }

Looking further at how the caller processes this "split" retval, if
the list has been fully shrunk by the page freeing, between the
split_count and split_scan, the caller seems to ignore a 0 value
returned above and it'll keep calling even if sc->nr_to_scan isn't
decreasing. The caller won't even check sc->nr_to_scan to notice that
it isn't decreasing anymore, it's write-only as far as the caller is
concerned.

It's also weird we can't return the number of freed pages and break
the loop with just one invocation of the split_scan, but that's a
slight inefficiency in the caller interface. The caller also seems to
forget to set total_scan to 0 if SHRINK_STOP was returned but perhaps
that's on purpose, however for our purpose it'd be better off if it
did.

The split_queue.next is going to be hot in the CPU cache anyway, so
unless we change the caller, it should be worth it to add a list_empty
check and return SHRINK_STOP if it was empty. Doing it at the start or
end doesn't make much difference, at the end lockless it'll deal with
the split failures too if any.

	return split ? : list_empty(&pgdat->split_queue) ? SPLIT_STOP : 0;

Thanks,
Andrea

[toc] | [prev] | [next] | [standalone]

#1315073 — Re: [PATCH 2/3] thp: change deferred_split_count() to return number of THP in queue

On Fri, Jan 22, 2016 at 03:31:27PM +0100, Andrea Arcangeli wrote:
> On Thu, Jan 21, 2016 at 03:09:22PM +0300, Kirill A. Shutemov wrote:
> > @@ -3511,7 +3506,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
> >  	list_splice_tail(&list, &pgdata->split_queue);
> >  	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
> >  
> > -	return split * HPAGE_PMD_NR / 2;
> > +	return split;
> >  }
> 
> Looking further at how the caller processes this "split" retval, if
> the list has been fully shrunk by the page freeing, between the
> split_count and split_scan, the caller seems to ignore a 0 value
> returned above and it'll keep calling even if sc->nr_to_scan isn't
> decreasing. The caller won't even check sc->nr_to_scan to notice that
> it isn't decreasing anymore, it's write-only as far as the caller is
> concerned.
> 
> It's also weird we can't return the number of freed pages and break
> the loop with just one invocation of the split_scan, but that's a
> slight inefficiency in the caller interface. The caller also seems to
> forget to set total_scan to 0 if SHRINK_STOP was returned but perhaps
> that's on purpose, however for our purpose it'd be better off if it
> did.
> 
> The split_queue.next is going to be hot in the CPU cache anyway, so
> unless we change the caller, it should be worth it to add a list_empty
> check and return SHRINK_STOP if it was empty. Doing it at the start or
> end doesn't make much difference, at the end lockless it'll deal with
> the split failures too if any.
> 
> 	return split ? : list_empty(&pgdat->split_queue) ? SPLIT_STOP : 0;

Ughh. Shrinker interface is confusing.

From ed85b527b2ea5c0b8ed93d9d212f9f0d25cae3ab Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Fri, 22 Jan 2016 18:10:59 +0300
Subject: [PATCH] thp: deferred_split_scan(): stop shrinker if the queue is
 empty

If pages on queue were freed under us, deferred_split_scan() would
return zero. It makes caller keep calling deferred_split_scan() without
any result.

Let's return SHRINK_STOP in this situation.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
---
 mm/huge_memory.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 298dbc001b07..2ea5e26ce069 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3508,6 +3508,12 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 	list_splice_tail(&list, &pgdata->split_queue);
 	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 
+	/*
+	 * Stop shrinker, if we didn't split any page, but the queue is empty.
+	 * This can happen if pages were freed under us.
+	 */
+	if (!split && list_empty(&pgdata->split_queue))
+		return SHRINK_STOP;
 	return split;
 }
 
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1314143 — [PATCH 3/3] thp: limit number of object to scan on deferred_split_scan()

If we have a lot of pages in queue to be split, deferred_split_scan()
can spend unreasonable amount of time under spinlock with disabled
interrupts.

Let's cap number of pages to split on scan by sc->nr_to_scan.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Andrea Arcangeli <aarcange@redhat.com>
---
 mm/huge_memory.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 36f98459f854..298dbc001b07 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3478,17 +3478,19 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 	int split = 0;
 
 	spin_lock_irqsave(&pgdata->split_queue_lock, flags);
-	list_splice_init(&pgdata->split_queue, &list);
-
 	/* Take pin on all head pages to avoid freeing them under us */
 	list_for_each_safe(pos, next, &list) {
 		page = list_entry((void *)pos, struct page, mapping);
 		page = compound_head(page);
-		/* race with put_compound_page() */
-		if (!get_page_unless_zero(page)) {
+		if (get_page_unless_zero(page)) {
+			list_move(page_deferred_list(page), &list);
+		} else {
+			/* We lost race with put_compound_page() */
 			list_del_init(page_deferred_list(page));
 			pgdata->split_queue_len--;
 		}
+		if (!--sc->nr_to_scan)
+			break;
 	}
 	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
 
-- 
2.7.0.rc3

[toc] | [prev] | [next] | [standalone]

#1314146 — [PATCH 0/3] Couple of fixes for deferred_split_huge_page()

Hi Andrea,

Sorry, I should be noticed and address the issue with scan before...

Patchset below should address your concern.

I've tested it in qemu with fake numa.

Kirill A. Shutemov (3):
  thp: make split_queue per-node
  thp: change deferred_split_count() to return number of THP in queue
  thp: limit number of object to scan on deferred_split_scan()

 include/linux/mmzone.h |  6 +++++
 mm/huge_memory.c       | 64 +++++++++++++++++++++++++-------------------------
 mm/page_alloc.c        |  5 ++++
 3 files changed, 43 insertions(+), 32 deletions(-)

-- 
2.7.0.rc3

[toc] | [prev] | [next] | [standalone]

#1314606 — Re: [PATCH 0/3] Couple of fixes for deferred_split_huge_page()

On Thu, Jan 21, 2016 at 03:09:20PM +0300, Kirill A. Shutemov wrote:
> Hi Andrea,
> 
> Sorry, I should be noticed and address the issue with scan before...
> 
> Patchset below should address your concern.
> 
> I've tested it in qemu with fake numa.

That was fast and already in -mm!

Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>

Great thanks,
Andrea

> 
> Kirill A. Shutemov (3):
>   thp: make split_queue per-node
>   thp: change deferred_split_count() to return number of THP in queue
>   thp: limit number of object to scan on deferred_split_scan()
> 
>  include/linux/mmzone.h |  6 +++++
>  mm/huge_memory.c       | 64 +++++++++++++++++++++++++-------------------------
>  mm/page_alloc.c        |  5 ++++
>  3 files changed, 43 insertions(+), 32 deletions(-)
> 
> -- 
> 2.7.0.rc3
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web