Messages in this thread | | | Date | Thu, 11 Apr 2013 15:53:40 +0200 | From | Veaceslav Falico <> | Subject | Re: [PATCH] module: add kset_obj_exists() and use it |
| |
On Thu, Apr 11, 2013 at 06:28:31AM -0700, Greg KH wrote: >On Thu, Apr 11, 2013 at 11:55:37AM +0200, Veaceslav Falico wrote: >> However, I think my patch still adds something good, cause now we have 2 >> cases where we basically do: >> >> k = kset_find_obj(); >> if (!k) >> return; >> kobject_put(k); >> >> which adds useless overhead (by using kobject_get()/kobject_put(), and >> kobject_release() - which is called from kobject_put()) - where we should >> only verify if there exists a kobject with the specified name. >> >> Should I resend it with a properly fixed commit message, or it's really not >> needed? > >I don't think it's really needed, there is no speed/overhead issue here >and you need to do the kobject_get/put stuff anyway if you are trying to >look at a kobject.
This is the point, actually, that we don't need to look at a kobject. We only need to know if it existed that time or not, here are those two examples of code:
static int mod_sysfs_init(struct module *mod) { int err; struct kobject *kobj;
...
kobj = kset_find_obj(module_kset, mod->name); if (kobj) { printk(KERN_ERR "%s: module is already loaded\n", mod->name); kobject_put(kobj); err = -EINVAL; goto out; }
...
So we just verify if there's a kobject with mod->name, and if it exists - _put() it back and return, otherwise do nothing (with it).
Same here:
static char *make_slot_name(const char *name) { ...
for (;;) { struct kobject *dup_slot; dup_slot = kset_find_obj(pci_slots_kset, new_name); if (!dup_slot) break; kobject_put(dup_slot);
...
We look if there exists a kobject named new_name in pci_slots_kset, if yes - free it and try another name, if not - then we're good to go.
In both examples we don't look at that kobject, and only uselessly _get()/_put() it. And it looks a bit ugly. After the patch, in both cases, it takes only one call to kset_obj_exists() to find out if the object exists at that time.
However, I have absolutely no knowledge/experience in this domain and might for sure be missing something. Sorry if it's the case.
> >thanks, > >greg k-h
| |