Re: [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 24 Dec 2020 10:16:45 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:

> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > Add some asserts to make sure the state is consistent.
> >
> > Simplify and improve the readability of vm_free.
> >
> > Fixes: 3f6fee0d4da4 ("lib/vmalloc: vmalloc support for handling
> > allocation metadata")
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> >   lib/vmalloc.c | 20 +++++++++++---------
> >   1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> > index 986a34c..7a49adf 100644
> > --- a/lib/vmalloc.c
> > +++ b/lib/vmalloc.c
> > @@ -162,13 +162,14 @@ static void *vm_memalign(size_t alignment,
> > size_t size) static void vm_free(void *mem)
> >   {
> >   	struct metadata *m;
> > -	uintptr_t ptr, end;
> > +	uintptr_t ptr, page, i;
> >   
> >   	/* the pointer is not page-aligned, it was a single-page
> > allocation */  
> 
> 
> Do we need an assert() for 'mem' if it is NULL for some reason ?

I thought the NULL case was handled in alloc.c, but I was mistaken.
In any case, the metadata will probably not match and trigger the other
asserts.

I will still add assert(mem), though, as it looks cleaner

> >   	if (!IS_ALIGNED((uintptr_t)mem, PAGE_SIZE)) {
> >   		assert(GET_MAGIC(mem) == VM_MAGIC);
> > -		ptr = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
> > -		free_page(phys_to_virt(ptr));
> > +		page = virt_to_pte_phys(page_root, mem) &
> > PAGE_MASK;
> > +		assert(page);
> > +		free_page(phys_to_virt(page));
> >   		return;
> >   	}
> >   
> > @@ -176,13 +177,14 @@ static void vm_free(void *mem)
> >   	m = GET_METADATA(mem);
> >   	assert(m->magic == VM_MAGIC);
> >   	assert(m->npages > 0);
> > +	assert(m->npages < BIT_ULL(BITS_PER_LONG - PAGE_SHIFT));  
> 
> 
> NIT:  Combine the two assert()s for 'npages' perhaps ?

no, when one assert is triggered, it will print a useful message; if
you combine the two asserts you don't know why exactly it failed.

it's functionally equivalent, but it has a better user experience.

> >   	/* free all the pages including the metadata page */
> > -	ptr = (uintptr_t)mem - PAGE_SIZE;
> > -	end = ptr + m->npages * PAGE_SIZE;
> > -	for ( ; ptr < end; ptr += PAGE_SIZE)
> > -		free_page(phys_to_virt(virt_to_pte_phys(page_root,
> > (void *)ptr)));
> > -	/* free the last one separately to avoid overflow issues */
> > -	free_page(phys_to_virt(virt_to_pte_phys(page_root, (void
> > *)ptr)));
> > +	ptr = (uintptr_t)m & PAGE_MASK;
> > +	for (i = 0 ; i < m->npages + 1; i++, ptr += PAGE_SIZE) {
> > +		page = virt_to_pte_phys(page_root, (void *)ptr) &
> > PAGE_MASK;
> > +		assert(page);
> > +		free_page(phys_to_virt(page));
> > +	}
> >   }
> >   
> >   static struct alloc_ops vmalloc_ops = {