| Message ID | 20201216201200.255172-4-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix and improve the page allocator | expand |
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 ? > 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 ? > /* 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 = {
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 = {
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 */ 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)); /* 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 = {
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(-)