Li, Aubrey wrote: > 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?
I don't think so. I'll raise this issue on dtrace-discuss, I'm not sure consumers should have to check for this. >>> 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. Cool, thanks. Rafael
