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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs
On Mon, Apr 30, 2012 at 3:04 AM, Chris Metcalf <[email protected]> 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 <[email protected]>
> ---
Acked-by: Hillf Danton <[email protected]>
> 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
>
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 <[email protected]>
Acked-by: Hugh Dickins <[email protected]>
Cc: [email protected]
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