[systemd-devel] [PATCH 1/2] bootchart: don't parse /proc/uptime, use CLOCK_BOOTTIME

[Ronny Chevalier]

2014-08-15 22:02 GMT+02:00 Ronny Chevalier <chevalier.ronny at gmail.com>:
> 2014-07-31 10:15 GMT+02:00 Karel Zak <kzak at redhat.com>:
> Hi,
>
>> * systemd-bootchart always parses /proc/uptime, although the
>>   information is unnecessary when --rel specified
>>
>> * use /proc/uptime is overkill, since Linux 2.6.39 we have
>>   clock_gettime(CLOCK_BOOTTIME, ...). The backend on kernel side is
>>   get_monotonic_boottime() in both cases.
>>
>> * main() uses "if (graph_start <= 0.0)" to detect that /proc is
>>   available.
>>
>>   This is fragile solution as graph_start is always smaller than zero
>>   on all systems after suspend/resume (e.g. laptops), because in this
>>   case the system uptime includes suspend time and uptime is always
>>   greater number than monotonic time. For example right now difference
>>   between uptime and monotonic time is 37 hours on my laptop.
>>
>>   Note that main() calls log_uptime() (to parse /proc/uptime) for each
>>   sample when it believes that /proc is not available. So on my laptop
>>   systemd-boochars spends all live with /proc/uptime parsing +
>>   nanosleep(), try
>>
>>     strace  /usr/lib/systemd/systemd-bootchart
>>
>>   to see the never ending loop.
>>
>>   This patch uses access("/proc/vmstat", F_OK) to detect procfs.
>> ---
>>  man/systemd-bootchart.xml |  4 +++-
>>  src/bootchart/bootchart.c | 11 +++++++----
>>  src/bootchart/store.c     | 29 ++++++++++++-----------------
>>  3 files changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/man/systemd-bootchart.xml b/man/systemd-bootchart.xml
>> index e19bbc1..150ca48 100644
>> --- a/man/systemd-bootchart.xml
>> +++ b/man/systemd-bootchart.xml
>> @@ -131,7 +131,9 @@
>>                                  not graph the time elapsed since boot
>>                                  and before systemd-bootchart was
>>                                  started, as it may result in extremely
>> -                                large graphs.  </para></listitem>
>> +                                large graphs. The time elapsed since boot
>> +                                might also include any time that the system
>> +                                was suspended.</para></listitem>
>>                          </varlistentry>
>>                  </variablelist>
>>          </refsect1>
>> diff --git a/src/bootchart/bootchart.c b/src/bootchart/bootchart.c
>> index cbfc28d..909ef46 100644
>> --- a/src/bootchart/bootchart.c
>> +++ b/src/bootchart/bootchart.c
>> @@ -310,6 +310,7 @@ int main(int argc, char *argv[]) {
>>          time_t t = 0;
>>          int r;
>>          struct rlimit rlim;
>> +        bool has_procfs = false;
>>
>>          parse_conf();
>>
>> @@ -349,6 +350,8 @@ int main(int argc, char *argv[]) {
>>
>>          log_uptime();
>>
>> +        has_procfs = access("/proc/vmstat", F_OK) == 0;
>> +
>>          LIST_HEAD_INIT(head);
>>
>>          /* main program loop */
>> @@ -385,11 +388,11 @@ int main(int argc, char *argv[]) {
>>                                  parse_env_file("/usr/lib/os-release", NEWLINE, "PRETTY_NAME", &build, NULL);
>>                  }
>>
>> -                /* wait for /proc to become available, discarding samples */
>> -                if (graph_start <= 0.0)
>> -                        log_uptime();
>> -                else
>> +                if (has_procfs)
>>                          log_sample(samples, &sampledata);
>> +                else
>> +                        /* wait for /proc to become available, discarding samples */
>> +                        has_procfs = access("/proc/vmstat", F_OK) == 0;
>>
>>                  sample_stop = gettime_ns();
>>
>> diff --git a/src/bootchart/store.c b/src/bootchart/store.c
>> index e071983..cedcba8 100644
>> --- a/src/bootchart/store.c
>> +++ b/src/bootchart/store.c
>> @@ -57,27 +57,22 @@ double gettime_ns(void) {
>>          return (n.tv_sec + (n.tv_nsec / 1000000000.0));
>>  }
>>
>> -void log_uptime(void) {
>> -        _cleanup_fclose_ FILE *f = NULL;
>> -        char str[32];
>> -        double uptime;
>> -
>> -        f = fopen("/proc/uptime", "re");
>> -
>> -        if (!f)
>> -                return;
>> -        if (!fscanf(f, "%s %*s", str))
>> -                return;
>> -
>> -        uptime = strtod(str, NULL);
>> +static double gettime_up(void) {
>> +        struct timespec n;
>>
>> -        log_start = gettime_ns();
>> +        clock_gettime(CLOCK_BOOTTIME, &n);
>> +        return (n.tv_sec + (n.tv_nsec / 1000000000.0));
> You can use NSEC_PER_SEC from src/shared/time-util.h instead of 1000000000.0.
>
Oops it has already been applied, sorry.

>> +}
>>
>> -        /* start graph at kernel boot time */
>> +void log_uptime(void) {
>>          if (arg_relative)
>> -                graph_start = log_start;
>> -        else
>> +                graph_start = log_start = gettime_ns();
>> +        else {
>> +                double uptime = gettime_up();
>> +
>> +                log_start = gettime_ns();
>>                  graph_start = log_start - uptime;
>> +        }
>>  }
>>
>>  static char *bufgetline(char *buf) {
>> --
>> 1.9.3
>>
>> _______________________________________________
>> systemd-devel mailing list
>> systemd-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel