On 10/2/18 10:47 PM, Daniel Black wrote:
> On Mon, 1 Oct 2018 15:11:32 -0700
> Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
>> On 9/29/18 10:46 PM, Daniel Black wrote:
>> <snip>
>> > diff --git a/mm/madvise.c b/mm/madvise.c
>> > index 972a9eaa898b..71d21df2a3f3 100644
>> > --- a/mm/madvise.c
>> > +++ b/mm/madvise.c
>> > @@ -96,7 +96,7 @@ static long madvise_behavior(struct
>> > vm_area_struct *vma, new_flags |= VM_DONTDUMP;
>> > break;
>> > case MADV_DODUMP:
>> > - if (new_flags & VM_SPECIAL) {
>> > + if (!is_vm_hugetlb_page(vma) && new_flags &
>> > VM_SPECIAL) {
>>
>> Thanks Daniel,
>>
>> This is certainly a regression. My only question is whether this
>> condition should be more specific and test the default hugetlb vma
>> flags (VM_DONTEXPAND | VM_HUGETLB).
>
>> Or, whether simply checking
>> VM_HUGETLB as you have done above is sufficient.
>
> The is_vm_hugetlb_page() function seems widely used elsewhere for that
> single purpose.
>
>> Only reason for
>> concern is that I am not 100% certain other VM_SPECIAL flags could
>> not be set in VM_HUGETLB vma.
>
> They might be, but being a VM_HUGETLB flag is the main criteria for
> being able to madvise(DODUMP) on the memory. It highlight its user
> memory for the user to do as they wish.
>
> When 314e51b9851b was added, it seemed the direction was to kill of the
> VM_RESERVED, now a few years later VM_SPECIAL seems to be replacing
> this. I think it would be better to preserve the original goal and
> keep flags having a single meaning.
>
> The purpose in 0103bd16fb90 as I surmise it, is that VM_IO | VM_PFNMAP
> | VM_MIXEDMAP are the true things that want to be prevented from having
> madvise(DO_DUMP) on them, based on frequent use of DONT_DUMP with those
> memory pages. Was VM_DONTEXPAND an intentional inclusion there it did it
> just get included with VM_SPECIAL?
>
> Either way, I've tried to keep to the principles of the
> is_vm_hugetlb_page function being the authoritative source of a HUGETLB
> page.
>
>> Perhaps Konstantin has an opinion as he did a bunch of the vm_flag
>> reorg.
>>
>
> Thanks for the review.
>
Thanks for the explanation. I was thinking of the case where hugetlb pages
could be used for RDMA, and wanted to make sure those drivers were not doing
anything to vm_flags. A quick look reveals nothing special happening.
You can add,
Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
Adding Andrew on Cc:
--
Mike Kravetz
