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.1...@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
