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

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

[wuchi]

1. Merge checking of following code:
    if (type >= KOBJ_NS_TYPES)
    ...
    if (type <= KOBJ_NS_TYPE_NONE)

2. Move the checking of parameters out of critical section, there is
no need to check that with 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))
+		return -EINVAL;
 
-	error = -EINVAL;
-	if (type <= KOBJ_NS_TYPE_NONE)
-		goto out;
+	spin_lock(&kobj_ns_type_lock);
 
-	error = -EBUSY;
 	if (kobj_ns_ops_tbl[type])
 		goto out;
 
-- 
2.20.1

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

[Greg KH]

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?

> 2. Move the checking of parameters out of critical section, there is
> no need to check that with spinlock.

Why does it matter?


> 
> 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.

> +		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?

thanks,

greg k-h

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

[chi wu]

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