Need help understanding the logic of __cpuidle_set_driver

Need help understanding the logic of __cpuidle_set_driver

[Mohammad Merajul Islam Molla]

Hello,

I was looking into the code of drivers/cpuidle/driver.c. I have some
doubts regarding the implementation of __cpuidle_set_driver function
when CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined.

If CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined, the code for
__cpuidle_set_driver/__cpuidle_unset_driver looks as -

 39  * __cpuidle_unset_driver - unset per CPU driver variables.
 40  * @drv: a valid pointer to a struct cpuidle_driver
 41  *
 42  * For each CPU in the driver's CPU mask, unset the registered
driver per CPU
 43  * variable. If @drv is different from the registered driver, the
corresponding
 44  * variable is not cleared.
 45  */
 46 static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
 47 {
 48         int cpu;
 49
 50         for_each_cpu(cpu, drv->cpumask) {
 51
 52                 if (drv != __cpuidle_get_cpu_driver(cpu))
 53                         continue;
 54
 55                 per_cpu(cpuidle_drivers, cpu) = NULL;
 56         }
 57 }
 58
 59 /**
 60  * __cpuidle_set_driver - set per CPU driver variables for the given driver.
 61  * @drv: a valid pointer to a struct cpuidle_driver
 62  *
 63  * For each CPU in the driver's cpumask, unset the registered driver per CPU
 64  * to @drv.
 65  *
 66  * Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
 67  */
 68 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 69 {
 70         int cpu;
 71
 72         for_each_cpu(cpu, drv->cpumask) {
 73
 74                 if (__cpuidle_get_cpu_driver(cpu)) {
 75                         __cpuidle_unset_driver(drv);
 76                         return -EBUSY;
 77                 }
 78
 79                 per_cpu(cpuidle_drivers, cpu) = drv;
 80         }
 81
 82         return 0;
 83 }

Apparently, the comment should be - "set/register the driver per CPU
to @drv" instead of "unset the registered driver per CPU to @drv" in
case of __cpuidle_set_driver.

However, regarding the logic, I have a few doubts -

1. for each cpu in drv->cpumask, if there is already a driver
registered, its calling __cpuidle_unset_driver which loops over for
each cpu in drv->cpumask again. Isn't it unnecessary to do this nested
calls?

2. after calling __cpuidle_unset_driver, if drv equals already
registered driver, it sets per_cpu driver to null? Isn't it wrong when
we are trying to set to a new driver? Why do we need to unset and make
the driver null when we are returning EBUSY from __cpuidle_set_driver?

Would it be correct and cleaner if the code is written as below -

 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
 {
         int ret = -EBUSY;
         int cpu;

        for_each_cpu(cpu, drv->cpumask) {
                if (drv == __cpuidle_get_cpu_driver(cpu))    [if drv
is already the registered driver, do nothing]
                         continue;

                 per_cpu(cpuidle_drivers, cpu) = drv;  [if only drv !=
already registered driver, set per_cpu driver to drv and set ret 0]
                 ret = 0;
         }

         return ret;     [only if all cpus already had drv as
registered driver, return -EBUSY. Otherwise return 0]
 }


Please let me know your views.

--
Thanks,
-Meraj

Re: Need help understanding the logic of __cpuidle_set_driver

[AYAN KUMAR HALDER]

On Fri, Aug 15, 2014 at 3:50 PM, Mohammad Merajul Islam Molla
<meraj.enigma@gmail.com> wrote:
> Hello,
>
> I was looking into the code of drivers/cpuidle/driver.c. I have some
> doubts regarding the implementation of __cpuidle_set_driver function
> when CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined.
>
> If CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined, the code for
> __cpuidle_set_driver/__cpuidle_unset_driver looks as -
>
>  39  * __cpuidle_unset_driver - unset per CPU driver variables.
>  40  * @drv: a valid pointer to a struct cpuidle_driver
>  41  *
>  42  * For each CPU in the driver's CPU mask, unset the registered
> driver per CPU
>  43  * variable. If @drv is different from the registered driver, the
> corresponding
>  44  * variable is not cleared.
>  45  */
>  46 static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>  47 {
>  48         int cpu;
>  49
>  50         for_each_cpu(cpu, drv->cpumask) {
>  51
>  52                 if (drv != __cpuidle_get_cpu_driver(cpu))
>  53                         continue;
>  54
>  55                 per_cpu(cpuidle_drivers, cpu) = NULL;
>  56         }
>  57 }
>  58
>  59 /**
>  60  * __cpuidle_set_driver - set per CPU driver variables for the given driver.
>  61  * @drv: a valid pointer to a struct cpuidle_driver
>  62  *
>  63  * For each CPU in the driver's cpumask, unset the registered driver per CPU
>  64  * to @drv.
>  65  *
>  66  * Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
>  67  */
>  68 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  69 {
>  70         int cpu;
>  71
>  72         for_each_cpu(cpu, drv->cpumask) {
>  73
>  74                 if (__cpuidle_get_cpu_driver(cpu)) {
>  75                         __cpuidle_unset_driver(drv);
>  76                         return -EBUSY;
>  77                 }
>  78
>  79                 per_cpu(cpuidle_drivers, cpu) = drv;
>  80         }
>  81
>  82         return 0;
>  83 }
>
> Apparently, the comment should be - "set/register the driver per CPU
> to @drv" instead of "unset the registered driver per CPU to @drv" in
> case of __cpuidle_set_driver.

