Hi Bill and Eric,
I have made minor changes to the FIPE PSARC onepager according to your
suggestion, basically return an handle when registers callback and the handle
will be used to unregister the callback.
Any more comments here? If no more comments, I'll cooperate with Randy
to go on with the PSARC process.
Thanks!
>-----Original Message-----
>From: tesla-dev-bounces at opensolaris.org
>[mailto:tesla-dev-bounces at opensolaris.org] On Behalf Of Liu, Jiang
>Sent: 2008?11?25? 23:23
>To: Eric.Saxe at Sun.COM
>Cc: Bill.Holler at Sun.COM; Randall Fishel; tesla-dev at opensolaris.org
>Subject: Re: [tesla-dev] Request for comment for PSARC
>onapager about CPU idle nofication interface
>
>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
>_______________________________________________
>tesla-dev mailing list
>tesla-dev at opensolaris.org
>http://mail.opensolaris.org/mailman/listinfo/tesla-dev
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: onepager_v3.txt
URL:
<http://mail.opensolaris.org/pipermail/tesla-dev/attachments/20081203/71636247/attachment.txt>
