Re: [PATCH v3 01/10] mm/hugetlb: add cache of descriptors to resv_map for region_add

[Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>]

On Mon, Jul 20, 2015 at 10:50:12AM -0700, Mike Kravetz wrote:
...
> > ...
> >> @@ -3236,11 +3360,14 @@ retry:
> >>   	 * any allocations necessary to record that reservation occur outside
> >>   	 * the spinlock.
> >>   	 */
> >> -	if ((flags & FAULT_FLAG_WRITE) && !(vma->vm_flags & VM_SHARED))
> >> +	if ((flags & FAULT_FLAG_WRITE) && !(vma->vm_flags & VM_SHARED)) {
> >>   		if (vma_needs_reservation(h, vma, address) < 0) {
> >>   			ret = VM_FAULT_OOM;
> >>   			goto backout_unlocked;
> >>   		}
> >> +		/* Just decrements count, does not deallocate */
> >> +		vma_abort_reservation(h, vma, address);
> >> +	}
> > 
> > This is not "abort reservation" operation, but you use "abort reservation"
> > routine, which might confusing and makes future maintenance hard. I think
> > this should be done in a simplified variant of vma_commit_reservation()
> > (maybe just an alias of your vma_abort_reservation()) or fast path in
> > vma_commit_reservation().
> 
> I am struggling a bit with the names of these routines.  The
> routines in question are:
> 
> vma_needs_reservation - This is a wrapper for region_chg(), so the
> 	return value is the number of regions needed for the page.
> 	Since there is only one page, the routine effectively
> 	becomes a boolean.  Hence the name "needs".
> 
> vma_commit_reservation - This is a wrapper for region_add().  It
> 	must be called after a prior call to vma_needs_reservation
> 	and after actual allocation of the page.
> 
> We need a way to handle the case where vma_needs_reservation has
> been called, but the page allocation is not successful.  I chose
> the name vma_abort_reservation, but as noted (even in my comments)
> it is not an actual abort.
> 
> I am not sure if you are suggesting vma_commit_reservation() should
> handle this as a special case.  I think a separately named routine which
> indicates and end of the reservation/allocation process would be
> easier to understand.
> 
> What about changing the name vma_abort_reservation() to
> vma_end_reservation()?  This would indicate that the reservation/
> allocation process is ended.

OK, vma_end_reservation() sounds nice to me.

> > Thanks,
> > Naoya Horiguchi
> 
> Thank you for your reviews.

You're welcome :)

Naoya Horiguchi