Hi Bill,
Thanks for your help, pleaser refer to comments below.
And I'm planning to post another patchset today or tomorrow,
which will address most issues mentioned by you.
Bill.Holler at Sun.COM <mailto:Bill.Holler at Sun.COM> wrote:
> Hello Liu Jiang,
>
> Here are some quick comments:
>
> General: There should be an Intel Copyright added to files
> you are significantly changing (IIRC per the Intel/SUN
> agreement). New Intel Copyrights are missing.
Copyright is always a sensitive topic and thanks for your
reminder. Currently we have a rule for adding copyright to
existing files. If the change is bigger than 100 lines, we need
to add Intel copyright, otherwise it's optional to add intel
copyright. In the patchset, we have changed several existing
files, each about 10 lines, so no Intel copyright added to
those file.
>
> intr.c line 941:
> Does a callback in do_interrupt() add too much latency?
> I assume this is here only to catch idle-wakeup from the
> C1 idle loops which enable interrupt when waking up
> from the HLT or MWAIT instruction?
Yes, we only need to detect the cases you mentioned and
Aubrey raised the same concern during internal review. and I
have worked out a patch to remedy the pain caused by
cpu_idle_exit. With new patch, on x86 native, it should not
cause bigger latency than current implementation. On xpv,
it still adds a minor latency when handling interrupt, but the
latency should be very little.
>
> General: The Deep C-state idle loop only uses C1 when it
> predicts added latency will hurt performance, or the CPU
> will not be idle long enough to enter a deeper C-state.
> Should this knowledge be used in cpu_idle_enter() and
> cpu_idle_exit()?
Yes, we have taken that into account when design cpu idle
notification. But more discussions are need to really integrate
cpu idle notification with deep c driver. In first version, we just
make things work as expected.
>
> General: How do libmicro numbers look?
I'll try to do libmacro benchmark against cpu idle notification.
>
> cpu.h line 22: update Copyright date to 2009. ;-)
At begin, I'm not sure whether we could change Sun copyright.
Aubrey has confirmed that we should/could change Sun
copyright to make it "hg pbchk" clean. I'll fix that in next patch.
>
> cpu_idle.h line 37 - 40 and cpu.h lines 79:
> The CPU_ACPI_C# values are defined by ACPI. There
> could be a comment in cpu.h above line 79 mentioning
> that these IDLE_STATE_# defines must match the ACPI
> definition. Otherwise maintainers may not know where
> these numbers came from.
Thanks for suggestion, will add it into next patch.
>
>
> I have only very quickly looked at the cpu_event files, and
> still need to review these.
>
> cpu_event.c line 453: s/unregisger/unregister/
Already found and fixed this bug, a stupid typo.
>
>
> On 03/31/09 20:35, Liu, Jiang wrote:
>> Hi buddies,
>> We are planning to integrate cpu idle notification into b113 if
>> thing goes smoothly, so quick response is appreciated.
>> Thanks!
>>
>> Liu, Jiang <> wrote:
>>
>>> Hi buddies,
>>> I'm glad to announce the first release of PSARC/2009/115 CPU idle
>>> notification implementation. Great thanks to Randy and Garrett for
>>> supporting
>>> this project.
>>> The gate for CPU idle notification and FIPE project is
>>> ssh://hg.opensolaris.org/tesla/mempm-fipe.
>>> The webrev release is at
>>> http://cr.opensolaris.org/~gerry/cpuidle_20090327/. The PSARC
>>> onepager is at
>>> http://cr.opensolaris.org/~gerry/CPU_idle_notification_onepager.txt.
>>> Could you please help to review the source code and give us any
>>> comments and suggestions about it? There are known limitations in
>>> current implementation
>>> and several directions for future work as below:
>>> 1) Currently implementation only supports x86, supporting of SPARC
>>> will be added later (I'm not familiar with SPARC and needs help from
>>> SPARC experts.)
>>>
>
> It is good start having cpu_event.c in common code.
>
>>> 2) There's unit test code in current code, which will be removed in
>>> final release.
>>> 3) Needs your comments about moving "tlb_going_idle/tlb_service"
>>> and "cpu_dtrace_idle_probe" into CPU idle notifcation as callbacks.
>>> Then
>>> we could control relative calling order with other idle callbacks.
>>> It will also help to remove unnecessary calling of "tlb_service" in
>>> do_interrupt.
>>>
>
> IIRC the HLT instruction requires interrupts be unmasked.
> As far as I know it is not possible to ensure cpu_idle_exit()
> runs before interrupts in cpu_idle()? Some of the earlier
> MWAIT implementations did not allow interrupts to be
> masked either. These will prevent tlb_service() from being
> moved out of do_interrupt().
>
> It seems reasonable to move cpu_dtrace_idle_probe into
> CPU idle notification callback.
>
>
>>> 4) Needs your comments about better cooperation with CPU deep C
>>> driver to avoid duplicated work both in cpupm and idle
>>> notification.
>>>
>
> I will look at this more.
>
> Unfortunately Deep C-states are not enabled on many platforms.
> That prevents CPU Idle Callback from relying too heavily on
> the Deep C-state code and collected data. :-(
>
> At first glance cpupm_wakeup_cstate_data() can be a CPU Idle
> Callback without any additional work.
>
>
>>> 5) Needs your comments on whether need to add "check_wakeup"
>>> hook to cpu_idle_enter, then cpu_idle_enter could detect CPU wakeup
>>> as soon
>>> as possible.
>>>
>
> Can you describe "check_wakeup" a little more?
> I do not understand what this would do?
>
> Regards,
> Bill
>
>
>>> 6) There are several violations to PSARC/2009/115.
>>> So please share your ideas/suggestions with me!
>>> Thanks!
>>>
>>> Liu Jiang (Gerry)
>>> Senior Software Engineer
>>> OpenSolaris, OTC, SSG, Intel
>>> Tel: (8610)82171643
>>> iNet: 8-758-1643
>>> Location: Raycom 9W013
>>>
>>>
>>> _______________________________________________
>>> tesla-dev mailing list
>>> tesla-dev at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/tesla-dev
>>>
>>
>> Liu Jiang (Gerry)
>> OpenSolaris, OTC, SSG, Intel
>> _______________________________________________
>> tesla-dev mailing list
>> tesla-dev at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/tesla-dev
Liu Jiang (Gerry)
OpenSolaris, OTC, SSG, Intel
