[lkml]   [2022]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2 2/7] mm/vmalloc.c: add flags to mark vm_map_ram area
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.:-

va = __find_vmap_area((unsigned long)addr, &vmap_area_root);
if (va)
va->flags &= ~VMAP_RAM;

> > 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.

 \ /
  Last update: 2022-12-19 14:02    [W:0.092 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site