Re: [PATCH 2/3] x86/vdso: Allow clock specific mult and shift values

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, 11 Apr 2019, Huw Davies wrote:

CC+: Vincenzo Frascino who is working on the generic VDSO.

> This will allow clocks with different mult and shift values,
> e.g. CLOCK_MONOTONIC_RAW, to be supported in the vDSO.
> 
> The coarse clocks do not require these data so the values are not
> copied for these clocks.
> 
> One could add potential new values of mult and shift alongside the
> existing values in struct vsyscall_gtod_data, but it seems more
> natural to group them with the actual clock data in the basetime array
> at the expense of a few more cycles in update_vsyscall().

The few cycles are one thing. Did you verify that this does not change the
cache layout for CLOCK_MONOTONIC and CLOCK_REALTIME? Let's look:

>  struct vgtod_ts {
>  	u64		sec;
>  	u64		nsec;
> +	u32		mult;
> +	u32		shift;
>  };
>  
>  #define VGTOD_BASES	(CLOCK_TAI + 1)
> @@ -40,8 +42,6 @@ struct vsyscall_gtod_data {
>  
>  	int		vclock_mode;
>  	u64		cycle_last;
> -	u32		mult;
> -	u32		shift;
>  
>  	struct vgtod_ts	basetime[VGTOD_BASES];

The current state is:

struct vsyscall_gtod_data {
	unsigned int               seq;                  /*     0     4 */
	int                        vclock_mode;          /*     4     4 */
	u64                        cycle_last;           /*     8     8 */
	u64                        mask;                 /*    16     8 */
	u32                        mult;                 /*    24     4 */
	u32                        shift;                /*    28     4 */
	struct vgtod_ts            basetime[12];         /*    32   192 */

       basetime[CLOCK_REALTIME]		32 .. 47
       basetime[CLOCK_MONOTONIC]	48 .. 63

So with your change it looks like this:

struct vsyscall_gtod_data {
	unsigned int               seq;                  /*     0     4 */
	int                        vclock_mode;          /*     4     4 */
	u64                        cycle_last;           /*     8     8 */
	struct vgtod_ts            basetime[12];         /*    16   288 */

which makes

       basetime[CLOCK_REALTIME]		16 .. 39
       basetime[CLOCK_MONOTONIC]	40 .. 63

So it stays in the same cache line, but as we move the VDSO to generic
code, the mask field needs to stay and this will make basetime[CLOCK_MONOTONIC] 
overlap into the next cache line.

See https://lkml.kernel.org/r/alpine.DEB.2.21.1902231727060.1666@nanos.tec.linutronix.de
for an alternate solution to this problem, which avoids this and just gives
CLOCK_MONOTONIC_RAW a separate storage space alltogether.

Thanks,

	tglx