Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Mike Kravetz]

On 2/2/22 00:14, David Hildenbrand wrote:
> On 02.02.22 02:40, Mike Kravetz wrote:
>> MADV_DONTNEED is currently disabled for hugetlb mappings.  This
>> certainly makes sense in shared file mappings as the pagecache maintains
>> a reference to the page and it will never be freed.  However, it could
>> be useful to unmap and free pages in private mappings.
>>
>> The only thing preventing MADV_DONTNEED from working on hugetlb mappings
>> is a check in can_madv_lru_vma().  To allow support for hugetlb mappings
>> create and use a new routine madvise_dontneed_free_valid_vma() that will
>> allow hugetlb mappings.  Also, before calling zap_page_range in the
>> DONTNEED case align start and size to huge page size for hugetlb vmas.
>> madvise only requires PAGE_SIZE alignment, but the hugetlb unmap routine
>> requires huge page size alignment.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>> ---
>>  mm/madvise.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 5604064df464..7ae891e030a4 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -796,10 +796,30 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
>>  static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
>>  					unsigned long start, unsigned long end)
>>  {
>> +	/*
>> +	 * start and size (end - start) must be huge page size aligned
>> +	 * for hugetlb vmas.
>> +	 */
>> +	if (is_vm_hugetlb_page(vma)) {
>> +		struct hstate *h = hstate_vma(vma);
>> +
>> +		start = ALIGN_DOWN(start, huge_page_size(h));
>> +		end = ALIGN(end, huge_page_size(h));
> 
> So you effectively extend the range silently. IIUC, if someone would zap
> a 4k range you would implicitly zap a whole 2M page and effectively zero
> out more data than requested.
> 
> 
> Looking at do_madvise(), we:
> (1) reject start addresses that are not page-aligned
> (2) shrink lengths that are not page-aligned and refuse if it turns 0

I believe length is extended (rounded up) by this line:
	len = PAGE_ALIGN(len_in);

but, I see your point.

> The man page documents (1) but doesn't really document (2).
> 
> Naturally I'd have assume that we apply the same logic to huge page
> sizes and documenting it in the man page accordingly.
> 
> 
> Why did you decide to extend the range? I'd assume MADV_REMOVE behaves
> like FALLOC_FL_PUNCH_HOLE:
>   "Within the specified range, partial filesystem blocks are zeroed, and
>    whole filesystem blocks are removed from the file.  After a
>    successful call, subsequent reads from  this  range will return
>    zeros."
> So we don't "discard more than requested".

Well.  hugetlbfs does not follow the man page. :(  It does not zero
partial blocks.  I assume a filesystem block would be a huge page.
Instead it does,

        /*
         * For hole punch round up the beginning offset of the hole and
         * round down the end.
         */
        hole_start = round_up(offset, hpage_size);
        hole_end = round_down(offset + len, hpage_size);

So, not only is this patch not following the man page.  It is not even
following the existing MADV_REMOVE hugetlb code.  Thanks for pointing
that out.  Part of my reason for adding this functionality was to make
hugetlb be more like 'normal' memory.  I clearly failed.

Related comment about madvise man page for PAGE_SIZE MADV_REMOVE.  The man
page says.

       MADV_REMOVE (since Linux 2.6.16)
              Free up a given range of pages and its associated backing store.
              This is equivalent to punching a hole in the corresponding  byte
              range  of  the backing store (see fallocate(2)).  Subsequent ac‐
              cesses in the specified address range will see bytes  containing
              zero.

This may need some clarification.  It says it will free pages.  We know
madvise only operates on pages (PAGE_ALIGN(len)).  Yet, the statement about
equivalent to a fallocate byte range may lead one to believe that length is
treated the same in madvise and fallocate.

> I see the following possible alternatives:
> (a) Fail if the range is not aligned
> -> Clear semantics
> (b) Fail if the start is not aligned, shrink the end if required
> -> Same rules as for PAGE_SIZE
> (c) Zero out the requested part
> -> Same semantics as FALLOC_FL_PUNCH_HOLE.
> 
> My preference would be a), properly documenting it in the man page.

However, a) would make hugetlb behave differently than other memory as
len does not need to be aligned.

I would prefer b) as it is more in line with PAGE_SIZE.  But, that does
make it different than MADV_REMOVE hugetlb alignment.

I thought this was simple. :)
-- 
Mike Kravetz