Received-SPF: pass (google.com: domain of linux-kernel+bounces-81028-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20230503090708.2524310-7-nmi@metaspace.dk> <20230503120354.534136-1-aliceryhl@google.com>
 <87y1b7a994.fsf@metaspace.dk>
In-Reply-To: <87y1b7a994.fsf@metaspace.dk>
From: Alice Ryhl <aliceryhl@google.com>
Date: Mon, 26 Feb 2024 10:15:12 +0100
Message-ID: <CAH5fLgjQm72KmKBtcnBEXOtVqd=Gto5UGF7jrekx8f+7QXh_Ag@mail.gmail.com>
Subject: Re: [RFC PATCH 06/11] rust: apply cache line padding for `SpinLock`
To: "Andreas Hindborg (Samsung)" <nmi@metaspace.dk>
Cc: Damien.LeMoal@wdc.com, alex.gaynor@gmail.com, axboe@kernel.dk, 
	benno.lossin@proton.me, bjorn3_gh@protonmail.com, boqun.feng@gmail.com, 
	gary@garyguo.net, gost.dev@samsung.com, hare@suse.de, hch@lst.de, 
	kbusch@kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, 
	lsf-pc@lists.linux-foundation.org, ojeda@kernel.org, 
	rust-for-linux@vger.kernel.org, wedsonaf@gmail.com, willy@infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 26, 2024 at 10:02=E2=80=AFAM Andreas Hindborg (Samsung)
<nmi@metaspace.dk> wrote:
>
>
> Hi Alice,
>
> Alice Ryhl <aliceryhl@google.com> writes:
>
> > On Wed, 3 May 2023 11:07:03 +0200, Andreas Hindborg <a.hindborg@samsung=
com> wrote:
> >> The kernel `struct spinlock` is 4 bytes on x86 when lockdep is not ena=
bled. The
> >> structure is not padded to fit a cache line. The effect of this for `S=
pinLock`
> >> is that the lock variable and the value protected by the lock will sha=
re 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 rea=
ds to
> >> memory backed devices at 6 concurrent readers.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.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