I would like to slightly differ here. As per my understanding, the
comments given
for the function is correct. The function __cpuidle_set_driver(drv)
would do the following:-

1. If the cpu has any registered idle driver which is same as @drv,
the registered
driver would be unset. ie "unset the registered driver per CPU to @drv"
2. If the cpu has any registered idle driver which is different from
@drv, do nothing
3. If the cpu has no registered idle driver, set the idle driver to @drv

> However, regarding the logic, I have a few doubts -
>
> 1. for each cpu in drv->cpumask, if there is already a driver
> registered, its calling __cpuidle_unset_driver which loops over for
> each cpu in drv->cpumask again. Isn't it unnecessary to do this nested
> calls?

__cpuidle_unset_driver :- This function gets called from
"__cpuidle_unregister_driver()" too.
So it needs to loop over each cpu to see if its registered driver is
same as @drv. What
you might be trying to convey here is that instead of calling
__cpuidle_unset_driver, we could
have done the following:-
static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
{
        int cpu;
        struct cpuidle_driver *tmp = NULL;
        for_each_cpu(cpu, drv->cpumask) {

                tmp = __cpuidle_get_cpu_driver(cpu); /* This would
prevent nesting of loops */
                if ( tmp != NULL ) {
                        if ( tmp == drv )
                              per_cpu(cpuidle_drivers, cpu) = NULL;
                        return -EBUSY;
                }

                per_cpu(cpuidle_drivers, cpu) = drv;
        }

        return 0;
}



> 2. after calling __cpuidle_unset_driver, if drv equals already
> registered driver, it sets per_cpu driver to null? Isn't it wrong when
> we are trying to set to a new driver? Why do we need to unset and make
> the driver null when we are returning EBUSY from __cpuidle_set_driver?

My understanding is that if there is a previously registered cpuidle
driver, returning
EBUSY is fine. But I do share the same doubt as you have that if the previous
registered cpuidle driver is same as the new one, then why should it
be unset and NULLed.

> Would it be correct and cleaner if the code is written as below -
>
>  static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>  {
>          int ret = -EBUSY;
>          int cpu;
>
>         for_each_cpu(cpu, drv->cpumask) {
>                 if (drv == __cpuidle_get_cpu_driver(cpu))    [if drv
> is already the registered driver, do nothing]
>                          continue;
>
>                  per_cpu(cpuidle_drivers, cpu) = drv;  [if only drv !=
> already registered driver, set per_cpu driver to drv and set ret 0]
>                  ret = 0;
>          }
>
>          return ret;     [only if all cpus already had drv as
> registered driver, return -EBUSY. Otherwise return 0]
>  }
>
The difference that might cause some trouble is that the timer
broadcast notification is
not sent while changing the cpuide drivers.

Regards,
Ayan Kumar Halder

Re: Need help understanding the logic of __cpuidle_set_driver

[Mohammad Merajul Islam Molla]

Thanks Ayan.

My answers are inline below -


