[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210403155148.0f5c94ff@sf>
Date: Sat, 3 Apr 2021 15:51:48 +0100
From: Sergei Trofimovich <slyfox@...too.org>
To: "Elliott, Robert (Servers)" <elliott@....com>
Cc: Arnd Bergmann <arnd@...nel.org>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
"John Paul Adrian Glaubitz" <glaubitz@...sik.fu-berlin.de>,
Don Brace - C33706 <don.brace@...rochip.com>,
"linux-ia64@...r.kernel.org" <linux-ia64@...r.kernel.org>,
"storagedev@...rochip.com" <storagedev@...rochip.com>,
linux-scsi <linux-scsi@...r.kernel.org>,
"jszczype@...hat.com" <jszczype@...hat.com>,
Scott Benesh <scott.benesh@...rochip.com>,
Scott Teel <scott.teel@...rochip.com>,
"thenzl@...hat.com" <thenzl@...hat.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] hpsa: add an assert to prevent from __packed
reintroduction
On Fri, 2 Apr 2021 14:40:39 +0000
"Elliott, Robert (Servers)" <elliott@....com> wrote:
> It looks like ia64 implements atomic_t as a 64-bit value and expects atomic_t
> to be 64-bit aligned, but does nothing to ensure that.
>
> For x86, atomic_t is a 32-bit value and atomic64_t is a 64-bit value, and
> the definition of atomic64_t is overridden in a way that ensures
> 64-bit (8 byte) alignment:
>
> Generic definitions are in include/linux/types.h:
> typedef struct {
> int counter;
> } atomic_t;
>
> #define ATOMIC_INIT(i) { (i) }
>
> #ifdef CONFIG_64BIT
> typedef struct {
> s64 counter;
> } atomic64_t;
> #endif
>
> Override in arch/x86/include/asm/atomic64_32.h:
> typedef struct {
> s64 __aligned(8) counter;
> } atomic64_t;
>
> Perhaps ia64 needs to take over the definition of both atomic_t and atomic64_t
> and do the same?
I don't think it's needed. ia64 is a 64-bit arch with expected
natural alignment for s64: alignof(s64)=8.
Also if my understanding is correct adding __aligned(8) would not fix
use case of embedding locks into packed structs even on x86_64 (or i386):
$ cat a.c
#include <stdio.h>
#include <stddef.h>
typedef struct { unsigned long long __attribute__((aligned(8))) l; } lock_t;
struct s { char c; lock_t lock; } __attribute__((packed));
int main() { printf ("offsetof(struct s, lock) = %lu\nsizeof(struct s) = %lu\n", offsetof(struct s, lock), sizeof(struct s)); }
$ x86_64-pc-linux-gnu-gcc a.c -o a && ./a
offsetof(struct s, lock) = 1
sizeof(struct s) = 9
$ x86_64-pc-linux-gnu-gcc a.c -o a -m32 && ./a
offsetof(struct s, lock) = 1
sizeof(struct s) = 9
Note how alignment of 'lock' stays 1 byte in both cases.
8-byte alignment added for i386 in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bbf2a330d92c5afccfd17592ba9ccd50f41cf748
is only as a performance optimization (not a correctness fix).
Larger alignment on i386 is preferred because alignof(s64)=4 on
that target which might make atomic op span cache lines that
leads to performance degradation.
--
Sergei
Powered by blists - more mailing lists