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