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

[Ronny Chevalier]

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.

> +}
>
> -        /* 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