vm_reserv: extract common gap-checking code

[dougm]

Summary

vm_reserv_alloc_contig and vm_reserv_alloc_page both have code to determine whether a new reservation will fit between the predecessor and successor. Extract this code into a separate function rather than having it appear twice. Optimize slightly to avoid checking object size limit when there is a successor, and to avoid checking the rightcap at all when the size to be allocated is a multiple of reservation size.

[markj]

sys/vm/vm_reserv.c

589

Shouldn't rightcap also be clamped to object->size (for non-anonymous objects) in the msucc != NULL case?

742

I think this comment should be preserved.

[dougm]

sys/vm/vm_reserv.c

589

If msucc != NULL, then rightcap is clamped to either msucc->pindex or rv->pindex, both of which are less than object->size.

[dougm]

Add/restore some comments.

[markj]

sys/vm/vm_reserv.c

555

The return value of this function doesn't directly answer this question, so I'd explain here what the value represents.

560

Why add an explicit inline annotation?

579

Suppose mpred == NULL and minpages is a multiple of VM_LEVEL_0_NPAGES. This check means that we won't check the successor, but I can't see how that's correct.

To give an example, suppose we're allocating 2*VM_LEVEL_0_NPAGES pages starting at pindex 0. Suppose msucc->pindex == VM_LEVEL_0_NPAGES. We can thus only allocate one reservation, so should return VM_LEVEL_0_NPAGES.

[dougm]

sys/vm/vm_reserv.c

560

So the initial computation of allocpages can be done at compile time when the minpages parameter is 1. But it's not that important to me.

579

Consider the current code for the scenario you describe, where in vm_reserv_alloc_contig, where minpages is a multiple of VM_LEVEL_0_NPAGES. Then minpages == maxpages, and if the msucc is checked and first + maxpages exceeds rightcap, minpages is assigned to allocpages, which does not fix the problem.

I started drafting an email a few nights ago about how I thought I'd found a bug. But then I thought about it some more.

This function assumes that minpages can safely be allocated. Before this function is called, somebody determined that pages from pindex to pindex+npages-1 don't currently exist. So, when minpages is a multiple of VM_LEVEL_0_NPAGES, we know that the rightcap limit won't be violated, and there's no need to check the successor.

[dougm]

Seek to improve the function name and header comment.

[dougm]

Edit function header comment slightly.

[dougm]

Rename the function and de-inline it. Modify comments.

[markj]

sys/vm/vm_reserv.c

557

This sentence refers to local variables, so would make more sense within the function body.

760–761

[dougm]

Update comments.

