Messages in this thread |  | | Date | Tue, 7 Jun 2022 22:34:21 +0800 | Subject | Re: [PATCH v2 2/4] mm: kmemleak: add rbtree for objects allocated with physical address | From | Patrick Wang <> |
| |
On 2022/6/6 22:38, Catalin Marinas wrote: > On Fri, Jun 03, 2022 at 11:54:13AM +0800, Patrick Wang wrote: >> @@ -536,27 +543,32 @@ static struct kmemleak_object *find_and_get_object(unsigned long ptr, int alias) >> } >> >> /* >> - * Remove an object from the object_tree_root and object_list. Must be called >> - * with the kmemleak_lock held _if_ kmemleak is still enabled. >> + * Remove an object from the object_tree_root (or object_phys_tree_root) >> + * and object_list. Must be called with the kmemleak_lock held _if_ kmemleak >> + * is still enabled. >> */ >> static void __remove_object(struct kmemleak_object *object) >> { >> - rb_erase(&object->rb_node, &object_tree_root); >> + rb_erase(&object->rb_node, object->flags & OBJECT_PHYS ? >> + &object_phys_tree_root : >> + &object_tree_root); > > This pattern appears in a few place, I guess it's better with a macro, > say get_object_tree_root(object). But see how many are left, I have some > comments below on reducing the diff.
Will do.
> >> @@ -709,12 +724,12 @@ static void delete_object_full(unsigned long ptr) >> * delete it. If the memory block is partially freed, the function may create >> * additional metadata for the remaining parts of the block. >> */ >> -static void delete_object_part(unsigned long ptr, size_t size) >> +static void delete_object_part(unsigned long ptr, size_t size, bool is_phys) >> { >> struct kmemleak_object *object; >> unsigned long start, end; >> >> - object = find_and_remove_object(ptr, 1); >> + object = find_and_remove_object(ptr, 1, is_phys); >> if (!object) { >> #ifdef DEBUG >> kmemleak_warn("Partially freeing unknown object at 0x%08lx (size %zu)\n", > > The previous patch introduced a check on object->flags for > delete_object_part(). I think you can just use is_phys directly now when > calling create_object().
Will do.
> >> @@ -1275,7 +1290,7 @@ static void scan_block(void *_start, void *_end, >> * is still present in object_tree_root and object_list >> * (with updates protected by kmemleak_lock). >> */ >> - object = lookup_object(pointer, 1); >> + object = lookup_object(pointer, 1, false); >> if (!object) >> continue; >> if (object == scanned) >> @@ -1299,7 +1314,7 @@ static void scan_block(void *_start, void *_end, >> raw_spin_unlock(&object->lock); >> >> if (excess_ref) { >> - object = lookup_object(excess_ref, 0); >> + object = lookup_object(excess_ref, 0, false); >> if (!object) >> continue; >> if (object == scanned) >> @@ -1728,7 +1743,7 @@ static int dump_str_object_info(const char *str) >> >> if (kstrtoul(str, 0, &addr)) >> return -EINVAL; >> - object = find_and_get_object(addr, 0); >> + object = find_and_get_object(addr, 0, false); >> if (!object) { >> pr_info("Unknown object at 0x%08lx\n", addr); >> return -EINVAL; > > I think find_and_get_object() is never called on a phys object, so you > can probably simplify these a bit. Just add an is_phys argument where > strictly necessary and maybe even add a separate function like > lookup_object_phys() to reduce the other changes.
Will add lookup_object_phys() function and find_and_get_object_phys() function. The find_and_get_object() function is also called in many places.
Thanks, Patrick
|  |