lkml.org 
[lkml]   [2015]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 01/10] mm/hugetlb: add cache of descriptors to resv_map for region_add
Date
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

\
 
 \ /
  Last update: 2015-07-21 06:41    [W:0.277 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site