Re: [patch] mm, hugetlb_cgroup: suppress SIGBUS when hugetlb_cgroup charge fails

From: David Rientjes <rientjes@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Mike Kravetz <mike.kravetz@oracle.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
	Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [patch] mm, hugetlb_cgroup: suppress SIGBUS when hugetlb_cgroup charge fails
Date: Fri, 25 May 2018 13:59:40 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.1805251356570.7798@chino.kir.corp.google.com> (raw)
In-Reply-To: <20180525134459.5c6f8e06f55307f72b95a901@linux-foundation.org>

On Fri, 25 May 2018, Andrew Morton wrote:

> On Fri, 25 May 2018 13:16:45 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> 
> > When charging to a hugetlb_cgroup fails, alloc_huge_page() returns
> > ERR_PTR(-ENOSPC) which will cause VM_FAULT_SIGBUS to be returned to the
> > page fault handler.
> > 
> > Instead, return the proper error code, ERR_PTR(-ENOMEM), so VM_FAULT_OOM
> > is handled correctly.  This is consistent with failing mem cgroup charges
> > in the non-hugetlb fault path.
> > 
> > At the same time, restructure the return paths of alloc_huge_page() so it
> > is consistent.
> 
> Patch doesn't appear to match the changelog?
> 

In what way?

> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -2006,8 +2006,10 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
> >  	 * code of zero indicates a reservation exists (no change).
> >  	 */
> >  	map_chg = gbl_chg = vma_needs_reservation(h, vma, addr);
> > -	if (map_chg < 0)
> > -		return ERR_PTR(-ENOMEM);
> > +	if (map_chg < 0) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> 
> This doesn't change the return value.
> 

This, and the subsequent comments, are referring to the third paragraph of 
the changelog.

The functional part of the change is for the 
hugetlb_cgroup_charge_cgroup() return value that is now actually used.

> >  	/*
> >  	 * Processes that did not create the mapping will have no
> > @@ -2019,8 +2021,8 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
> >  	if (map_chg || avoid_reserve) {
> >  		gbl_chg = hugepage_subpool_get_pages(spool, 1);
> >  		if (gbl_chg < 0) {
> > -			vma_end_reservation(h, vma, addr);
> > -			return ERR_PTR(-ENOSPC);
> > +			ret = -ENOSPC;
> > +			goto out_reservation;
> >  		}
> 
> Nor does this.
>  
> >  		/*
> > @@ -2049,8 +2051,10 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
> >  	if (!page) {
> >  		spin_unlock(&hugetlb_lock);
> >  		page = alloc_buddy_huge_page_with_mpol(h, vma, addr);
> > -		if (!page)
> > +		if (!page) {
> > +			ret = -ENOSPC;
> >  			goto out_uncharge_cgroup;
> > +		}
> 
> Nor does this.
> 
> >  		if (!avoid_reserve && vma_has_reserves(vma, gbl_chg)) {
> >  			SetPagePrivate(page);
> >  			h->resv_huge_pages--;
> > @@ -2087,8 +2091,10 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
> >  out_subpool_put:
> >  	if (map_chg || avoid_reserve)
> >  		hugepage_subpool_put_pages(spool, 1);
> > +out_reservation:
> >  	vma_end_reservation(h, vma, addr);
> > -	return ERR_PTR(-ENOSPC);
> > +out:
> > +	return ERR_PTR(ret);
> >  }
> >  
> 
> It would be nice if you could add a comment over alloc_huge_page()
> explaining the return values (at least).  Why sometimes ENOMEM, other
> times ENOSPC?
> 

The ENOSPC is used to specifically induce a VM_FAULT_SIGBUS, which 
Documentation/vm/hugetlbfs_reserv.txt specifies is how faults are handled 
if no hugetlb pages are available.