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

From: Thomas Gleixner
Date: Sun Apr 14 2019 - 07:05:10 EST


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@xxxxxxxxxxxxxxxxxxxxxxx
for an alternate solution to this problem, which avoids this and just gives
CLOCK_MONOTONIC_RAW a separate storage space alltogether.

Thanks,

tglx