On Fri, Aug 15, 2014 at 6:41 PM, AYAN KUMAR HALDER <ayankumarh@gmail.com> wrote:
> On Fri, Aug 15, 2014 at 3:50 PM, Mohammad Merajul Islam Molla
> <meraj.enigma@gmail.com> wrote:
>> Hello,
>>
>> I was looking into the code of drivers/cpuidle/driver.c. I have some
>> doubts regarding the implementation of __cpuidle_set_driver function
>> when CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined.
>>
>> If CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined, the code for
>> __cpuidle_set_driver/__cpuidle_unset_driver looks as -
>>
>>  39  * __cpuidle_unset_driver - unset per CPU driver variables.
>>  40  * @drv: a valid pointer to a struct cpuidle_driver
>>  41  *
>>  42  * For each CPU in the driver's CPU mask, unset the registered
>> driver per CPU
>>  43  * variable. If @drv is different from the registered driver, the
>> corresponding
>>  44  * variable is not cleared.
>>  45  */
>>  46 static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>>  47 {
>>  48         int cpu;
>>  49
>>  50         for_each_cpu(cpu, drv->cpumask) {
>>  51
>>  52                 if (drv != __cpuidle_get_cpu_driver(cpu))
>>  53                         continue;
>>  54
>>  55                 per_cpu(cpuidle_drivers, cpu) = NULL;
>>  56         }
>>  57 }
>>  58
>>  59 /**
>>  60  * __cpuidle_set_driver - set per CPU driver variables for the given driver.
>>  61  * @drv: a valid pointer to a struct cpuidle_driver
>>  62  *
>>  63  * For each CPU in the driver's cpumask, unset the registered driver per CPU
>>  64  * to @drv.
>>  65  *
>>  66  * Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
>>  67  */
>>  68 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>>  69 {
>>  70         int cpu;
>>  71
>>  72         for_each_cpu(cpu, drv->cpumask) {
>>  73
>>  74                 if (__cpuidle_get_cpu_driver(cpu)) {
>>  75                         __cpuidle_unset_driver(drv);
>>  76                         return -EBUSY;
>>  77                 }
>>  78
>>  79                 per_cpu(cpuidle_drivers, cpu) = drv;
>>  80         }
>>  81
>>  82         return 0;
>>  83 }
>>
>> Apparently, the comment should be - "set/register the driver per CPU
>> to @drv" instead of "unset the registered driver per CPU to @drv" in
>> case of __cpuidle_set_driver.
>
> I would like to slightly differ here. As per my understanding, the
> comments given
> for the function is correct. The function __cpuidle_set_driver(drv)
> would do the following:-
>
> 1. If the cpu has any registered idle driver which is same as @drv,
> the registered
> driver would be unset. ie "unset the registered driver per CPU to @drv"
> 2. If the cpu has any registered idle driver which is different from
> @drv, do nothing
> 3. If the cpu has no registered idle driver, set the idle driver to @drv
>
>> However, regarding the logic, I have a few doubts -
>>
>> 1. for each cpu in drv->cpumask, if there is already a driver
>> registered, its calling __cpuidle_unset_driver which loops over for
>> each cpu in drv->cpumask again. Isn't it unnecessary to do this nested
>> calls?
>
> __cpuidle_unset_driver :- This function gets called from
> "__cpuidle_unregister_driver()" too.
> So it needs to loop over each cpu to see if its registered driver is
> same as @drv. What

Yes, to unset the driver __cpuidle_unregister_driver calls
__cpuidle_unset_driver(),
which has to loop over the cpus. But __cpuidle_set_driver() can be
implemented itself and therefore,
I don't see any point doing the loop twice.

> you might be trying to convey here is that instead of calling
> __cpuidle_unset_driver, we could
> have done the following:-
> static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
> {
>         int cpu;
>         struct cpuidle_driver *tmp = NULL;
>         for_each_cpu(cpu, drv->cpumask) {
>
>                 tmp = __cpuidle_get_cpu_driver(cpu); /* This would
> prevent nesting of loops */
>                 if ( tmp != NULL ) {
>                         if ( tmp == drv )
>                               per_cpu(cpuidle_drivers, cpu) = NULL;
>                         return -EBUSY;
>                 }
>
>                 per_cpu(cpuidle_drivers, cpu) = drv;
>         }
>
>         return 0;
> }
>
>
>
>> 2. after calling __cpuidle_unset_driver, if drv equals already
>> registered driver, it sets per_cpu driver to null? Isn't it wrong when
>> we are trying to set to a new driver? Why do we need to unset and make
>> the driver null when we are returning EBUSY from __cpuidle_set_driver?
>
> My understanding is that if there is a previously registered cpuidle
> driver, returning
> EBUSY is fine. But I do share the same doubt as you have that if the previous
> registered cpuidle driver is same as the new one, then why should it
> be unset and NULLed.


Right, I don't understand this either.


>
>> Would it be correct and cleaner if the code is written as below -
>>
>>  static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>>  {
>>          int ret = -EBUSY;
>>          int cpu;
>>
>>         for_each_cpu(cpu, drv->cpumask) {
>>                 if (drv == __cpuidle_get_cpu_driver(cpu))    [if drv
>> is already the registered driver, do nothing]
>>                          continue;
>>
>>                  per_cpu(cpuidle_drivers, cpu) = drv;  [if only drv !=
>> already registered driver, set per_cpu driver to drv and set ret 0]
>>                  ret = 0;
>>          }
>>
>>          return ret;     [only if all cpus already had drv as
>> registered driver, return -EBUSY. Otherwise return 0]
>>  }
>>
> The difference that might cause some trouble is that the timer
> broadcast notification is
> not sent while changing the cpuide drivers.
>
> Regards,
> Ayan Kumar Halder


Anyone else looking at this? Please clarify.



Thanks,
-Meraj

Re: Need help understanding the logic of __cpuidle_set_driver

[Daniel Lezcano]

