Rafael.Vanoni <> wrote: > Li, Aubrey wrote: >> Hi Rafael, >> >> Rafael.Vanoni <> wrote: >> >>> I've posted a webrev at >>> http://cr.opensolaris.org/~rafaelv/ptop-3bugs/ with fixes for >>> >>> 11863 i18n and pt_error() >>> 2861 C0 residency sits at 100% and all wakeups are 0.0% when >>> interval is large 11862 powertop(1) reports incorrect c-state data >>> for sun4v systems >>> >>> Each bug report has its own patch, but they are different enough >>> that you can spot individual changes in the webrev. Please have a >>> look and let me know what you think. >>> >> >> Two comments about the webrev >> >> 1) I'm still a bit confused why we need this change. Looking at the >> dtrace probe in the kernel, >> >> DTRACE_PROBE1(idle__state__transition, uint_t, state); >> >> The type of state is uint_t. No matter on 32-bit or 64-bit system, >> the size of the type should be 4. The patch proposed 8 on 64-bit >> system, I have no idea about SPARC, but this is not correct on IA. >> Did I miss something? >> >> - /* LINTED - alignment */ >> - state = *(int32_t *)(data->dtada_data + rec->dtrd_offset); >> >> + switch (g_bit_depth) { >> + case 32: >> + /* LINTED - alignment */ >> + state = *(uint32_t *)(data->dtada_data + >> + rec->dtrd_offset); >> + break; >> + case 64: >> + /* LINTED - alignment */ >> + state = *(uint64_t *)(data->dtada_data + >> + rec->dtrd_offset); >> + break; >> + } >> + > > We're actually de-referencing a casted pointer at different pointer > lengths, so it's not the value itself that has a longer or shorter > data type size, but the pointer in the DTrace structure. I suppose > the data type of 'state' isn't terribly relevant here, as long as > it's some type of int, since it will be implicitly casted by the > attribution. It should be safe to have it declared as uint_t, if you > prefer. >
I don't have preference, do we have these dtrace data structure documented? >> 2) for what reason we need to do the change like this? >> >> -#define INTERVAL_MAX 100.0 >> +#define INTERVAL_MAX 30.0 >> >> - "data in seconds [1-100s]\n"); >> + "data in seconds [1-30s]\n"); > > These changes are for 2861, please have a look at > http://defect.opensolaris.org/bz/show_bug.cgi?id=2861 > Thanks, others in webrev look good to me. -Aubrey
