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.
>
>> +
>> + 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.
Thanks,
-Aubrey
