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

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.

Rafael

On Monday, November 24, 2014 08:14:35 AM Paul E. McKenney wrote:

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:

Correct, that's SRCU. Sorry for my confusion.

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.

On Tue, Nov 25, 2014 at 04:07:35PM +0530, Viresh Kumar wrote:

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

Indeed I did! ;-)

...

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.

Fair enough!

...

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.

Done!

Thanx, Paul