On Thu, 24 Dec 2020 10:16:45 -0800
Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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 = {
