On Mon, Dec 19, 2022 at 08:24:47PM +0800, Baoquan He wrote: > In fact, I should not do the checking, but do the clearing anyway. If I > change it as below, does it look better to you? > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 5e578563784a..369b848d097a 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2253,8 +2253,7 @@ void vm_unmap_ram(const void *mem, unsigned int count) > spin_lock(&vmap_area_lock); > va = __find_vmap_area((unsigned long)addr, &vmap_area_root); > BUG_ON(!va); > - if (va) > - va->flags &= ~VMAP_RAM; > + va->flags &= ~VMAP_RAM; > spin_unlock(&vmap_area_lock); > debug_check_no_locks_freed((void *)va->va_start, > (va->va_end - va->va_start)); This is better as it avoids the slightly contradictory situation of checking for a condition we've asserted is not the case, but I would still far prefer keeping this as-is and placing the unlock before the BUG_ON(). This avoids explicitly and knowingly holding a lock over a BUG_ON() and is simple to implement, e.g.:- spin_lock(&vmap_area_lock); va = __find_vmap_area((unsigned long)addr, &vmap_area_root); if (va) va->flags &= ~VMAP_RAM; spin_unlock(&vmap_area_lock); BUG_ON(!va); > > You are at this point clearing the VMAP_RAM flag though, so if it is unimportant > > what the flags are after this call, why are you clearing this one? > > With my understanding, We had better do the clearing. Currently, from > the code, not doing the clearing won't cause issue. If possible, I would > like to clear it as below draft code. > Sure, it seems appropriate to clear it, I'm just unsure as to why you aren't just clearing both flags? Perhaps just set va->flags = 0? > > > > It is just a little confusing, I wonder whether the VMAP_BLOCK flag is necessary > > at all, is it possible to just treat a non-VMAP_BLOCK VMAP_RAM area as if it > > were simply a fully occupied block? Do we gain much by the distinction? > > Yeah, VMAP_BLOCK flag is necessary. vmap_block contains used region, > or dirty/free regions. While the non-vmap_blcok vm_map_ram area is > similar with the non vm_map_ram area. When reading out vm_map_ram > regions, vmap_block regions need be treated differently. OK looking through again closely I see you're absolutely right, I wondered whether you could somehow make a non-VMAP_BLOCK vread() operation be equivalent to a block one (only across the whole mapping), but I don't think you can.
