Re: [PATCH v2] linux-user: Implement starttime field in self stat emulation

[Laurent Vivier]

Le 27/01/2022 à 21:58, Cameron Esfahani a écrit :
> Instead of always returning 0, return actual starttime.
>
> v2: Use clock_gettime() instead of scanning /proc/self/stat
>
> Signed-off-by: Cameron Esfahani <dirty@apple.com>
> ---
>   linux-user/main.c    | 11 +++++++++++
>   linux-user/qemu.h    |  3 +++
>   linux-user/syscall.c |  3 +++
>   3 files changed, 17 insertions(+)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 16def5215d..2e5e0e886b 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -196,6 +196,17 @@ void init_task_state(TaskState *ts)
>           .ss_size = 0,
>           .ss_flags = TARGET_SS_DISABLE,
>       };
> +
> +    /* Capture task start time relative to system boot */
> +
> +    long long ticksPerSec = sysconf(_SC_CLK_TCK);

I don't think we need a "long long" here. sysconf() returns "long".

Use lower case with underscore

https://qemu-project.gitlab.io/qemu/devel/style.html#variable-naming-conventions

Put the declaration at the beginning of the function

https://qemu-project.gitlab.io/qemu/devel/style.html#declarations

> +    struct timespec bt;
> +
> +    if ((ticksPerSec > 0) && !clock_gettime(CLOCK_BOOTTIME, &bt)) {
> +        /* start_boottime is expressed in clock ticks */
> +        ts->start_boottime = bt.tv_sec * ticksPerSec;

You can cast ticksPerSec with uint64_t to have a 64bit result.

> +        ts->start_boottime += bt.tv_nsec * ticksPerSec / 1000000000L;

Use NANOSECONDS_PER_SECOND

> +    }
>   }
>   
>   CPUArchState *cpu_copy(CPUArchState *env)
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 7910ce59cc..86bc169e72 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -158,6 +158,9 @@ typedef struct TaskState {
>   
>       /* This thread's sigaltstack, if it has one */
>       struct target_sigaltstack sigaltstack_used;
> +
> +    /* Start time of task after system boot in clock ticks */
> +    unsigned long long start_boottime;

Use uint64_t, like the kernel does for start_boottime.

>   } __attribute__((aligned(16))) TaskState;
>   
>   abi_long do_brk(abi_ulong new_brk);
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 5950222a77..2f77dbdda7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8107,6 +8107,9 @@ static int open_self_stat(void *cpu_env, int fd)
>           } else if (i == 3) {
>               /* ppid */
>               g_string_printf(buf, FMT_pid " ", getppid());
> +        } else if (i == 21) {
> +            /* starttime */
> +            g_string_printf(buf, "%llu ", ts->start_boottime);

with uint64_t, use "%PRIu64 ".

>           } else if (i == 27) {
>               /* stack bottom */
>               g_string_printf(buf, TARGET_ABI_FMT_ld " ", 
> ts->info->start_stack);

Except these minor tweaks, it looks good.

Thanks,
Laurent