linux-kernel - Re: [PATCH] arch/x86/entry/vsyscall/vsyscall_gtod.c: remove __read_mostly from vclocks

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sat, 23 Jun 2018 00:47:12 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Michael Rodin <michael-git@...in.online>
cc:     x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH] arch/x86/entry/vsyscall/vsyscall_gtod.c: remove
 __read_mostly from vclocks_used

Michael,

On Mon, 4 Jun 2018, Michael Rodin wrote:

> The variable "vclocks_used" doesn't appear to be "read mostly".
> Measurements of the access frequency with perf stat [1] and
> perf report show, that approximately half of the accesses to
> this variable are write accesses and happen in update_vsyscall()
> in the file arch/x86/entry/vsyscall/vsyscall_gtod.c.
> The measurements were done with the kernel 4.13.0-43-generic used by
> ubuntu as well as with the stable kernel 4.16.7 with a custom config.
> I've used "perf bench sched" and iperf3 as workloads.
> 
> This was discovered during my master thesis in the CADOS project [2].

Nice find, but ...

The point is that the content of that variable changes once in a blue moon,
so the intent of marking it read_mostly is almost correct. Almost, because
the unconditional write even with the ever repeating same value is not
free. To the best of my knowledge there is no CPU implementation which
supports optimizing silent stores, but I might be just not up to date or
wrong as usual.

So there is a cost to the write and there are two things which have to be
looked at before just doing the obvious knee jerk reaction
's/read_mostly//'.

1) Does it make sense to avoid the write if the value hasn't changed? That
   might be pretty much a free lunch because the variable has to be read in
   the first place in order to OR the supplied value to it before writing
   it back.

2) The variable is in a totally separate cache line than what the following
   stores target, i.e. vsyscall_gtod_data

So it might make sense just to do #1 but as vsyscall_gtod_data is anyway
going to be dirtied it probably makes even more sense to move that
vclocks_used variable into struct vsyscall_gtod_data right away to reduce
the amount of cache lines touched by update_vsyscall().

Care to have a look at that?

Thanks,

	tglx