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.
>>> +
>>> + 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.
thanks
Rafael
