Re: [PATCH] mm: madvise(MADV_DODUMP) allow hugetlbfs pages

[Mike Kravetz <mike.kravetz@oracle.com>]

On 10/2/18 10:47 PM, Daniel Black wrote:
> On Mon, 1 Oct 2018 15:11:32 -0700
> Mike Kravetz <mike.kravetz@oracle.com> 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@oracle.com>

Adding Andrew on Cc:
-- 
Mike Kravetz