[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHS8izOHs0ZYR-0FCsDS6nxYbnNfYJFzWKGwB_utBJ_9bswjkA@mail.gmail.com>
Date: Thu, 20 May 2021 13:31:48 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Mike Kravetz <mike.kravetz@...cle.com>
Cc: Axel Rasmussen <axelrasmussen@...gle.com>,
Peter Xu <peterx@...hat.com>, Linux-MM <linux-mm@...ck.org>,
Andrew Morton <akpm@...ux-foundation.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm, hugetlb: fix resv_huge_pages underflow on UFFDIO_COPY
On Thu, May 20, 2021 at 1:00 PM Mike Kravetz <mike.kravetz@...cle.com> wrote:
>
> On 5/20/21 12:21 PM, Mina Almasry wrote:
> > On Thu, May 20, 2021 at 12:18 PM Mina Almasry <almasrymina@...gle.com> wrote:
> >>
> >> On Thu, May 13, 2021 at 5:14 PM Mike Kravetz <mike.kravetz@...cle.com> wrote:
> >>>
> >>> How about this approach?
> >>> - Keep the check for hugetlbfs_pagecache_present in hugetlb_mcopy_atomic_pte
> >>> that you added. That will catch the race where the page was added to
> >>> the cache before entering the routine.
> >>> - With the above check in place, we only need to worry about the case
> >>> where copy_huge_page_from_user fails and we must drop locks. In this
> >>> case we:
> >>> - Free the page previously allocated.
> >>> - Allocate a 'temporary' huge page without consuming reserves. I'm
> >>> thinking of something similar to page migration.
> >>> - Drop the locks and let the copy_huge_page_from_user be done to the
> >>> temporary page.
> >>> - When reentering hugetlb_mcopy_atomic_pte after dropping locks (the
> >>> *pagep case) we need to once again check
> >>> hugetlbfs_pagecache_present.
> >>> - We then try to allocate the huge page which will consume the
> >>> reserve. If successful, copy contents of temporary page to newly
> >>> allocated page. Free temporary page.
> >>>
> >>> There may be issues with this, and I have not given it deep thought. It
> >>> does abuse the temporary huge page concept, but perhaps no more than
> >>> page migration. Things do slow down if the extra page allocation and
> >>> copy is required, but that would only be the case if copy_huge_page_from_user
> >>> needs to be done without locks. Not sure, but hoping that is rare.
> >>
> >> Just following up this a bit: I've implemented this approach locally,
> >> and with it it's passing the test as-is. When I hack the code such
> >> that the copy in hugetlb_mcopy_atomic_pte() always fails, I run into
> >> this edge case, which causes resv_huge_pages to underflow again (this
> >> time permemantly):
> >>
> >> - hugetlb_no_page() is called on an index and a page is allocated and
> >> inserted into the cache consuming the reservation.
> >> - remove_huge_page() is called on this index and the page is removed from cache.
> >> - hugetlb_mcopy_atomic_pte() is called on this index, we do not find
> >> the page in the cache and we trigger this code patch and the copy
> >> fails.
> >> - The allocations in this code path seem to double consume the
> >> reservation and resv_huge_pages underflows.
> >>
> >> I'm looking at this edge case to understand why a prior
> >> remove_huge_page() causes my code to underflow resv_huge_pages.
> >>
> >
> > I should also mention, without a prior remove_huge_page() this code
> > path works fine, so it seems the fact that the reservation is consumed
> > before causes trouble, but I'm not sure why yet.
> >
>
> Hi Mina,
>
> How about quickly posting the code? I may be able to provide more
> suggestions if I can see the actual code.
Sure thing, attached my patch so far. It's quite messy with prints
everywhere and VM_BUG_ON() in error paths that I'm not handling yet.
I've also hacked the code so that the hugetlb_mcopy_atomic_pte() copy
always fails so I exercise that code path.
> --
> Mike Kravetz
Download attachment "0001-mm-hugetlb-fix-resv_huge_pages-underflow-on-UFFDIO_C.patch" of type "application/octet-stream" (8530 bytes)
Powered by blists - more mailing lists