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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]