From: David Hildenbrand <david@redhat.com>
To: Mike Kravetz <mike.kravetz@oracle.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Cc: Naoya Horiguchi <naoya.horiguchi@linux.dev>,
Axel Rasmussen <axelrasmussen@google.com>,
Mina Almasry <almasrymina@google.com>,
Michal Hocko <mhocko@suse.com>, Peter Xu <peterx@redhat.com>,
Andrea Arcangeli <aarcange@redhat.com>,
Shuah Khan <shuah@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings
Date: Fri, 4 Feb 2022 09:35:01 +0100 [thread overview]
Message-ID: <6a82ea68-6e1e-8f5a-ca89-6744fc896a0b@redhat.com> (raw)
In-Reply-To: <7b174c48-d368-43ba-7eab-13719a0d15ef@oracle.com>
>>> + /*
>>> + * 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
next prev parent reply other threads:[~2022-02-04 8:35 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-02 1:40 [PATCH v2 0/3] Add hugetlb MADV_DONTNEED support Mike Kravetz
2022-02-02 1:40 ` [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings Mike Kravetz
2022-02-02 8:14 ` David Hildenbrand
2022-02-02 19:32 ` Mike Kravetz
2022-02-04 8:35 ` David Hildenbrand [this message]
2022-02-07 23:47 ` Mike Kravetz
2022-02-10 13:09 ` David Hildenbrand
2022-02-10 22:11 ` Mike Kravetz
2022-02-11 8:43 ` David Hildenbrand
2022-02-10 3:21 ` Peter Xu
2022-02-10 21:36 ` Mike Kravetz
2022-02-11 2:28 ` Peter Xu
2022-02-11 19:08 ` Axel Rasmussen
2022-02-11 19:18 ` Mike Kravetz
2022-02-02 1:40 ` [PATCH v2 2/3] selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test Mike Kravetz
2022-02-02 1:40 ` [PATCH v2 3/3] userfaultfd/selftests: enable huegtlb remap and remove event testing Mike Kravetz
2022-02-02 6:11 ` Mike Rapoport
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6a82ea68-6e1e-8f5a-ca89-6744fc896a0b@redhat.com \
--to=david@redhat.com \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=almasrymina@google.com \
--cc=axelrasmussen@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=mike.kravetz@oracle.com \
--cc=naoya.horiguchi@linux.dev \
--cc=peterx@redhat.com \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).