Hi Gerry,
Liu, Jiang wrote:
> Bill.Holler at Sun.COM <mailto:Bill.Holler at Sun.COM> wrote:
>
>> Hi Gerry,
>>
>> Here are my comments on this proposal.
>>
>> ci_c_state should be architecture neutral. Something like
>> ci_idle_state would be better. Each architecture can have public
>> defines for what these mean (e.g. #define C1 1).
>>
> Good suggestion, I'll refine it.
>
>
>> Should "arg" be removed from the typedefs for cpu_idle_enter_cbfn
>> and cpu_idle_exit_cbfn and also removed from
>> cpu_idle_register_callback and cpu_idle_unregister_callback?
>> There is no purpose for arg in the new version of the interface.
>>
> The arg is used to pass context info back to callbacks.
> It's useful when the same callback handler will be registered more than one
> time. If no such usage scenario, we can remove it.
>
I guess that context could also be passed via something in the
cpu_idle_info_t.
>> Should cpu_idle_exit handlers be called before enabling interrupts
>> when the CPU resumes from idle?
>>
> Yes, it will be called before enabling interrupt and all cpu_idle_exit
> handlers will called with interrupt enabling flag cleared.
>
>
>> What are your thoughts on priority? Is this related to
>> hardware requirements (for example run handler before
>> disabling LAPIC)? Or is this for power performance?
>> This seems like a tricky area where some of the hardware
>> implementation may need to be exposed.
>>
> Mainly the priority is used to resolve hardware dependencies among callbacks.
> Actually this is the most tricky and dirty part, under some circumstances we
> may need to statically allocate priority based on hardware dependency and
> performance requirements.
> I have splitted the priority space into three sub-spaces:
> HIGH_PRIORITY sub-space: statically allocated high priorities
> DYNAMIC_PRIORITY sub-space: callbacks don't care about calling order and
> system allocate priority for callbacks.
> LOW_PRIORITY sub-space: statically allocated low priorities
>
This seems a bit dicey. Would it be cleaner to just provide an interface
to allow one callback to specify that it "depends" on another one? For
example, perhaps the register callback could return a "handle", that
could then be passed to an interface to say that the newly registered
callback depends on another already registered callback...something
similar to:
cpu_idle_cb_handle_t a, b;
b = cpu_idle_register_callback(callback_func_b, NULL);
a = cpu_idle_register_callback(callback_func_a, NULL);
/*
* Notify the framework that execution of callback "a" depends
* on the execution of callback "b"
*/
if (cpu_idle_register_dependency(a, b) < 0) {
/*
* Unwind registration, log error, etc
*/
...
}
(Actually, such a "handle" would be a convenient thing to pass back to
the interface to do the unregistration).
This way, the dependencies are always specified in relative terms, and
you don't get into problems with callbacks having to know too much about
the priorities of other callbacks.
Thanks,
-Eric