sys/vm/vm_reserv.c

 static __inline boolean_t
 vm_reserv_has_pindex(vm_reserv_t rv, vm_pindex_t pindex)
 {
 
         return (((pindex - rv->pindex) & ~(VM_LEVEL_0_NPAGES - 1)) == 0);
 }
 
 /*
+ * How many pages should be in a new allocation that starts at the first page of
+ * the reservation superpage that contains 'first', fits between the allocations
+ * that include 'mpred' and 'msucc', fits within 'object', includes at least
+ * 'minpages' pages, and tries to include every allocated page in a superpage?
+ *
+ * We must synchronize with the reserv object lock to protect the pindex/object
+ * of the resulting reservations against rename while we are inspecting.
+ */
+static u_long
+vm_reserv_num_alloc_pages(vm_object_t object, vm_pindex_t first,
+    u_long minpages, vm_page_t mpred, vm_page_t msucc)
+{
+        vm_pindex_t leftcap, rightcap;
+        vm_reserv_t rv;
+        u_int allocpages;
+
+        allocpages = roundup2(minpages, VM_LEVEL_0_NPAGES);
+
+        vm_reserv_object_lock(object);
+        if (mpred != NULL) {
+                if ((rv = vm_reserv_from_page(mpred))->object != object)
+                        leftcap = mpred->pindex + 1;
+                else
+                        leftcap = rv->pindex + VM_LEVEL_0_NPAGES;
+                if (leftcap > first)
+                        allocpages = 0;
+        }
+        if (minpages < allocpages) {
+                if (msucc == NULL) {
+                        /*
+                         * Would the last new reservation extend past the end of
+                         * the object?
+                         *
+                         * If the object is unlikely to grow don't allocate a
+                         * reservation for the tail.
+                         */
+                        if ((object->flags & OBJ_ANON) == 0)
+                                rightcap = object->size;
+                        else
+                                rightcap = OBJ_MAX_SIZE;
+                } else {
+                        /*
+                         * Would the last new reservation extend past the start
+                         * of another page or reservation?
+                         *
+                         * If the object would, don't allocate a reservation for
+                         * the tail.
+                         */
+                        if ((rv = vm_reserv_from_page(msucc))->object != object)
+                                rightcap = msucc->pindex;
+                        else
+                                rightcap = rv->pindex;
+                }
+                if (first + allocpages > rightcap) {
+                        /*
+                         * A reservation for the last of the requested pages
+                         * will not fit.  Reduce the size of the upcoming
+                         * allocation accordingly.
+                         */
+                        allocpages = minpages;
+                }
+        }
+        vm_reserv_object_unlock(object);
+        return (allocpages);
+}
+
+/*
  * Increases the given reservation's population count.  Moves the reservation
  * to the tail of the partially populated reservation queue.
  */
 static void
 vm_reserv_populate(vm_reserv_t rv, int index)
 {
 
         vm_reserv_assert_locked(rv);

 vm_page_t
 vm_reserv_alloc_contig(vm_object_t object, vm_pindex_t pindex, int domain,
     int req, vm_page_t mpred, u_long npages, vm_paddr_t low, vm_paddr_t high,
     u_long alignment, vm_paddr_t boundary)
 {
         struct vm_domain *vmd;
         vm_paddr_t pa, size;
         vm_page_t m, m_ret, msucc;
-        vm_pindex_t first, leftcap, rightcap;
+        vm_pindex_t first;
         vm_reserv_t rv;
-        u_long allocpages, maxpages, minpages;
+        u_long allocpages;
         int i, index, n;
 
         VM_OBJECT_ASSERT_WLOCKED(object);
         KASSERT(npages != 0, ("vm_reserv_alloc_contig: npages is 0"));
 
         /*
          * Is a reservation fundamentally impossible?
          */
if (rv != NULL) {
                 vm_reserv_unlock(rv);
                 return (m);
 out:
                 vm_reserv_unlock(rv);
                 return (NULL);
         }
 
         /*
-         * Could at least one reservation fit between the first index to the
-         * left that can be used ("leftcap") and the first index to the right
-         * that cannot be used ("rightcap")?
-         *
-         * We must synchronize with the reserv object lock to protect the
-         * pindex/object of the resulting reservations against rename while
-         * we are inspecting.
+         * Check whether an allocation including at least one reservation can
+         * fit between mpred and msucc.
          */
         first = pindex - VM_RESERV_INDEX(object, pindex);
-        minpages = VM_RESERV_INDEX(object, pindex) + npages;
-        maxpages = roundup2(minpages, VM_LEVEL_0_NPAGES);
-        allocpages = maxpages;
-        vm_reserv_object_lock(object);
-        if (mpred != NULL) {
-                if ((rv = vm_reserv_from_page(mpred))->object != object)
-                        leftcap = mpred->pindex + 1;
-                else
-                        leftcap = rv->pindex + VM_LEVEL_0_NPAGES;
-                if (leftcap > first) {
-                        vm_reserv_object_unlock(object);
-                        return (NULL);
-                }
-        }
-        if (msucc != NULL) {
-                if ((rv = vm_reserv_from_page(msucc))->object != object)
-                        rightcap = msucc->pindex;
-                else
-                        rightcap = rv->pindex;
-                if (first + maxpages > rightcap) {
-                        if (maxpages == VM_LEVEL_0_NPAGES) {
-                                vm_reserv_object_unlock(object);
-                                return (NULL);
-                        }
+        allocpages = vm_reserv_num_alloc_pages(object, first,
+            VM_RESERV_INDEX(object, pindex) + npages, mpred, msucc);
+        if (allocpages < VM_LEVEL_0_NPAGES)
 
-                        /*
-                         * At least one reservation will fit between "leftcap"
-                         * and "rightcap".  However, a reservation for the
-                         * last of the requested pages will not fit.  Reduce
-                         * the size of the upcoming allocation accordingly.
-                         */
-                        allocpages = minpages;
-                }
-        }
-        vm_reserv_object_unlock(object);
-
-        /*
-         * Would the last new reservation extend past the end of the object?
-         *
-         * If the object is unlikely to grow don't allocate a reservation for
-         * the tail.
-         */
-        if ((object->flags & OBJ_ANON) == 0 &&
-            first + maxpages > object->size) {
-                if (maxpages == VM_LEVEL_0_NPAGES)
-                        return (NULL);
-                allocpages = minpages;
-        }
-
-        /*
          * Allocate the physical pages.  The alignment and boundary specified
          * for this allocation may be different from the alignment and
          * boundary specified for the requested pages.  For instance, the
          * specified index may not be the first page within the first new
          * reservation.
          */
         m = NULL;
         vmd = VM_DOMAIN(domain);

  * The object must be locked.
  */
 vm_page_t
 vm_reserv_alloc_page(vm_object_t object, vm_pindex_t pindex, int domain,
     int req, vm_page_t mpred)
 {
         struct vm_domain *vmd;
         vm_page_t m, msucc;
-        vm_pindex_t first, leftcap, rightcap;
+        vm_pindex_t first;
         vm_reserv_t rv;
         int index;
 
         VM_OBJECT_ASSERT_WLOCKED(object);
 
         /*
          * Is a reservation fundamentally impossible?
          */
if (rv != NULL) {
                 else
                         vm_reserv_populate(rv, index);
 out:
                 vm_reserv_unlock(rv);
                 return (m);
         }
 
         /*
-         * Could a reservation fit between the first index to the left that
-         * can be used and the first index to the right that cannot be used?
-         *
-         * We must synchronize with the reserv object lock to protect the
-         * pindex/object of the resulting reservations against rename while
-         * we are inspecting.
+         * Check whether an allocation including reservations can fit
+         * between mpred and msucc.
          */
         first = pindex - VM_RESERV_INDEX(object, pindex);
-        vm_reserv_object_lock(object);
-        if (mpred != NULL) {
-                if ((rv = vm_reserv_from_page(mpred))->object != object)
-                        leftcap = mpred->pindex + 1;
-                else
-                        leftcap = rv->pindex + VM_LEVEL_0_NPAGES;
-                if (leftcap > first) {
-                        vm_reserv_object_unlock(object);
-                        return (NULL);
-                }
-        }
-        if (msucc != NULL) {
-                if ((rv = vm_reserv_from_page(msucc))->object != object)
-                        rightcap = msucc->pindex;
-                else
-                        rightcap = rv->pindex;
-                if (first + VM_LEVEL_0_NPAGES > rightcap) {
-                        vm_reserv_object_unlock(object);
-                        return (NULL);
-                }
-        }
-        vm_reserv_object_unlock(object);
-
-        /*
-         * Would the last new reservation extend past the end of the object?
-         *
-         * If the object is unlikely to grow don't allocate a reservation for
-         * the tail.
-         */
-        if ((object->flags & OBJ_ANON) == 0 &&
-            first + VM_LEVEL_0_NPAGES > object->size)
+        if (vm_reserv_num_alloc_pages(object, first, 1, mpred, msucc) < 
+            VM_LEVEL_0_NPAGES)
                 return (NULL);
 
         /*
          * Allocate and populate the new reservation.
          */
         m = NULL;
         vmd = VM_DOMAIN(domain);
         if (vm_domain_allocate(vmd, req, 1)) {