* [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() @ 2012-04-29 19:04 Chris Metcalf 2012-04-30 20:19 ` Hugh Dickins 0 siblings, 1 reply; 7+ messages in thread From: Chris Metcalf @ 2012-04-29 19:04 UTC (permalink / raw) To: Andrew Morton, Hillf Danton, Michal Hocko, KAMEZAWA Hiroyuki, Hugh Dickins, linux-mm, linux-kernel Commit 66aebce747eaf added code to avoid a race condition by elevating the page refcount in hugetlb_fault() while calling hugetlb_cow(). However, one code path in hugetlb_cow() includes an assertion that the page count is 1, whereas it may now also have the value 2 in this path. Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> --- We discovered this while testing the original path; one particular application triggered this due to the specific number of huge pages it started with. mm/hugetlb.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/mm/hugetlb.c b/mm/hugetlb.c index cd65cb1..d5b0254 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2498,7 +2498,14 @@ retry_avoidcopy: if (outside_reserve) { BUG_ON(huge_pte_none(pte)); if (unmap_ref_private(mm, vma, old_page, address)) { - BUG_ON(page_count(old_page) != 1); + /* + * Page refcount may be 1 in the common case, + * but since we may do an extra get_page() + * when called from hugetlb_fault(), we allow + * a page refcount of 2 as well. + */ + BUG_ON(page_count(old_page) != 1 && + page_count(old_page) != 2); BUG_ON(huge_pte_none(pte)); spin_lock(&mm->page_table_lock); ptep = huge_pte_offset(mm, address & huge_page_mask(h)); -- 1.6.5.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() 2012-04-29 19:04 [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() Chris Metcalf @ 2012-04-30 20:19 ` Hugh Dickins 2012-05-01 13:14 ` Mel Gorman 0 siblings, 1 reply; 7+ messages in thread From: Hugh Dickins @ 2012-04-30 20:19 UTC (permalink / raw) To: Chris Metcalf, Mel Gorman Cc: Andrew Morton, Hillf Danton, Michal Hocko, KAMEZAWA Hiroyuki, linux-mm, linux-kernel On Sun, 29 Apr 2012, Chris Metcalf wrote: > Commit 66aebce747eaf added code to avoid a race condition by > elevating the page refcount in hugetlb_fault() while calling > hugetlb_cow(). However, one code path in hugetlb_cow() includes > an assertion that the page count is 1, whereas it may now also > have the value 2 in this path. > > Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> > --- > We discovered this while testing the original path; one particular > application triggered this due to the specific number of huge pages > it started with. Well done finding that. But I think it would be better to remove the BUG_ON() than complicate it, and then no need to add a comment there. IIRC it's unsafe to make any assertions about what a page_count() may be, beyond whether it's 0 or non-0: because of speculative accesses to the page from elsewhere (perhaps it used to be visible in a radix_tree, perhaps __isolate_lru_pages is having a go at it). I'd say that BUG_ON() has outlived its usefulness, and should just be eliminated now: but git "blames" Mel for it, so let's see if he agrees. Hugh > > mm/hugetlb.c | 9 ++++++++- > 1 files changed, 8 insertions(+), 1 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index cd65cb1..d5b0254 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2498,7 +2498,14 @@ retry_avoidcopy: > if (outside_reserve) { > BUG_ON(huge_pte_none(pte)); > if (unmap_ref_private(mm, vma, old_page, address)) { > - BUG_ON(page_count(old_page) != 1); > + /* > + * Page refcount may be 1 in the common case, > + * but since we may do an extra get_page() > + * when called from hugetlb_fault(), we allow > + * a page refcount of 2 as well. > + */ > + BUG_ON(page_count(old_page) != 1 && > + page_count(old_page) != 2); > BUG_ON(huge_pte_none(pte)); > spin_lock(&mm->page_table_lock); > ptep = huge_pte_offset(mm, address & huge_page_mask(h)); > -- > 1.6.5.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() 2012-04-30 20:19 ` Hugh Dickins @ 2012-05-01 13:14 ` Mel Gorman 2012-04-29 19:04 ` Chris Metcalf 0 siblings, 1 reply; 7+ messages in thread From: Mel Gorman @ 2012-05-01 13:14 UTC (permalink / raw) To: Hugh Dickins Cc: Chris Metcalf, Mel Gorman, Andrew Morton, Hillf Danton, Michal Hocko, KAMEZAWA Hiroyuki, linux-mm, linux-kernel On Mon, Apr 30, 2012 at 01:19:27PM -0700, Hugh Dickins wrote: > On Sun, 29 Apr 2012, Chris Metcalf wrote: > > > Commit 66aebce747eaf added code to avoid a race condition by > > elevating the page refcount in hugetlb_fault() while calling > > hugetlb_cow(). However, one code path in hugetlb_cow() includes > > an assertion that the page count is 1, whereas it may now also > > have the value 2 in this path. > > > > Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> > > --- > > We discovered this while testing the original path; one particular > > application triggered this due to the specific number of huge pages > > it started with. > > Well done finding that. Agreed. > But I think it would be better to remove the > BUG_ON() than complicate it, and then no need to add a comment there. > > IIRC it's unsafe to make any assertions about what a page_count() may > be, beyond whether it's 0 or non-0: because of speculative accesses to > the page from elsewhere (perhaps it used to be visible in a radix_tree, > perhaps __isolate_lru_pages is having a go at it). > There are relatively few cases where this type of hugetlbfs page can be found and the count elevated. The pages are not on the LRU for example and as it is privately mapped there are fewer cases where speculative accesses elevate the count. > I'd say that BUG_ON() has outlived its usefulness, and should just be > eliminated now: but git "blames" Mel for it, so let's see if he agrees. > The reason it was added in the first place was to rattle out any bugs related to unmap_ref_private(). As that was 4 years ago, I agree with High and the BUG_ON can go as it has done its job. Thanks. -- Mel Gorman SUSE Labs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() 2012-05-01 13:14 ` Mel Gorman @ 2012-04-29 19:04 ` Chris Metcalf 2012-05-01 13:47 ` Mel Gorman ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Chris Metcalf @ 2012-04-29 19:04 UTC (permalink / raw) To: Hugh Dickins Cc: Mel Gorman, Andrew Morton, Hillf Danton, Michal Hocko, KAMEZAWA Hiroyuki, linux-mm, linux-kernel Commit 66aebce747eaf added code to avoid a race condition by elevating the page refcount in hugetlb_fault() while calling hugetlb_cow(). However, one code path in hugetlb_cow() includes an assertion that the page count is 1, whereas it may now also have the value 2 in this path. Consensus is that this BUG_ON has served its purpose, so rather than extending it to cover both cases, we just remove it. Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> --- mm/hugetlb.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/mm/hugetlb.c b/mm/hugetlb.c index cd65cb1..baaad5d 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2498,7 +2498,6 @@ retry_avoidcopy: if (outside_reserve) { BUG_ON(huge_pte_none(pte)); if (unmap_ref_private(mm, vma, old_page, address)) { - BUG_ON(page_count(old_page) != 1); BUG_ON(huge_pte_none(pte)); spin_lock(&mm->page_table_lock); ptep = huge_pte_offset(mm, address & huge_page_mask(h)); -- 1.6.5.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() 2012-04-29 19:04 ` Chris Metcalf @ 2012-05-01 13:47 ` Mel Gorman 2012-05-01 13:53 ` Hillf Danton 2012-05-01 15:43 ` Hugh Dickins 2 siblings, 0 replies; 7+ messages in thread From: Mel Gorman @ 2012-05-01 13:47 UTC (permalink / raw) To: Chris Metcalf Cc: Hugh Dickins, Andrew Morton, Hillf Danton, Michal Hocko, KAMEZAWA Hiroyuki, linux-mm, linux-kernel On Sun, Apr 29, 2012 at 03:04:51PM -0400, Chris Metcalf wrote: > Commit 66aebce747eaf added code to avoid a race condition by > elevating the page refcount in hugetlb_fault() while calling > hugetlb_cow(). However, one code path in hugetlb_cow() includes > an assertion that the page count is 1, whereas it may now also > have the value 2 in this path. Consensus is that this BUG_ON > has served its purpose, so rather than extending it to cover both > cases, we just remove it. > > Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> Acked-by: Mel Gorman <mel@csn.ul.ie> -- Mel Gorman SUSE Labs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() 2012-04-29 19:04 ` Chris Metcalf 2012-05-01 13:47 ` Mel Gorman @ 2012-05-01 13:53 ` Hillf Danton 2012-05-01 15:43 ` Hugh Dickins 2 siblings, 0 replies; 7+ messages in thread From: Hillf Danton @ 2012-05-01 13:53 UTC (permalink / raw) To: Chris Metcalf Cc: Hugh Dickins, Mel Gorman, Andrew Morton, Michal Hocko, KAMEZAWA Hiroyuki, linux-mm, linux-kernel On Mon, Apr 30, 2012 at 3:04 AM, Chris Metcalf <cmetcalf@tilera.com> wrote: > Commit 66aebce747eaf added code to avoid a race condition by > elevating the page refcount in hugetlb_fault() while calling > hugetlb_cow(). However, one code path in hugetlb_cow() includes > an assertion that the page count is 1, whereas it may now also > have the value 2 in this path. Consensus is that this BUG_ON > has served its purpose, so rather than extending it to cover both > cases, we just remove it. > > Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> > --- Acked-by: Hillf Danton <dhillf@gmail.com> > mm/hugetlb.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index cd65cb1..baaad5d 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2498,7 +2498,6 @@ retry_avoidcopy: > if (outside_reserve) { > BUG_ON(huge_pte_none(pte)); > if (unmap_ref_private(mm, vma, old_page, address)) { > - BUG_ON(page_count(old_page) != 1); > BUG_ON(huge_pte_none(pte)); > spin_lock(&mm->page_table_lock); > ptep = huge_pte_offset(mm, address & huge_page_mask(h)); > -- > 1.6.5.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() 2012-04-29 19:04 ` Chris Metcalf 2012-05-01 13:47 ` Mel Gorman 2012-05-01 13:53 ` Hillf Danton @ 2012-05-01 15:43 ` Hugh Dickins 2 siblings, 0 replies; 7+ messages in thread From: Hugh Dickins @ 2012-05-01 15:43 UTC (permalink / raw) To: Chris Metcalf Cc: Mel Gorman, Andrew Morton, Hillf Danton, Michal Hocko, KAMEZAWA Hiroyuki, linux-mm, linux-kernel On Sun, 29 Apr 2012, Chris Metcalf wrote: No, Chris wrote this version on Mon, 30 Apr 2012, but gmail, or its intereaction with imap, is too confused to grasp that. > Commit 66aebce747eaf added code to avoid a race condition by > elevating the page refcount in hugetlb_fault() while calling > hugetlb_cow(). However, one code path in hugetlb_cow() includes > an assertion that the page count is 1, whereas it may now also > have the value 2 in this path. Consensus is that this BUG_ON > has served its purpose, so rather than extending it to cover both > cases, we just remove it. > > Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> Acked-by: Hugh Dickins <hughd@google.com> Cc: stable@vger.kernel.org It is rather important that we Cc stable on this, since the earlier fix triggering this BUG went out in 3.0.29, 3.2.16 and 3.3.3 stable. Sadly, 3.2.16 was the end of the 3.2 line... > --- > mm/hugetlb.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index cd65cb1..baaad5d 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2498,7 +2498,6 @@ retry_avoidcopy: > if (outside_reserve) { > BUG_ON(huge_pte_none(pte)); > if (unmap_ref_private(mm, vma, old_page, address)) { > - BUG_ON(page_count(old_page) != 1); > BUG_ON(huge_pte_none(pte)); > spin_lock(&mm->page_table_lock); > ptep = huge_pte_offset(mm, address & huge_page_mask(h)); > -- > 1.6.5.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-01 15:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-29 19:04 [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() Chris Metcalf 2012-04-30 20:19 ` Hugh Dickins 2012-05-01 13:14 ` Mel Gorman 2012-04-29 19:04 ` Chris Metcalf 2012-05-01 13:47 ` Mel Gorman 2012-05-01 13:53 ` Hillf Danton 2012-05-01 15:43 ` Hugh Dickins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).