Hi Eric,
Please see following comments.
Thanks!
Eric.Saxe at Sun.COM <mailto:Eric.Saxe at Sun.COM> wrote:
> 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.
Here I mean per-callback context, you may pass in one argument when register
callback handler and the argument will be passed back when callback get called.
I think it's simpler and clearer to pass "arg" as a separate argument when
calling registered callback though "arg" may be integrated into cpu_idle_info_t
structure,
>
>>> 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).
Ashok has suggested to return a "handle" when registering callbacks which could
be used to unregister callbacks and I'll integrate it into next version
onepager.
> 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.
This is a novel idea which eliminates the priority allocation issue. But it may
introduces some new issues.
1) Each handle returned by cpu_idle_register_callback need to be exported in
order that other handles could make dependency on it.
2) We need to ensure some sort of order when registering callbacks to make use
of cpu_idle_register_dependency.
3) It may introduce dependencies among drivers. For example, driver A registers
callback A, driver B registers callback B, driver B will have dependency on
driver A if callback B is dependent on callback A.
4) It may cause trouble when dynamically registering/unregister callbacks.
Thinking about following scenarion:
- Callback B has dependency on callback A
- Register callback A
- Register callback B and make dependency on callback A
- Try to unregister callback A. We have two options here: fail the
unregister operation because there're other callbacks dependent on it or
complete the unregister operation successfully.
- Register callback A again if unregistering succeed. Then the
dependency between A and B becomes broken,
5) I have a feeling, to adopt the above solution, all callbacks need to know
each other in order to maintain dependencies correctly among callbacks.
The idea is very interesting, but we still need to solve some troublesome
issues,
How about making some tradeoff here, first implement a simple version with
current priority based design and enhance it when there're strong requirements.
>
> Thanks,
> -Eric
