[4/8] opp: Introduce APIs to remove OPPs

OPPs are created statically (from DT) or dynamically. Currently we don't free
OPPs that are created statically, when the module unloads. And so if the module
is inserted back again, we get warning for duplicate OPPs as the same were
already present.

Also, there might be a need to remove dynamic OPPs in future and so API for that
is also added.

This patch adds helper APIs to remove/free existing static and dynamic OPPs.

Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
@Paul/Rafael: Do we need to use call_srcu() instead of kfree_rcu() in
opp_set_availability()? I left it as it looked a bit different.
srcu_notifier_call_chain() is getting called after deleting the node.

 drivers/base/power/opp.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h   |  12 +++++-
 2 files changed, 113 insertions(+), 1 deletion(-)

On 25 November 2014 at 21:54, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> If the data is alway accessed under an SRCU read-side critical section,
> then you do need call_srcu() when freeing it -- as you pointed out,
> -with- the srcu_struct included.  ;-)

:)

> If the data is accessed under both SRCU and RCU, then things get a bit
> more involved.

Yes, that is always the case here.

>> +void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp)
>> +{
>> +     /*
>> +      * Notify the changes in the availability of the operable
>> +      * frequency/voltage list.
>> +      */
>> +     srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
>> +     list_del_rcu(&opp->node);
>> +     call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu);
>
> I am guessing that opp->node is being removed from the list traversed
> by srcu_notifier_call_chain()...  (Looks that way below.)

I couldn't find any code in kernel which is registered for this notifier chain
yet. And so don't know what that code will do. It may not traverse the list
or it may :)

I couldn't understand this comment of yours, sorry: (Looks that way below.)

>> +
>> +     if (list_empty(&dev_opp->opp_list)) {
>
> Does the above want to be "if (!list_empty(&dev_opp->opp_list)) {"?

No. dev_opp contains list of all OPPs. When all OPPs are gone, we don't
want dev_opp anymore and so freeing it.

>> +             list_del_rcu(&dev_opp->node);
>> +             call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,
>> +                       kfree_device_rcu);
>> +     }
>> +}

So, I am still not sure what we need to do here as we have readers with
both rcu and srcu critical regions.
--
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

On 25 November 2014 at 23:09, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Nov 25, 2014 at 10:46:29PM +0530, Viresh Kumar wrote:

>> I couldn't find any code in kernel which is registered for this notifier chain
>> yet. And so don't know what that code will do. It may not traverse the list
>> or it may :)
>
> I would guess that the srcu_notifier_chain_register() in
> devfreq_register_opp_notifier() is adding to the call chain, but I
> do not claim to understand this code.  The srcu_notifier_call_chain()
> above is traversing it.

Actually I searched for who is calling devfreq_register_opp_notifier() and
couldn't notice any callers. But there was one from within devfreq.c and
that was called from exynos code..

So yes, it is used and I was wrong. Ultimately update_devfreq() would
be called from the notifier chain and that may try to change the freq and
so is sleep-able.

>> So, I am still not sure what we need to do here as we have readers with
>> both rcu and srcu critical regions.
>
> Well, the most straightforward approach from an RCU perspective would
> be to make all the readers use the same flavor of RCU.  From Rafael's
> earlier note, I am guessing that some of the code paths absolutely
> require SRCU.  Of course, if all the readers use SRCU, then you can
> simply free using SRCU.

Yeah, so the update_devfreq() call surely requires SRCU.

> You -can- handle situations having more than one type of reader, but
> this requires waiting for all corresponding flavors of grace period.
> This turns out to be fairly simple in this case, for example, have your
> kfree_opp_rcu() function invoke kfree_rcu() instead of kfree().

Hmm, I see.

> But I strongly recommend gaining a full understanding of the code first.
> You can dig yourself a really deep hole really quickly by patching code
> without understanding it!  And apologies if you really do already fully
> understand the code, but your statement above leads me to believe that
> you do not yet fully understand it.

I wouldn't claim to know every part of these frameworks (OPP, Devfreq),
but the maintenance of the nodes/lists is what I understand well.

But yeah, I understand you are worried about. Pushing more bugs for
solving one.

>         I couldn't find any code in kernel which is registered for this
>         notifier chain yet. And so don't know what that code will do. It
>         may not traverse the list or it may :)
>
> One thing that can help internalize code (relatively) quickly is to
> get a large piece of paper and to draw the relationships of the data
> structures first and the relationship of the code later.  When the
> drawing gets too messy, start over with a clean sheet of paper.

Thanks for your suggestions Paul. I was able to do it without a pen
and paper this time as it wasn't that complex. But yes this pen-paper
things really works while working on complex stuff. I found a memory
leak Bug in scheduling domains creation code earlier that way :)

Also it looks like I should use call_srcu() with kfree_rcu() for
opp_set_availability() case as well to avoid any corner cases.

Thanks for your time and help. Really appreciate that :)

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