On Mon, Feb 26, 2024 at 10:02 AM Andreas Hindborg (Samsung) <nmi@xxxxxxxxxxxx> wrote: > > > Hi Alice, > > Alice Ryhl <aliceryhl@xxxxxxxxxx> writes: > > > On Wed, 3 May 2023 11:07:03 +0200, Andreas Hindborg <a.hindborg@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > > > 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
