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

[chi wu]

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