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

[Daniel Black <daniel@linux.ibm.com>]

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.