Re: [PATCH 1/2] hugetlbfs: extend hugetlb_vma_lock to private VMAs

[Mike Kravetz]

On 09/22/23 10:37, Rik van Riel wrote:
> On Thu, 2023-09-21 at 15:42 -0700, Mike Kravetz wrote:
> > On 09/19/23 22:16, riel@xxxxxxxxxxx wrote:
> > > From: Rik van Riel <riel@xxxxxxxxxxx>
> > > 
> > > Extend the locking scheme used to protect shared hugetlb mappings
> > > from truncate vs page fault races, in order to protect private
> > > hugetlb mappings (with resv_map) against MADV_DONTNEED.
> > > 
> > > Add a read-write semaphore to the resv_map data structure, and
> > > use that from the hugetlb_vma_(un)lock_* functions, in preparation
> > > for closing the race between MADV_DONTNEED and page faults.
> > > 
> > > Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> > > ---
> > >  include/linux/hugetlb.h |  6 ++++++
> > >  mm/hugetlb.c            | 36 ++++++++++++++++++++++++++++++++----
> > >  2 files changed, 38 insertions(+), 4 deletions(-)
> > 
> > This looks straight forward.
> > 
> > However, I ran just this patch through libhugetlbfs test suite and it
> > hung on
> > misaligned_offset (2M: 32).
> > https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/misaligned_offset.c
> 
> 
> Speaking of "looks straightforward", how do I compile the
> libhugetlbfs code?
> 
> The __morecore variable, which is pointed at either the
> THP or hugetlbfs morecore function, does not seem to be
> defined anywhere in the sources.
> 
> Do I need to run some magic script (didn't find it) to
> get a special header file set up before I can build
> libhugetlbfs?

libhugetlbfs is a mess!  Distros have dropped it.  However, I still find
the test cases useful.  I have a special VM with an old glibc just for
running the tests.

Sorry, can't give instructions for using tests on a recent glibc.

But, back to this patch ...
With the hints from the locking debug code, it came to me on my walk this
morning.  We need to also have __hugetlb_vma_unlock_write_free() work
for private vmas as called from __unmap_hugepage_range_final.  This
additional change (or something like it) is required in this patch.

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f906c5fa4d09..8f3d5895fffc 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -372,6 +372,11 @@ static void __hugetlb_vma_unlock_write_free(struct vm_area_struct *vma)
 		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
 
 		__hugetlb_vma_unlock_write_put(vma_lock);
+	} else if (__vma_private_lock(vma)) {
+		struct resv_map *resv_map = vma_resv_map(vma);
+
+		/* no free for anon vmas, but still need to unlock */
+		up_write(&resv_map->rw_sema);
 	}
 }
 

-- 
Mike Kravetz