| Message ID | 1348580573-21877-1-git-send-email-vincent.guittot@linaro.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi Rafael, Ping. Regards, Vincent On 25 September 2012 15:42, Vincent Guittot <vincent.guittot@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.guittot@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Thanks, Rafael > On 25 September 2012 15:42, Vincent Guittot <vincent.guittot@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.guittot@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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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 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. Thanks, Rafael > On 25 September 2012 15:42, Vincent Guittot <vincent.guittot@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.guittot@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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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 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 <paulmck@linux.vnet.ibm.com> > Thanks, > Rafael > > > > On 25 September 2012 15:42, Vincent Guittot <vincent.guittot@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.guittot@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 majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- 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 Wednesday 03 of October 2012 21:38:09 Vincent Guittot wrote: > On Wednesday, 3 October 2012, Paul E. McKenney <paulmck@linux.vnet.ibm.com> > wrote: > > 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 <paulmck@linux.vnet.ibm.com> > > Hi Paul, > > Thanks for the review. Thanks, I will queue it up for v3.8 when I get back from the current trip. Rafael
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; }
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.guittot@linaro.org> --- drivers/base/power/opp.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)