Re: [PATCH v7 1/2] mm/vmalloc: Moved macros with no functional change happened

[Baoquan He]

On 03/06/24 at 08:01pm, Uladzislau Rezki wrote:
> On Fri, Mar 01, 2024 at 10:54:16AM -0500, rulinhuang wrote:
.....
> 
> Sorry for the late answer, i also just noticed this email. It was not in
> my inbox...
> 
> OK, now you move part of the per-cpu allocator on the top and leave
> another part down making it split. This is just for the:
> 
> BUG_ON(va_flags & VMAP_RAM);
> 
> VMAP_RAM macro. Do we really need this BUG_ON()?

Sorry, I suggested that when reviewing v5:
https://lore.kernel.org/all/ZdiltpK5fUvwVWtD@MiWiFi-R3L-srv/T/#u

About part of per-cpu kva allocator moving and the split making, I would
argue that we will have vmap_nodes defintion and basic helper functions
like addr_to_node_id() etc at top, and leave other part like
size_to_va_pool(), node_pool_add_va() etc down. These are similar.

While about whether we should add 'BUG_ON(va_flags & VMAP_RAM);', I am
not sure about it. When I suggested that, I am also hesitant. From the
current code, alloc_vmap_area() is called in below three functions, only
__get_vm_area_node() will pass the non-NULL vm. 
 new_vmap_block()     -|
 vm_map_ram()         ----> alloc_vmap_area()
 __get_vm_area_node() -|

It could be wrongly passed in the future? Only checking if vm is
non-NULL makes me feel a little unsafe. While I am fine if removing the
BUG_ON, because there's no worry in the current code. We can wait and
see in the future.

       if (vm) {
               BUG_ON(va_flags & VMAP_RAM);
               setup_vmalloc_vm(vm, va, flags, caller);
       }

Thanks
Baoquan