On 08/15/2014 12:20 PM, Mohammad Merajul Islam Molla wrote:
> Hello,
>
> I was looking into the code of drivers/cpuidle/driver.c. I have some
> doubts regarding the implementation of __cpuidle_set_driver function
> when CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined.
>
> If CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined, the code for
> __cpuidle_set_driver/__cpuidle_unset_driver looks as -
>
>   39  * __cpuidle_unset_driver - unset per CPU driver variables.
>   40  * @drv: a valid pointer to a struct cpuidle_driver
>   41  *
>   42  * For each CPU in the driver's CPU mask, unset the registered
> driver per CPU
>   43  * variable. If @drv is different from the registered driver, the
> corresponding
>   44  * variable is not cleared.
>   45  */
>   46 static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv)
>   47 {
>   48         int cpu;
>   49
>   50         for_each_cpu(cpu, drv->cpumask) {
>   51
>   52                 if (drv != __cpuidle_get_cpu_driver(cpu))
>   53                         continue;
>   54
>   55                 per_cpu(cpuidle_drivers, cpu) = NULL;
>   56         }
>   57 }
>   58
>   59 /**
>   60  * __cpuidle_set_driver - set per CPU driver variables for the given driver.
>   61  * @drv: a valid pointer to a struct cpuidle_driver
>   62  *
>   63  * For each CPU in the driver's cpumask, unset the registered driver per CPU
>   64  * to @drv.
>   65  *
>   66  * Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
>   67  */
>   68 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>   69 {
>   70         int cpu;
>   71
>   72         for_each_cpu(cpu, drv->cpumask) {
>   73
>   74                 if (__cpuidle_get_cpu_driver(cpu)) {
>   75                         __cpuidle_unset_driver(drv);
>   76                         return -EBUSY;
>   77                 }
>   78
>   79                 per_cpu(cpuidle_drivers, cpu) = drv;
>   80         }
>   81
>   82         return 0;
>   83 }
>
> Apparently, the comment should be - "set/register the driver per CPU
> to @drv" instead of "unset the registered driver per CPU to @drv" in
> case of __cpuidle_set_driver.

Right, it is a typo.

> However, regarding the logic, I have a few doubts -
>
> 1. for each cpu in drv->cpumask, if there is already a driver
> registered, its calling __cpuidle_unset_driver which loops over for
> each cpu in drv->cpumask again. Isn't it unnecessary to do this nested
> calls?

It is to rollback the previous changes done in the loop.

> 2. after calling __cpuidle_unset_driver, if drv equals already
> registered driver, it sets per_cpu driver to null? Isn't it wrong when
> we are trying to set to a new driver? Why do we need to unset and make
> the driver null when we are returning EBUSY from __cpuidle_set_driver?
>
> Would it be correct and cleaner if the code is written as below -
>
>   static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
>   {
>           int ret = -EBUSY;
>           int cpu;
>
>          for_each_cpu(cpu, drv->cpumask) {
>                  if (drv == __cpuidle_get_cpu_driver(cpu))    [if drv
> is already the registered driver, do nothing]
>                           continue;
>
>                   per_cpu(cpuidle_drivers, cpu) = drv;  [if only drv !=
> already registered driver, set per_cpu driver to drv and set ret 0]
>                   ret = 0;
>           }
>
>           return ret;     [only if all cpus already had drv as
> registered driver, return -EBUSY. Otherwise return 0]
>   }
>
>
> Please let me know your views.
>
> --
> Thanks,
> -Meraj
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: Need help understanding the logic of __cpuidle_set_driver

[AYAN KUMAR HALDER]

>> However, regarding the logic, I have a few doubts -
>>
>> 1. for each cpu in drv->cpumask, if there is already a driver
>> registered, its calling __cpuidle_unset_driver which loops over for
>> each cpu in drv->cpumask again. Isn't it unnecessary to do this nested
>> calls?
>
>
> It is to rollback the previous changes done in the loop.

However, I have two concerns here:-
1.
static inline int __cpuidle_set_driver(struct cpuidle_driver *drv)
{
        int cpu;
        struct cpuidle_driver *tmp = NULL;
        for_each_cpu(cpu, drv->cpumask) {

                tmp = __cpuidle_get_cpu_driver(cpu); /* This would
prevent nesting of loops */
                if ( tmp != NULL ) {
                        if ( tmp == drv )
                              per_cpu(cpuidle_drivers, cpu) = NULL;
                        return -EBUSY;
                }

                per_cpu(cpuidle_drivers, cpu) = drv;
        }

        return 0;
}
Instead of calling __cpuidle_unset_driver() which would loop-over
again for all the cpus , is it not that the above function would be
more efficient.

2. Secondly, if the cpuidle driver to be registered matches to the one
already registered , then why should it be unset instead of leaving it
as it is.


Any views would be highly appreciated.