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