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;
/*
=======================================================
>
>>
>> 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.
Thanks,
-Aubrey
