On Mon, 07 Jun 2010 09:10:40 -0400
James Bottomley <James.Bottomley@xxxxxxx> wrote:
> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index f42d3f7..0a67997 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -63,7 +63,8 @@ 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 blocking_notifier_head *blocking_notifiers;
> > struct miscdevice pm_qos_power_miscdev;
> > char *name;
> > s32 default_value;
> > @@ -72,20 +73,24 @@ 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 BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_blocking_notifier);
>
> So I think it might be better implemented by having only a single active
> notifier head: either blocking or atomic because all this depends on
> where the callsites for the notifiers are, and the person adding the
> notifier should know this..
> We can add atomic notifiers to the blocking
> chain, just not vice versa. The idea is that if all the add and update
> call sites are blocking, you just register the blocking chain and forget
> the atomic one.
> The only difference between atomic and blocking
> notifiers is whether we use a spinlock or a mutex to guard the integrity
> of the call chain ... if you know you always have user context at the
> callsites, then you can always use the mutex.
> Then, for blocking notifiers, I think in init, we can register a single
> notifier which just calls __might_sleep() ... that will pick up at
> runtime any atomic callsite.
I like that part. Simple and elegant :)
>
> For atomics, you just set up an atomic call chain and leave the blocking
> one null. Then we get a BUG if anyone tries to register a blocking
> notifier to an atomic only pm_qos_object.
>
(Well, we can also just ignore and print a WARN() ... but I got your
point)
But I don't think I understand how you want to set up the call chains.
(I.e. How to decide if all call-sites are from process-context (mutex
allowed)? )
As far as I see, the locking for the notifier-chains is in the head. So
I have to decide before the first AddNotifier what locking I
want (blocking_ or atomic_notifier_head).
Are you thinking about having it hardcoded alongside the pm_qos_object
instantiation? (I think that would be ok)
Or are you thinking about some other scheme I don't see?
> The implementation looks fine, except:
>
> [...]
> > /**
> > + * pm_qos_add_notifier_nonblocking - sets notification entry for changes to target value
> > + *
> > + * Code executed by the notifier block may not sleep!
> > + *
> > + * @pm_qos_class: identifies which qos target changes should be notified.
> > + * @notifier: notifier block managed by caller.
> > + *
> > + * Will register the notifier into a notification chain that gets called
> > + * upon changes to the pm_qos_class target value.
> > + */
> > +int pm_qos_add_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier)
>
> Rightly or wrongly, the notifier people use atomic not nonblocking, so
> we should really stick with it to avoid confusion.
Yes. I think that's better.
> James
>
... mulling it over, I think everything you say add's up if I move the
decision to the pm_qos_object instatiation. So I will send out a new
patch with that implemented.
Cheers,
Flo
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
