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. >> >>> >>> 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. Chad
