On Tue, Jun 17, 2008 at 10:34 PM, Rafael Vanoni <Rafael.Vanoni at sun.com>
wrote:
> Aubrey Li wrote:
>>
>> On Tue, Jun 17, 2008 at 5:36 PM, Rafael Vanoni <Rafael.Vanoni at sun.com>
>> wrote:
>>>
>>> Aubrey Li wrote:
>>>>
>>>> 2008/6/16 Rafael Vanoni <Rafael.Vanoni at sun.com>:
>>>>>
>>>>> Here's a diff that fixes the last three bugs listed on the powertop
>>>>> project
>>>>> page:
>>>>>
>>>>> # PowerTOP does not exit with non-zero exit code if handed an invalid
>>>>> option
>>>>> # PowerTOP works with invalid options
>>>>> # PowerTOP dumps usage message to stdout, not stderr
>>>>
>>>> - if ((bit_depth = get_bit_depth()) < 0)
>>>> - return -1;
>>>> + if ((bit_depth = get_bit_depth()) < 0) {
>>>> + exit(EXIT_FAILURE);
>>>> + return (EXIT_FAILURE);
>>>> + }
>>>>
>>>> Why exit(EXIT_FAILURE) needed here?
>>>
>>> That happens if sysinfo(2) fails to return the bit depth. Do you prefer
>>> to
>>> fall back to a default value for bit depth ?
>>
>> er..., what I mean is, there is a return, why still needs an exit?
>> either one will let powertop quit from running.
>
> Ok, I'm being a purist here by keeping the return() :)
> We need to exit to indicate an abnormal exit value.
>
The patch with the slight modifications is good enough.
Please feel free to commit.
>>>> +
>>>> + if (argc > 1)
>>>> + if (argv[1][0] != '-')
>>>> + usage();
>>>>
>>>> This code looks ugly, and not a fix if run "powertop --"
>>>> Any better idea?
>>>
>>> getopt_long(3C) doesn't check this, so we're left with having to do it
>>> manually. Do you know of a routine that we could use here ?
>>>
>>> I've seen code in nevada that checks if the options are valid before
>>> parsing
>>> them. But that's ugly code as well.
>>>
>>>>> I also would like to remove the following piece of code from powertop.c
>>>>>
>>>>> /*
>>>>> * Quiet down the effects of any IO to xterms
>>>>> */
>>>>> if (!key && ticktime >= 4.8) {
>>>>> FD_ZERO(&rfds);
>>>>> FD_SET(0, &rfds);
>>>>> tv.tv_sec = 3;
>>>>> tv.tv_usec = 0;
>>>>> key = select(1, &rfds, NULL, NULL, &tv);
>>>>> }
>>>>>
>>>>> As the expected functionality is broken and is causing these two bugs:
>>>>>
>>>>> # Time interval is not constant
>>>>> # Multiple PowerTop instances experience time interval fluctuation
>>>>>
>>>>> An alternative would be to calculate the '4.8' during execution, but I
>>>>> don't
>>>>> know what to base it on.
>>>>>
>>>>> What do you think ?
>>>>
>>>> Why time interval has to be constant?
>>>
>>> Hmm.. is the expected behavior to have the interval fluctuating ?
>>> I thought that if the user specified an interval, then he wanted it to be
>>> constant.
>>>
>> That is the behavior in the original linux implementation.
>> Actually there is a mechanism to auto-adjust the ticktime, but it has been
>> removed to powertop prototype quickly delivered, it would be great if it
>> can be
>> recovered. Think about the future tickless kernel, the cpu wakeup times
>> become
>> less, making the ticktime longer makes more sense if the wakeup events is
>> few
>> in the current sampling period.
>
> So you mean adapting the interval based on the amount of wakeups - i.e few
> wakeups, higher interval and vice-versa ?
>
> I'll have a look at the original code.
>
That would be great, thanks, :-)
-Aubrey
