Attachment 644267 Details for Bug 876277 – The proposed fix

[patch] The proposed fix

list_corruption (text/plain), 4.86 KB, created by Russ Anderson on 2012-11-13 17:36:38 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Russ Anderson

Created: 2012-11-13 17:36:38 UTC

Size: 4.86 KB

patch

obsolete

>To: redhat
>Subject: [PATCH] mm: rhel6.3 gather_surplus_pages holds a lock
>From: Cliff Wickman <cpw@sgi.com>
>
>I am seeing list corruption occurring from within mm/hugetlb.c's
>gather_surplus_pages().  It's problem is its dropping of the hugetlb_lock.
>
>I have CONFIG_DEBUG_LIST=y, and am running an MPI application with 64 threads
>and a library that creates a large heap of hugetlbfs pages for it.
>
>The below patch fixes the problem.
>gather_surplus_pages() does not have to drop the lock if
>alloc_buddy_huge_page() is told whether the lock is already held.
>
>Current community code (linux-next) also has gather_surplus_pages() dropping
>the lock.  But gather_surplus_pages() is different in other ways that
>seem to prevent this problem.
>
>Michal Hocko's objection:
>  But you cannot hold spinlock while allocating memory because the
>  allocation is not atomic and you could deadlock easily.
>but the patch is correct.  The alloc_buddy_huge_page function always
>unlock/lock's around the allocation.  The point of the patch is whether
>the earlier part of alloc_buddy_huge_page() should lock.
>
>I have tested on a 64-processor system.  Without the patch the list_add
>corruption occurs almost immediately.  With the patch the entire regression
>suite of around 1000 tests runs with no list_add corruption.
>
>Signed-off-by: Cliff Wickman <cpw@sgi.com>
>---
> mm/hugetlb.c |   28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
>Index: linux/mm/hugetlb.c
>===================================================================
>--- linux.orig/mm/hugetlb.c
>+++ linux/mm/hugetlb.c
>@@ -748,7 +748,9 @@ static int free_pool_huge_page(struct hs
> 	return ret;
> }
> 
>-static struct page *alloc_buddy_huge_page(struct hstate *h, int nid)
>+/* already_locked means the caller has already locked hugetlb_lock */
>+static struct page *alloc_buddy_huge_page(struct hstate *h, int nid,
>+						int already_locked)
> {
> 	struct page *page;
> 	unsigned int r_nid;
>@@ -779,7 +781,8 @@ static struct page *alloc_buddy_huge_pag
> 	 * the node values until we've gotten the hugepage and only the
> 	 * per-node value is checked there.
> 	 */
>-	spin_lock(&hugetlb_lock);
>+	if (!already_locked)
>+		spin_lock(&hugetlb_lock);
> 	if (h->surplus_huge_pages >= h->nr_overcommit_huge_pages) {
> 		spin_unlock(&hugetlb_lock);
> 		return NULL;
>@@ -788,6 +791,7 @@ static struct page *alloc_buddy_huge_pag
> 		h->surplus_huge_pages++;
> 	}
> 	spin_unlock(&hugetlb_lock);
>+	/* page allocation may sleep, so the lock must be unlocked */
> 
> 	if (nid == NUMA_NO_NODE)
> 		page = alloc_pages(htlb_alloc_mask|__GFP_COMP|
>@@ -800,6 +804,9 @@ static struct page *alloc_buddy_huge_pag
> 
> 	if (page && arch_prepare_hugepage(page)) {
> 		__free_pages(page, huge_page_order(h));
>+		if (already_locked)
>+			/* leave it like it was */
>+			spin_lock(&hugetlb_lock);
> 		return NULL;
> 	}
> 
>@@ -818,7 +825,9 @@ static struct page *alloc_buddy_huge_pag
> 		h->surplus_huge_pages--;
> 		__count_vm_event(HTLB_BUDDY_PGALLOC_FAIL);
> 	}
>-	spin_unlock(&hugetlb_lock);
>+	if (!already_locked)
>+		/* leave it like it was */
>+		spin_unlock(&hugetlb_lock);
> 
> 	return page;
> }
>@@ -837,7 +846,7 @@ struct page *alloc_huge_page_node(struct
> 	spin_unlock(&hugetlb_lock);
> 
> 	if (!page)
>-		page = alloc_buddy_huge_page(h, nid);
>+		page = alloc_buddy_huge_page(h, nid, 0);
> 
> 	return page;
> }
>@@ -845,6 +854,7 @@ struct page *alloc_huge_page_node(struct
> /*
>  * Increase the hugetlb pool such that it can accomodate a reservation
>  * of size 'delta'.
>+ * This is entered and exited with hugetlb_lock locked.
>  */
> static int gather_surplus_pages(struct hstate *h, int delta)
> {
>@@ -864,9 +874,8 @@ static int gather_surplus_pages(struct h
> 
> 	ret = -ENOMEM;
> retry:
>-	spin_unlock(&hugetlb_lock);
> 	for (i = 0; i < needed; i++) {
>-		page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
>+		page = alloc_buddy_huge_page(h, NUMA_NO_NODE, 1);
> 		if (!page)
> 			/*
> 			 * We were not able to allocate enough pages to
>@@ -880,10 +889,9 @@ retry:
> 	allocated += needed;
> 
> 	/*
>-	 * After retaking hugetlb_lock, we need to recalculate 'needed'
>+	 * With hugetlb_lock still locked, we need to recalculate 'needed'
> 	 * because either resv_huge_pages or free_huge_pages may have changed.
> 	 */
>-	spin_lock(&hugetlb_lock);
> 	needed = (h->resv_huge_pages + delta) -
> 			(h->free_huge_pages + allocated);
> 	if (needed > 0)
>@@ -901,7 +909,6 @@ retry:
> 	h->resv_huge_pages += delta;
> 	ret = 0;
> 
>-	spin_unlock(&hugetlb_lock);
> 	/* Free the needed pages to the hugetlb pool */
> 	list_for_each_entry_safe(page, tmp, &surplus_list, lru) {
> 		if ((--needed) < 0)
>@@ -924,7 +931,6 @@ free:
> 			put_page(page);
> 		}
> 	}
>-	spin_lock(&hugetlb_lock);
> 
> 	return ret;
> }
>@@ -1044,7 +1050,7 @@ static struct page *alloc_huge_page(stru
> 	spin_unlock(&hugetlb_lock);
> 
> 	if (!page) {
>-		page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
>+		page = alloc_buddy_huge_page(h, NUMA_NO_NODE, 0);
> 		if (!page) {
> 			hugetlb_put_quota(inode->i_mapping, chg);
> 			return ERR_PTR(-VM_FAULT_SIGBUS);

Actions: View | Diff

Attachments on bug 876277: 644267