Messages in this thread Patch in this message | | | From | chi wu <> | Date | Fri, 17 Jun 2022 17:55:46 +0800 | Subject | Re: [PATCH] lib/kobject: Simplify checking of parameters in kobj_ns_type_register. |
| |
Greg KH <gregkh@linuxfoundation.org> 于2022年6月17日周五 16:49写道: > > On Fri, Jun 17, 2022 at 04:38:32PM +0800, wuchi wrote: > > 1. Merge checking of following code: > > if (type >= KOBJ_NS_TYPES) > > ... > > if (type <= KOBJ_NS_TYPE_NONE) > > Why?
As lib/kobjecct.c following code in kobj_ns_type_register(): L998: enum kobj_ns_type type = ops->type; L999: int error; L996: int kobj_ns_type_register(const struct kobj_ns_type_operations *ops) L1001: spin_lock(&kobj_ns_type_lock);
L1003: error = -EINVAL; L1004: if (type >= KOBJ_NS_TYPES) L1005: goto out;
L1007: error = -EINVAL; L1008: if (type <= KOBJ_NS_TYPE_NONE) L1009: goto out;
L1011: error = -EBUSY; ... L1018: out: L1019: spin_unlock(&kobj_ns_type_lock);
L1003~L1005 and L1007~1009 set twice vaule and do the similar checking. it is redundant code, So just simplify that.
> > > 2. Move the checking of parameters out of critical section, there is > > no need to check that with spinlock. > > Why does it matter? >
[2]: The result is ok, but just want to reduce the critical section code, as @ops->type is input arg and KOBJ_NS_{TYPES|TYPE_NONE} is enum, so it seems to be better to put the checking out of spinlock.
> > > > > Signed-off-by: wuchi <wuchi.zero@gmail.com> > > --- > > lib/kobject.c | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > diff --git a/lib/kobject.c b/lib/kobject.c > > index 5f0e71ab292c..6a8b009682b8 100644 > > --- a/lib/kobject.c > > +++ b/lib/kobject.c > > @@ -996,19 +996,13 @@ static const struct kobj_ns_type_operations *kobj_ns_ops_tbl[KOBJ_NS_TYPES]; > > int kobj_ns_type_register(const struct kobj_ns_type_operations *ops) > > { > > enum kobj_ns_type type = ops->type; > > - int error; > > - > > - spin_lock(&kobj_ns_type_lock); > > + int error = -EBUSY; > > > > - error = -EINVAL; > > - if (type >= KOBJ_NS_TYPES) > > - goto out; > > + if (unlikely(type >= KOBJ_NS_TYPES || type <= KOBJ_NS_TYPE_NONE)) > > Why add unlikely? Did you measure the performance benifit? If not, > please never add likely/unlikely.
[3] Yes, just want to get some performance benifit, but i am sorry for that I didn't do the test, but just see the only call in netdev_kobject_init from net/core/net-sysfs.c: kobj_ns_type_register(&net_ns_type_operations); and the type of net_ns_type_operations is KOBJ_NS_TYPE_NET which is valid.
> > > + return -EINVAL; > > > > - error = -EINVAL; > > - if (type <= KOBJ_NS_TYPE_NONE) > > - goto out; > > + spin_lock(&kobj_ns_type_lock); > > > > - error = -EBUSY; > > > How did you test this? What is the benefit?
As the reply [2] [3] snippet, and just for a little bit of possible performance to reduce the preempt_disable time though it just called when system is bootting.
thanks for greg k-h's time
| |