Date:   Fri, 4 Feb 2022 09:35:01 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Cc:     Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Mina Almasry <almasrymina@...gle.com>,
        Michal Hocko <mhocko@...e.com>, Peter Xu <peterx@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Shuah Khan <shuah@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

>>> +	/*
>>> +	 * 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);

Ah, right. I was confused by the "!len" check below that, but the
comment explains how this applies to negative values only.

> 
> 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);

Okay, so we skip any zeroing and only free completely covered blocks. We
might want to document that behavior. See below.

> 
> 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.

Yes

> 
>> 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. :)

It really bugs me that it's under-specified what's supposed to happen
when the length is not aligned.

BUT: in the posix world, "calling posix_madvise() shall not affect the
semantics of access to memory in the specified range". So we don't care
too much about if we align up/down, because it wouldn't affect the
semantics. Especially for MADV_DONTNEED/MADV_REMOVE as implemented by
Linux this is certainly different and the alignment handling matters.

So I guess especially for MADV_DONTNEED/MADV_REMOVE we need a clear
specification what's supposed to happen if the length falls into the
middle of a huge page. We should document alignment handling for
madvise() in general I assume.

IMHO we should have bailed out right from the start whenever something
is not properly aligned, but that ship has sailed. So I agree, maybe we
can make at least hugetlb MADV_DONTNEED obey the same (weird) rules as
ordinary pages.

So b) would mean, requiring start to be hugepage aligned and aligning-up
the end. Still feels wrong but at least matches existing semantics.

Hugetlb MADV_REMOVE semantics are unfortunate and we should document the
exception.

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists