Thread (16 messages) 16 messages, 3 authors, 2015-07-21

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

From: Naoya Horiguchi <hidden>
Date: 2015-07-21 04:18:44
Also in: linux-mm, lkml

On Mon, Jul 20, 2015 at 10:50:12AM -0700, Mike Kravetz wrote:
...
quoted
...
quoted
@@ -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.
quoted
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@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help