Hi Gerry, This onepager looks good to me. Please update with an explanation of the void *arg as we discussed on IRC. :-)
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 >> >>
