Re: [PATCH 1/5] arm64: Fix vDSO clock_getres()

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

Hi Catalin,

On 15/04/2019 18:35, Catalin Marinas wrote:
> On Mon, Apr 01, 2019 at 12:51:48PM +0100, Vincenzo Frascino wrote:
>> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
>> index 2d419006ad43..47ba72345739 100644
>> --- a/arch/arm64/kernel/vdso.c
>> +++ b/arch/arm64/kernel/vdso.c
>> @@ -245,6 +245,8 @@ void update_vsyscall(struct timekeeper *tk)
>>  		vdso_data->cs_shift		= tk->tkr_mono.shift;
>>  	}
>>  
>> +	vdso_data->hrtimer_res			= hrtimer_resolution;
>> +
>>  	smp_wmb();
>>  	++vdso_data->tb_seq_count;
>>  }
>> diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
>> index c39872a7b03c..7a2cd2f8e13a 100644
>> --- a/arch/arm64/kernel/vdso/gettimeofday.S
>> +++ b/arch/arm64/kernel/vdso/gettimeofday.S
>> @@ -296,32 +296,35 @@ ENDPROC(__kernel_clock_gettime)
>>  /* int __kernel_clock_getres(clockid_t clock_id, struct timespec *res); */
>>  ENTRY(__kernel_clock_getres)
>>  	.cfi_startproc
>> +	adr	vdso_data, _vdso_data
>>  	cmp	w0, #CLOCK_REALTIME
>>  	ccmp	w0, #CLOCK_MONOTONIC, #0x4, ne
>>  	ccmp	w0, #CLOCK_MONOTONIC_RAW, #0x4, ne
>> -	b.ne	1f
>> +	b.ne	2f
>>  
>> -	ldr	x2, 5f
>> -	b	2f
>> -1:
>> +1:	/* Get hrtimer_res */
>> +	seqcnt_acquire
>> +	syscall_check fail=5f
>> +	ldr	x2, [vdso_data, #CLOCK_REALTIME_RES]
>> +	seqcnt_check fail=1b
>> +	b	3f
>> +2:
> 
> We talked briefly but I'm still confused why we need the fallback to the
> syscall here if archdata.vdso_direct is false. Is it because if the
> timer driver code sets vdso_direct to false, we don't don't support
> highres timers? If my understanding is correct, you may want to move the
> hrtimer_res setting in update_vsyscall() to the !use_syscall block.
> 

Ok, so let me try to provide more details on what I mentioned yesterday:
- clock_getres syscall follows the rules of what defined in posix-timers.c
- based on the clock_id that, for this purpose, can be separated in coarse and
non-coarse calls either posix_get_coarse_res() or posix_get_hrtimer_res().
- if clock id is set to a coarse clock and posix_get_coarse_res() is invoked,
happens what follows:

static int posix_get_coarse_res(const clockid_t which_clock,
				struct timespec64 *tp)
{
	*tp = ktime_to_timespec64(KTIME_LOW_RES);
	return 0;
}

Note that since CONFIG_1HZ seems not supported (jiffies.h) by the kernel in this
case we do not need rounding in our vDSO implementation.

- if clock id is set to non-coarse and posix_get_hrtimer_res() is invoked,
happens the following:

static int posix_get_hrtimer_res(clockid_t which_clock, struct timespec64 *tp)
{
	tp->tv_sec = 0;
	tp->tv_nsec = hrtimer_resolution;
	return 0;
}

hrtimer_resolution can be high res or low res depending on the call of
hrtimer_switch_to_hres(). For us the only way to preserve the correct value is
to keep it in the vdso data page.

- The assembly code mimics exactly the same behaviour detailed above, with one
difference: the one related to the use_syscall parameter which is specific to arm64.
The use_syscall parameter is set by arm_arch_timer and consumed by
update_vsyscall(). To mirror what update_vsyscall does in update_vsyscall() I
check "syscall_check fail=5f" in clock_getres vdso function.

Said that, even if functionally it is the same thing, I think it is logically
more correct to have hrtimer_res setting inside the !use_syscall block, hence I
am going to change it in the next iteration.

Please let me know your thoughts.

-- 
Regards,
Vincenzo