Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 14 Apr 2019 12:53:32 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Huw Davies <huw@codeweavers.com>
cc:     linux kernel <linux-kernel@vger.kernel.org>,
        Andy Lutomirski <luto@kernel.org>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>
Subject: Re: [PATCH 2/3] x86/vdso: Allow clock specific mult and shift
 values
In-Reply-To: <20190411101205.10006-3-huw@codeweavers.com>
Message-ID: <alpine.DEB.2.21.1904141229380.4917@nanos.tec.linutronix.de>
References: <20190411101205.10006-1-huw@codeweavers.com> <20190411101205.10006-3-huw@codeweavers.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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