Chad Mynhier wrote: > On Thu, Sep 18, 2008 at 9:00 PM, Aubrey Li <aubreylee at gmail.com> wrote: >> On Fri, Sep 19, 2008 at 12:20 AM, Chad Mynhier <cmynhier at gmail.com> wrote: >>> On Wed, Sep 17, 2008 at 9:43 PM, Aubrey Li <aubreylee at gmail.com> wrote: >>>> On Thu, Sep 18, 2008 at 2:32 AM, Chad Mynhier <cmynhier at gmail.com> >>>> wrote: >>>>> http://cr.opensolaris.org/~cmynhier/powertop/ >>>>> >>>> Thanks for the patch, :-) >>>> >>>> sorry if I didn't read the latest code, all the collection could be >>>> successful and "reinit" >>>> will be used without initialization. right? >>> Yes, if the data collection is successful, the dtrace stuff won't be >>> reinitialized. During normal use, the dtrace stuff will be >>> initialized once and never again. It's only if something happens to >>> terminate the dtrace stuff that it will get restarted. >> hmm..., this is what I mean: >> ======================================================= >> diff -r c1e5c42b522a usr/src/cmd/powertop/powertop.c >> --- a/usr/src/cmd/powertop/powertop.c Fri Sep 19 08:47:17 2008 +0800 >> +++ b/usr/src/cmd/powertop/powertop.c Fri Sep 19 08:47:55 2008 +0800 >> @@ -171,7 +171,7 @@ >> while (true) { >> fd_set rfds; >> struct timeval tv; >> - int key, reinit; >> + int key, reinit = 0; >> char keychar; >> >> /* >> ======================================================= > > Okay, I see what you mean. > > This isn't harmful, but it is a bit sloppy. I've updated the webrev > (http://cr.opensolaris.org/~cmynhier/powertop/.) And for Raf: yep, > it's lint- and hg-pbchk-clean.
I've added you as a leader to tesla-dev so you can push your changes. Please update the bug report and include it in the comments for the changeset like it's done for ON. >>>> Others look good. >>>> >>>> I didn't dig into the dtrace problem, just wonder is this expected? >>>> Or Is the patch just a workaround temporarily and dtrace problem >>>> will be fixed eventually? >>> This is actually tickling a safety feature of dtrace, the deadman >>> timer. There's more information here: >>> http://blogs.sun.com/jonh/entry/the_dtrace_deadman_mechanism, but it's >>> basically a mechanism to prevent dtrace from rendering the system >>> unresponsive. It's possible that the mechanism could be modified to >>> handle cases like this, but I don't know that it would be a high >>> priority to fix it. >>> >>> I wouldn't say that the patch is just a workaround, though. The basic >>> problem is that it's ignoring the return value of dtrace_status(), and >>> it really shouldn't be doing that, anyway. >>> >> So, all the applications which use libdtrace need this fix for >> suspend/resume, >> this includes intrstat/lockstat/plockstat and dtrace itself. No object from >> me >> to commit this patch, but I still think this issue should be fixed in dtrace, >> otherwise all the dtrace applications have to use this trick. > > I'm actually going to take this conversation over to dtrace-discuss, > as it's a bit off-topic for tesla-dev. Cool. Please ping us if they decide to change this bits. And thanks again for looking into this. Rafael
