Bill Holler wrote: > Hi Gerry, > > This onepager looks good to me. > Please update with an explanation of the void *arg > as we discussed on IRC. :-) > Sounds good to me...
Thanks, -Eric > Thank you, > Bill > > > On 12/02/08 18:52, Liu, Jiang wrote: > >> 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 >>> >>> >>> > > _______________________________________________ > tesla-dev mailing list > tesla-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/tesla-dev >
