Re: [PATCH] lib/kobject: Simplify checking of parameters in kobj_ns_type_register.

From: chi wu
Date: Fri Jun 17 2022 - 05:56:07 EST


Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 于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@xxxxxxxxx>
> > ---
> > 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