Messages in this thread | | | Date | Sun, 14 Apr 2019 12:53:32 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/3] x86/vdso: Allow clock specific mult and shift values |
| |
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
| |