Rafael.Vanoni wrote: > Li, Aubrey wrote: >> Rafael.Vanoni wrote: >> >>> Li, Aubrey wrote: >>>> Rafael.Vanoni wrote: >>>> >>>>> I've updated the webrev @ >>>>> http://cr.opensolaris.org/~rafaelv/ptop-freq/ >>>>> >>>>> I'll file a separate CR to change the DTrace script in cpufreq.c, >>>>> we have other issues I think are more important at this time. >>>>> >>>>> This latest webrev is against PowerTOP's gate, not ON. I've added >>>>> a check to make sure we're deducting the accounted time for the >>>>> correct p-state. Let me know what you think. >>>>> >>>> I don't think the patch is doing the right thing to deal with >>>> accounted time. >>>> >>>> Example: >>>> >>>> Round1: 5 seconds >>>> 1) 2s in P1 >>>> 2) 3s in P0 >>>> >>>> Round2: 5 seconds >>>> 1) 3s in P0 >>>> 2) 2s in P1 >>>> >>>> Here, in the second round, accounted time is apparantly for P0, but >>>> cpu_pow->current_pstate is P1. So the suggested patch is > still wrong. >>> What accounted time? Your scenario is self contained. In both cases, >>> time_accounted would be zero since the aggregation results account >>> for the entire interval. >>> >>> Rafael > > Ah.. sorry, the cpufreq aggregation is ordered by key so I misread > your example. > >> In Round1, time_accounted would be ZERO and dtrace script only >> capture one time probe firing. the aggregation should be >> "P1 2s" >> >> And in Round2, the dtrace probe will be fired only once as well and >> the aggregation should be "P0 6s" >> >> After dtrace walk, pt_cpufreq_snapshot() get the current p-state is >> P1. And then, pt_cpufreq_stat_account() get the time_accounted is >> 3s(Round1 P0 residency, which is not captured by dtrace script). > > time_accounted won't be 3 by the time account() is called, it will > have been deducted from 6 and then zero'ed. > >> And, the suggested patch set the >> cpu_pow->speed_accounted = speed(P1). So, here > time_accounted will be accounted on P1. >> >> Is this expected? > > This is how I expect this to work: > > P0 | ---|---\ | > P1 |--/ | --| > > P0 0 6 > P1 2 0 > walk() account()| walk() account() > P0 P1 P0 | P0 P1 P1 > res 0 2 - | 3 0 - > total_time 0 2 3 | 3 0 2 > dtrace_time 0 2 - | 3 3 - > time_accounted 0 0 3 | 0 0 2 > speed_accounted 0 0 P0 | 0 0 P1 > duration - - 3 | - - 2 > > I don't see a problem here.. am I missing something ? > > I've posted a test program that simulates all of this at the Self > Testing page. I've used it to test this patch. > > Rafael
Okay, things became much more clear now. I thought the time_accounted and speed_accounted does not match, which is wrong. Please commit this patch. Thanks, -Aubrey
