> > I would remove it, because it is really hard to mess it, there is only
> > one place also BUG_ON() is really a show stopper. I really appreciate
> > what rulinhuang <rulin.huang@xxxxxxxxx> is doing and i understand that
> > it might be not so easy.
>
> I agree, I was hesitant, now it firms up my mind.
>
> >
> > So, if we can avoid of moving the code, that looks to me that we can do,
> > if we can pass less arguments into alloc_vmap_area() since it is overloaded
> > that would be great.
>
> Agree too, less arguments is much better. While I personnally prefer the open
> coding a little bit like below. There is suspicion of excessive packaging in
> __pre/__post_setup_vmalloc_vm() wrapping. They are very simple and few
> assignments after all.
>
> ---
> mm/vmalloc.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 0fd8ebaad17b..0c738423976d 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1924,8 +1924,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> unsigned long align,
> unsigned long vstart, unsigned long vend,
> int node, gfp_t gfp_mask,
> - unsigned long va_flags, struct vm_struct *vm,
> - unsigned long flags, const void *caller)
> + unsigned long va_flags, struct vm_struct *vm)
> {
> struct vmap_node *vn;
> struct vmap_area *va;
> @@ -1988,8 +1987,11 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> va->vm = NULL;
> va->flags = (va_flags | vn_id);
>
> - if (vm)
> - setup_vmalloc_vm(vm, va, flags, caller);
> + if (vm) {
> + vm->addr = (void *)va->va_start;
> + vm->size = va->va_end - va->va_start;
> + va->vm = vm;
> + }
>
> vn = addr_to_node(va->va_start);
>
> @@ -2565,8 +2567,7 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask)
> va = alloc_vmap_area(VMAP_BLOCK_SIZE, VMAP_BLOCK_SIZE,
> VMALLOC_START, VMALLOC_END,
> node, gfp_mask,
> - VMAP_RAM|VMAP_BLOCK, NULL,
> - 0, NULL);
> + VMAP_RAM|VMAP_BLOCK, NULL);
> if (IS_ERR(va)) {
> kfree(vb);
> return ERR_CAST(va);
> @@ -2924,7 +2925,7 @@ void *vm_map_ram(struct page **pages, unsigned int count, int node)
> va = alloc_vmap_area(size, PAGE_SIZE,
> VMALLOC_START, VMALLOC_END,
> node, GFP_KERNEL, VMAP_RAM,
> - NULL, 0, NULL);
> + NULL);
> if (IS_ERR(va))
> return NULL;
>
> @@ -3063,7 +3064,10 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
> if (!(flags & VM_NO_GUARD))
> size += PAGE_SIZE;
>
> - va = alloc_vmap_area(size, align, start, end, node, gfp_mask, 0, area, flags, caller);
> + area->flags = flags;
> + area->caller = caller;
> +
> + va = alloc_vmap_area(size, align, start, end, node, gfp_mask, 0, area);
> if (IS_ERR(va)) {
> kfree(area);
> return NULL;
> --
> 2.41.0
>
Reviewed-by: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
Looks even better :) It can be applied on on top of:
[PATCH v8] mm/vmalloc: Eliminated the lock contention from twice to once
We are a bit ahead since v8 will be taken later. Anyway please use the
reviewed-by tag once you send a complete patch.
Thanks!
--
Uladzislau Rezki
