linux-kernel - Re: [RFC PATCH 06/11] rust: apply cache line padding for `SpinLock`

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: Mon, 26 Feb 2024 10:15:12 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: "Andreas Hindborg (Samsung)" <nmi@...aspace.dk>
Cc: Damien.LeMoal@....com, alex.gaynor@...il.com, axboe@...nel.dk, 
	benno.lossin@...ton.me, bjorn3_gh@...tonmail.com, boqun.feng@...il.com, 
	gary@...yguo.net, gost.dev@...sung.com, hare@...e.de, hch@....de, 
	kbusch@...nel.org, linux-block@...r.kernel.org, linux-kernel@...r.kernel.org, 
	lsf-pc@...ts.linux-foundation.org, ojeda@...nel.org, 
	rust-for-linux@...r.kernel.org, wedsonaf@...il.com, willy@...radead.org
Subject: Re: [RFC PATCH 06/11] rust: apply cache line padding for `SpinLock`

On Mon, Feb 26, 2024 at 10:02 AM Andreas Hindborg (Samsung)
<nmi@...aspace.dk> wrote:
>
>
> Hi Alice,
>
> Alice Ryhl <aliceryhl@...gle.com> writes:
>
> > On Wed, 3 May 2023 11:07:03 +0200, Andreas Hindborg <a.hindborg@...sungcom> wrote:
> >> The kernel `struct spinlock` is 4 bytes on x86 when lockdep is not enabled. The
> >> structure is not padded to fit a cache line. The effect of this for `SpinLock`
> >> is that the lock variable and the value protected by the lock will share a cache
> >> line, depending on the alignment requirements of the protected value. Aligning
> >> the lock variable and the protected value to a cache line yields a 20%
> >> performance increase for the Rust null block driver for sequential reads to
> >> memory backed devices at 6 concurrent readers.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@...sung.com>
> >
> > This applies the cacheline padding to all spinlocks unconditionally.
> > It's not clear to me that we want to do that. Instead, I suggest using
> > `SpinLock<CachePadded<T>>` in the null block driver to opt-in to the
> > cache padding there, and let other drivers choose whether or not they
> > want to cache pad their locks.
>
> I was going to write that this is not going to work because the compiler
> is going to reorder the fields of `Lock` and put the `data` field first,
> followed by the `state` field. But I checked the layout, and it seems
> that I actually get the `state` field first (with an alignment of 4), 60
> bytes of padding, and then the `data` field (with alignment 64).
>
> I am wondering why the compiler is not reordering these fields? Am I
> guaranteed that the fields will not be reordered? Looking at the
> definition of `Lock` there does not seem to be anything that prevents
> rustc from swapping `state` and `data`.

It's because `Lock` has `: ?Sized` on the `T` generic. Fields that
might not be Sized must always be last.

Alice