| Message ID | 20130411095537.GC21320@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 11, 2013 at 03:53:40PM +0200, Veaceslav Falico wrote: > 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. But as your function does the same thing, logically it's the same code path :) Anyway, yes, I understand your point here, and in some new code I'm writing right now, we had to do much the same check as well. But as there are only 2 in-kernel users of this "pattern", I don't think it's justified to add a new api call for it, especially if it were to be misused as you were attempting to use it, which would only mask the real problem you were trying to solve. So, thanks for the idea, but for now, I'll pass. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 11, 2013 at 08:20:03AM -0700, Greg KH wrote: >On Thu, Apr 11, 2013 at 03:53:40PM +0200, Veaceslav Falico wrote: >> 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: ... >> >> 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. > >But as your function does the same thing, logically it's the same code >path :) > >Anyway, yes, I understand your point here, and in some new code I'm >writing right now, we had to do much the same check as well. But as >there are only 2 in-kernel users of this "pattern", I don't think it's >justified to add a new api call for it, especially if it were to be >misused as you were attempting to use it, which would only mask the real >problem you were trying to solve. Good point, it really might mask the real problem, as it would actually do for the initial race. > >So, thanks for the idea, but for now, I'll pass. Fair enough. Thank you for explaining :) > >thanks, > >greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/module.c b/kernel/module.c index d0afe23..8be6e97 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1063,6 +1063,9 @@ static ssize_t show_initstate(struct module_attribute *mattr, case MODULE_STATE_GOING: state = "going"; break; + case MODULE_STATE_UNFORMED: + state = "unformed"; + break; default: BUG(); }