On Sat, 2010-06-05 at 23:21 +0200, Florian Mickler wrote:
> On Sat, 05 Jun 2010 15:57:10 -0500
> James Bottomley <James.Bottomley@xxxxxxx> wrote:
>
> > On Sat, 2010-06-05 at 22:46 +0200, florian@xxxxxxxxxxx wrote:
> > > First we use atomic_notifier to get the spinlocked variant.
> > > Secondly we call the notifiers via schedule_work.
> > >
> > > Signed-off-by: Florian Mickler <florian@xxxxxxxxxxx>
> > > ---
> > >
> > > Please advise.
> > >
> > > kernel/pm_qos_params.c | 33 ++++++++++++++++++++++-----------
> > > 1 files changed, 22 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > index f42d3f7..7335c27 100644
> > > --- a/kernel/pm_qos_params.c
> > > +++ b/kernel/pm_qos_params.c
> > > @@ -40,6 +40,7 @@
> > > #include <linux/string.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/init.h>
> > > +#include <linux/workqueue.h>
> > >
> > > #include <linux/uaccess.h>
> > >
> > > @@ -63,7 +64,7 @@ static s32 min_compare(s32 v1, s32 v2);
> > >
> > > struct pm_qos_object {
> > > struct pm_qos_request_list requests;
> > > - struct blocking_notifier_head *notifiers;
> > > + struct atomic_notifier_head *notifiers;
> > > struct miscdevice pm_qos_power_miscdev;
> > > char *name;
> > > s32 default_value;
> > > @@ -72,7 +73,7 @@ struct pm_qos_object {
> > > };
> > >
> > > static struct pm_qos_object null_pm_qos;
> > > -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > > +static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > > static struct pm_qos_object cpu_dma_pm_qos = {
> > > .requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
> > > .notifiers = &cpu_dma_lat_notifier,
> > > @@ -82,7 +83,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> > > .comparitor = min_compare
> > > };
> > >
> > > -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> > > +static ATOMIC_NOTIFIER_HEAD(network_lat_notifier);
> > > static struct pm_qos_object network_lat_pm_qos = {
> > > .requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
> > > .notifiers = &network_lat_notifier,
> > > @@ -93,7 +94,7 @@ static struct pm_qos_object network_lat_pm_qos = {
> > > };
> > >
> > >
> > > -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> > > +static ATOMIC_NOTIFIER_HEAD(network_throughput_notifier);
> > > static struct pm_qos_object network_throughput_pm_qos = {
> > > .requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
> > > .notifiers = &network_throughput_notifier,
> > > @@ -103,7 +104,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
> > > .comparitor = max_compare
> > > };
> > >
> > > -
> > > static struct pm_qos_object *pm_qos_array[] = {
> > > &null_pm_qos,
> > > &cpu_dma_pm_qos,
> > > @@ -135,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
> > > return min(v1, v2);
> > > }
> > >
> > > +static void update_notify(struct work_struct* work){
> > > + int pm_qos_class = atomic_long_read(&work->data);
> > > +
> > > + s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> > > + atomic_notifier_call_chain(
> > > + pm_qos_array[pm_qos_class]->notifiers,
> > > + (unsigned long) extreme_value, NULL);
> > > +}
> > > +struct work_struct pm_qos_update_notify_work;
> > >
> > > static void update_target(int pm_qos_class)
> > > {
> > > @@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
> > > }
> > > spin_unlock_irqrestore(&pm_qos_lock, flags);
> > >
> > > - if (call_notifier)
> > > - blocking_notifier_call_chain(
> > > - pm_qos_array[pm_qos_class]->notifiers,
> > > - (unsigned long) extreme_value, NULL);
> > > + if (call_notifier){
> > > + atomic_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
> > > + schedule_work(&pm_qos_update_notify_work);
> >
> > Actually, you should use execute_in_process_context for this ... that
> > way any current call from user context won't change semantics (i.e. the
> > notifier will be executed before the call finishes).
>
> Ah, ok. Didn't know of its existence.
> After thinking a bit, I think it is crucial for the qos-infrastructure
> to provide direct notifiers. For example, any missed change in latency
> could impact the system.
>
> >
> > I also suspect we might need an atomic notifier chain to be called
> > in-line ... things like the dma latency notifier need in-line
> > notification, I can see that some android ones would at all. However,
> > this question can be deferred until an actual use case is seen.
>
> We should probably only provide an atomic queue and have all users
> either schedule work or be quick, as it can be crucial to get the
> information out there.
>
> What do you think?
Yes, considering the use cases, I don't think we want to muck with
workqueues at all, so just do two queues per constraint, one blocking
and one atomic. For a constraint that has atomic update sites,
registering a blocking notifier would be illegal and we'd give a runtime
warning (we can even code a __might_sleep in there before the call).
That way we preserve current semantics and permit blocking notifiers to
be registered, but only if all the call sites have user context.
James
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
