On Tue, Oct 02, 2012 at 04:02:05AM +0200, Rafael J. Wysocki wrote:
> On Monday 01 of October 2012 15:42:39 Vincent Guittot wrote:
> > Hi Rafael,
> > 
> > Ping.
> 
> I'm considering this for v3.8, but I'd like Paul (CCed) to tell me what he
> thinks about it.

Looks good to me -- the only thing deferred is the freeing of memory.
The only risk is that fast repeated invocation of opp_set_availability()
could result in a lot of memory waiting for RCU grace periods, but
call_rcu() has code to prevent this.  So:

Reviewed-by: Paul E. McKenney <paul...@linux.vnet.ibm.com>

> Thanks,
> Rafael
> 
> 
> > On 25 September 2012 15:42, Vincent Guittot <vincent.guit...@linaro.org> 
> > wrote:
> > > synchronize_rcu blocks the caller of opp_enable/disbale
> > > for a complete grace period. This blocking duration prevents
> > > any intensive use of the functions. Replace synchronize_rcu
> > > by call_rcu which will call our function for freeing the old
> > > opp element.
> > >
> > > The duration of opp_enable and opp_disable will be no more
> > >  dependant of the grace period.
> > >
> > > Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
> > > ---
> > >  drivers/base/power/opp.c |   19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > > index ac993ea..49e4626 100644
> > > --- a/drivers/base/power/opp.c
> > > +++ b/drivers/base/power/opp.c
> > > @@ -64,6 +64,7 @@ struct opp {
> > >         unsigned long u_volt;
> > >
> > >         struct device_opp *dev_opp;
> > > +       struct rcu_head head;
> > >  };
> > >
> > >  /**
> > > @@ -441,6 +442,17 @@ int opp_add(struct device *dev, unsigned long freq, 
> > > unsigned long u_volt)
> > >  }
> > >
> > >  /**
> > > + * opp_free_rcu() - helper to clear the struct opp when grace period has
> > > + * elapsed without blocking the the caller of opp_set_availability
> > > + */
> > > +static void opp_free_rcu(struct rcu_head *head)
> > > +{
> > > +       struct opp *opp = container_of(head, struct opp, head);
> > > +
> > > +       kfree(opp);
> > > +}
> > > +
> > > +/**
> > >   * opp_set_availability() - helper to set the availability of an opp
> > >   * @dev:               device for which we do this operation
> > >   * @freq:              OPP frequency to modify availability
> > > @@ -511,7 +523,7 @@ static int opp_set_availability(struct device *dev, 
> > > unsigned long freq,
> > >
> > >         list_replace_rcu(&opp->node, &new_opp->node);
> > >         mutex_unlock(&dev_opp_list_lock);
> > > -       synchronize_rcu();
> > > +       call_rcu(&opp->head, opp_free_rcu);
> > >
> > >         /* Notify the change of the OPP availability */
> > >         if (availability_req)
> > > @@ -521,13 +533,10 @@ static int opp_set_availability(struct device *dev, 
> > > unsigned long freq,
> > >                 srcu_notifier_call_chain(&dev_opp->head, 
> > > OPP_EVENT_DISABLE,
> > >                                          new_opp);
> > >
> > > -       /* clean up old opp */
> > > -       new_opp = opp;
> > > -       goto out;
> > > +       return 0;
> > >
> > >  unlock:
> > >         mutex_unlock(&dev_opp_list_lock);
> > > -out:
> > >         kfree(new_opp);
> > >         return r;
> > >  }
> > > --
> > > 1.7.9.5
> > >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 


_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Reply via email to