Step 2 Was: more strict KPI for vm_pager_get_pages()

[Gleb Smirnoff]

  Konstantin,

On Wed, Jun 17, 2015 at 04:45:38PM +0300, Konstantin Belousov wrote:
K> > +++ sys/vm/vm_pager.c	(working copy)
K> > @@ -251,7 +251,88 @@ vm_pager_deallocate(object)
K> >  }
K> >  
K> >  /*
K> > - * vm_pager_get_pages() - inline, see vm/vm_pager.h
K> > + * Retrieve pages from the VM system in order to map them into an object
K> > + * ( or into VM space somewhere ).  If the pagein was successful, we
K> > + * must fully validate it.
K> > + */
K> > +int
K> > +vm_pager_get_pages(vm_object_t object, vm_page_t *m, int count, int reqpage)
K> > +{
K> > +	int r;
K> > +
K> > +	VM_OBJECT_ASSERT_WLOCKED(object);
K> > +	KASSERT(count > 0, ("%s: 0 count", __func__));
K> > +
K> > +#ifdef INVARIANTS
K> > +	/*
K> > +	 * All pages must be busied, not mapped, not valid (save the last one),
K> > +	 * not dirty and belong to the proper object.
K> > +	 */
K> > +	for (int i = 0 ; i < count; i++) {
K> > +		vm_page_assert_xbusied(m[i]);
K> > +		KASSERT(!pmap_page_is_mapped(m[i]),
K> > +		    ("%s: page %p is mapped", __func__, m[i]));
K> > +		KASSERT(m[i]->valid == 0 || i == count - 1,
K> > +		    ("%s: request for a valid page %p", __func__, m[i]));
K> Are you sure that e.g. the requested page cannot be partially valid ?
K> I am surprised.

All save the last one are always valid. This passed stress2 for me and pho at .

K> > +		KASSERT(m[i]->dirty == 0,
K> > +		    ("%s: page %p is dirty", __func__, m[i]));
K> > +		KASSERT(m[i]->object == object,
K> > +		    ("%s: wrong object %p/%p", __func__, object, m[i]->object));
K> > +	}
K> > +#endif
K> Create a helper function with the loop, and use it, instead of copying
K> the block twice.

Done.

K> > +
K> > +	r = (*pagertab[object->type]->pgo_getpages)(object, m, count, reqpage);
K> > +	if (r != VM_PAGER_OK)
K> > +		return (r);
K> > +
K> > +	/*
K> > +	 * If pager has replaced the page, assert that it had
K> > +	 * updated the array.
K> > +	 */
K> > +	KASSERT(m[reqpage] == vm_page_lookup(object, m[reqpage]->pindex),
K> > +	    ("%s: mismatch page %p pindex %ju", __func__,
K> > +	    m[reqpage], (uintmax_t )m[reqpage]->pindex - 1));
K> Why -1 ?
K> Need an empty line after assert, to deliniate the code which is commented
K> about.

That's an artifact from future patch. Fixed.

K> Also, you may assert that the requested page is still busied.

Done.

Updated patch attached.

-- 
Totus tuus, Glebius.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pagers.step.2.diff
Type: text/x-diff
Size: 6343 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20150617/f1d38c5b/attachment.diff>