Messages in this thread | | | Subject | Re: [RFC][PATCH 0/1] kmemleak: Fix false positive with alias | From | Catalin Marinas <> | Date | Fri, 17 Sep 2010 14:14:15 +0100 |
| |
On Mon, 2010-08-30 at 13:30 -0700, Paul E. McKenney wrote: > On Fri, Aug 27, 2010 at 09:12:24AM +0300, Hiroshi DOYU wrote: > > > +static struct kmemleak_object *find_and_get_object(unsigned long ptr, int alias) > > > +{ > > > + struct kmemleak_object *object; > > > + > > > + rcu_read_lock(); > > > + object = __find_and_get_object(ptr, alias); > > > + rcu_read_unlock(); > > > + > > > + return object; > > > +} > > > + > > > +static struct kmemleak_object *__find_and_get_alias(unsigned long ptr, int alias) > > > +{ > > > + struct kmemleak_object *object = NULL; > > > + struct kmemleak_alias *ao = NULL; > > > + struct prio_tree_node *node; > > > + struct prio_tree_iter iter; > > > + unsigned long flags; > > > + > > > + read_lock_irqsave(&kmemleak_lock, flags); > > If we hold this readlock, how is RCU helping us? Or are there updates > that do not write-hold kmemleak_lock?
These kind of constructs are already in the existing kmemleak code. Kmemleak uses the slab allocator itself but it also gets called from the slab allocator for other memory allocation/freeing. To avoid a race on the kfree path, kmemleak objects are freed later via he RCU.
The read_lock is used for the prio_tree access/modifications but the rcu_lock protects an additional get_object() call.
> > > + prio_tree_iter_init(&iter, &alias_tree_root, ptr, ptr); > > > + node = prio_tree_next(&iter); > > > + if (node) { > > > + ao = prio_tree_entry(node, struct kmemleak_alias, tree_node); > > > + if (!alias && to_address(ao) != ptr) { > > > + kmemleak_warn("Found object by alias"); > > > + ao = NULL; > > > + } > > > + } > > > + > > > + read_unlock_irqrestore(&kmemleak_lock, flags); > > > + > > > + if (ao && get_object(ao->object)) > > > + object = ao->object; > > > + > > > + return object; > > > +} [...] > > > +static void __delete_alias_object(struct kmemleak_object *object) > > > +{ > > > + prio_tree_remove(&alias_tree_root, &object->alias->tree_node); > > > + list_del_rcu(&object->alias->alias_list); > > Don't we need an RCU grace period here, based on either synchronize_rcu() > or call_rcu()? Perhaps calling free_object_rcu(), though perhaps it frees > up more than you would like.
You may be right here. The kmemleak objects (not the aliases introduced by this patch) are freed later by the put_object() call via a call_rcu().
Anyway, I don't think its worth pursuing this approach for handling address aliases (e.g. virt_to_phys) because of the performance hit Hiroshi is seeing.
-- Catalin
| |