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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1904141229380.4917@nanos.tec.linutronix.de>
Date:   Sun, 14 Apr 2019 12:53:32 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Huw Davies <huw@...eweavers.com>
cc:     linux kernel <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Vincenzo Frascino <vincenzo.frascino@....com>
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