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. > 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, Rafael
