| Message ID | CAKohpoknu=V_wscVPWLdZO8xEGY6CVMNXuP4F76GNABnm9K4dQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wednesday, November 19, 2014 01:16:24 PM Viresh Kumar wrote: > On 19 November 2014 02:21, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, November 18, 2014 08:38:14 AM Viresh Kumar wrote: > > >> We are allowing addition of duplicate OPPs as a standard thing right now > >> as cpufreq drivers don't get rid of the OPPs they create with DT. So, that > >> shouldn't complain, isn't it ? > > > > Is cpufreq the only user of OPP? I thought there were other users, so what > > about them? > > Probably of CPU OPPs, but I am not sure. Obviously dev OPPs can be used > by others. > > > I'm not sure about that. If they aren't useful for anything after > > that, what's the benefit of keeping them around? > > I don't think they are of any use once the driver is gone, unless the driver is > inserted again. > > So, this is what we can do to distinguish DT OPPs with other dynamic ones: > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index 490e9db..7e25f01 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -49,6 +49,7 @@ > * are protected by the dev_opp_list_lock for integrity. > * IMPORTANT: the opp nodes should be maintained in increasing > * order. > + * @from_dt: created from static DT entries. What about @dynamic instead of @from_dt? That may apply to more use cases if need be. > * @available: true/false - marks if this OPP as available or not > * @rate: Frequency in hertz > * @u_volt: Nominal voltage in microvolts corresponding to this OPP > @@ -61,6 +62,7 @@ struct dev_pm_opp { > struct list_head node; > > bool available; > + bool from_dt; > unsigned long rate; > unsigned long u_volt; > > > > Does this look fine? I can then write of_free_opp_table(), opposite of > of_init_opp_table(). > -- > 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 21 November 2014 at 21:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > What about @dynamic instead of @from_dt? That may apply to more use cases if > need be. @Paul: I am stuck at a point and need help on RCUs :) File: drivers/base/power/opp.c We are trying to remove OPPs created from static data present in DT on cpufreq driver's removal (when configured as module). opp core uses RCUs internally and it looks like I need to implement: list_for_each_entry_safe_rcu() But, I am not sure because of these: http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-10/6280.html http://patchwork.ozlabs.org/patch/48989/ So, wanted to ask you if I really need that or the OPP code is buggy somewhere. The code removing OPPs is: list_for_each_entry_rcu(opp, &dev_opp->opp_list, node) { srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_REMOVE, opp); list_del_rcu(&opp->node); kfree(opp); } Because we are freeing opp at the end, list_for_each_entry_rcu() is trying to read the already freed opp to find opp->node.next and that results in a crash. What am I doing wrong ? -- 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
On 24 November 2014 at 20:39, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > I hope that doesn't happen under rcu_read_lock()? No. Should it? > The modification needs to be done under dev_opp_list_lock in the first place Yeah, its there to protect against other updaters. > in which case you don't need the _rcu version of list walking, so you simply > can use list_for_each_entry_safe() here. The mutex is sufficient for the > synchronization with other writers (if any). The freeing, though, has to be Correct. > deferred until all readers drop their references to the old entry. You can > use kfree_rcu() for that. Cool. I understood the concept atleast. And yes I followed the srcu mail from paul as well.. Will reply there. -- 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 24 November 2014 at 21:44, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > As Rafael says, if opp is reachable by RCU readers, you cannot just > immediately kfree() it. Immediately kfree()ing it like this -will- > cause your RCU readers to see freed memory, which, as you noted, can > cause crashes. In order to reply you at some level, I tried going through RCU documentation today before replying anymore. And yes I understood this part. > Except that srcu_notifier_call_chain() involves SRCU readers. So, > unless I am confused, you instead need something like this: > > static void kfree_opp_rcu(struct rcu_head *rhp) > { > struct device_opp *opp = container_of(rhp, struct device_opp, opp_list); > > kfree(opp); > } > > Then replace the above kfree() by: > > call_srcu(&opp->rcu, kfree_opp_rcu); Correct. But you missed the srcu which should be the first argument here :) > This will require adding the following to struct device_opp: > > struct rcu_head rcu; We were freeing struct dev_pm_opp, and so I believe you wanted me to add it there? Its already there. > All that said, I do not claim to understand the OPP code, so please take > the above suggested changes with a grain of salt. And if you let me know > where I am confused, I should be able to offer better suggestions. Thanks for your suggestions. I have sent the patch to list and cc'd you on the relevant ones. Would be great if you can review the rcu part there. -- 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
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 490e9db..7e25f01 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -49,6 +49,7 @@ * are protected by the dev_opp_list_lock for integrity. * IMPORTANT: the opp nodes should be maintained in increasing * order. + * @from_dt: created from static DT entries. * @available: true/false - marks if this OPP as available or not * @rate: Frequency in hertz * @u_volt: Nominal voltage in microvolts corresponding to this OPP @@ -61,6 +62,7 @@ struct dev_pm_opp { struct list_head node; bool available; + bool from_dt; unsigned long rate; unsigned long u_volt;