Re: [PATCH] opp: convert dev_warn() to dev_dbg() for duplicate OPPs

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, Nov 24, 2014 at 04:09:54PM +0100, Rafael J. Wysocki wrote:
> On Monday, November 24, 2014 04:10:00 PM Viresh Kumar wrote:
> > 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);

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.

> >         }
> > 
> > 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 ?
> 
> I hope that doesn't happen under rcu_read_lock()?
> 
> The modification needs to be done under dev_opp_list_lock in the first place
> 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
> deferred until all readers drop their references to the old entry.  You can
> use kfree_rcu() for that.

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);

This will require adding the following to struct device_opp:

	struct rcu_head rcu;

And yes, this would be simpler if there was a kfree_srcu().  If a few
more uses like this show up, I will create one.

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.

							Thanx, Paul