* [PATCH v3] devres: Explicitly align datai[] to 64-bit @ 2018-07-09 13:45 Alexey Brodkin 2018-07-09 13:45 ` Alexey Brodkin ` (2 more replies) 0 siblings, 3 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 13:45 UTC (permalink / raw) To: linux-kernel Cc: linux-snps-arc, linux-arch, Alexey Brodkin, Greg Kroah-Hartman, Geert Uytterhoeven, David Laight, Peter Zijlstra, Thomas Gleixner, Will Deacon, Greg KH, stable data[] must be 64-bit aligned even on 32-bit architectures because it might be accessed by instructions that require aligned memory arguments. One example is "atomic64_t" type accessed by special atomic instructions which may read/write entire 64-bit word. Atomic instructions are a bit special compared to normal loads and stores. Even if normal loads and stores may deal with unaligned data, atomic instructions still require data to be aligned because it's hard to manage atomic value that spans through multiple cache lines or even MMU pages. And hardware just raises an alignment fault exception. The problem with previously used approach is that depending on ABI "long long" type of a particular 32-bit CPU might be aligned to 8-, 16-, 32- or 64-bit boundary. Which will get in the way of mentioned above atomic instructions. Consider the following snippet: | struct mystruct { | atomic64_t myvar; | } | | struct mystruct *p; | p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); Here address of "myvar" will match data[] in "struct devres", that said if "data" is not 64-bit aligned atomic instruction will fail on the first access to "myvar". Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: David Laight <David.Laight@ACULAB.COM> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Will Deacon <will.deacon@arm.com> Cc: Greg KH <greg@kroah.com> Cc: <stable@vger.kernel.org> # 4.8+ --- Changes v2 -> v3: * Align explicitly to 8 bytes [David] * Rephrased in-line comment [David] * Added more techinical details to commit message [Greg] * Mention more alignment options in commit message [Geert] Changes v1 -> v2: * Reworded commit message * Inserted comment right in source [Thomas] drivers/base/devres.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/base/devres.c b/drivers/base/devres.c index f98a097e73f2..d65327cb83c9 100644 --- a/drivers/base/devres.c +++ b/drivers/base/devres.c @@ -24,8 +24,12 @@ struct devres_node { struct devres { struct devres_node node; - /* -- 3 pointers */ - unsigned long long data[]; /* guarantee ull alignment */ + /* + * data[] must be 64 bit aligned even on 32 bit architectures + * because it might be accessed by instructions that require + * aligned memory arguments such as atomic64_t. + */ + u8 __aligned(8) data[]; }; struct devres_group { -- 2.17.1 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 13:45 [PATCH v3] devres: Explicitly align datai[] to 64-bit Alexey Brodkin @ 2018-07-09 13:45 ` Alexey Brodkin 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 14:07 ` Peter Zijlstra 2 siblings, 0 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 13:45 UTC (permalink / raw) To: linux-kernel Cc: linux-snps-arc, linux-arch, Alexey Brodkin, Greg Kroah-Hartman, Geert Uytterhoeven, David Laight, Peter Zijlstra, Thomas Gleixner, Will Deacon, Greg KH, stable data[] must be 64-bit aligned even on 32-bit architectures because it might be accessed by instructions that require aligned memory arguments. One example is "atomic64_t" type accessed by special atomic instructions which may read/write entire 64-bit word. Atomic instructions are a bit special compared to normal loads and stores. Even if normal loads and stores may deal with unaligned data, atomic instructions still require data to be aligned because it's hard to manage atomic value that spans through multiple cache lines or even MMU pages. And hardware just raises an alignment fault exception. The problem with previously used approach is that depending on ABI "long long" type of a particular 32-bit CPU might be aligned to 8-, 16-, 32- or 64-bit boundary. Which will get in the way of mentioned above atomic instructions. Consider the following snippet: | struct mystruct { | atomic64_t myvar; | } | | struct mystruct *p; | p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); Here address of "myvar" will match data[] in "struct devres", that said if "data" is not 64-bit aligned atomic instruction will fail on the first access to "myvar". Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: David Laight <David.Laight@ACULAB.COM> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Will Deacon <will.deacon@arm.com> Cc: Greg KH <greg@kroah.com> Cc: <stable@vger.kernel.org> # 4.8+ --- Changes v2 -> v3: * Align explicitly to 8 bytes [David] * Rephrased in-line comment [David] * Added more techinical details to commit message [Greg] * Mention more alignment options in commit message [Geert] Changes v1 -> v2: * Reworded commit message * Inserted comment right in source [Thomas] drivers/base/devres.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/base/devres.c b/drivers/base/devres.c index f98a097e73f2..d65327cb83c9 100644 --- a/drivers/base/devres.c +++ b/drivers/base/devres.c @@ -24,8 +24,12 @@ struct devres_node { struct devres { struct devres_node node; - /* -- 3 pointers */ - unsigned long long data[]; /* guarantee ull alignment */ + /* + * data[] must be 64 bit aligned even on 32 bit architectures + * because it might be accessed by instructions that require + * aligned memory arguments such as atomic64_t. + */ + u8 __aligned(8) data[]; }; struct devres_group { -- 2.17.1 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 13:45 [PATCH v3] devres: Explicitly align datai[] to 64-bit Alexey Brodkin 2018-07-09 13:45 ` Alexey Brodkin @ 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 13:54 ` Peter Zijlstra ` (2 more replies) 2018-07-09 14:07 ` Peter Zijlstra 2 siblings, 3 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 13:54 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-arch, Greg Kroah-Hartman, Will Deacon, linux-kernel, stable, David Laight, Geert Uytterhoeven, Greg KH, Thomas Gleixner, linux-snps-arc On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index f98a097e73f2..d65327cb83c9 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -24,8 +24,12 @@ struct devres_node { > > struct devres { > struct devres_node node; > - /* -- 3 pointers */ > - unsigned long long data[]; /* guarantee ull alignment */ > + /* > + * data[] must be 64 bit aligned even on 32 bit architectures > + * because it might be accessed by instructions that require > + * aligned memory arguments such as atomic64_t. > + */ > + u8 __aligned(8) data[]; > }; From a quick reading in Documentation/driver-model/devres.txt this devres muck is supposed to be device memory, right? atomics (as in atomic*_t) are not defined for use on mmio. wth kind of code is relying on this? ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 13:54 ` Peter Zijlstra @ 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 14:04 ` Mark Rutland 2018-07-09 14:07 ` Alexey Brodkin 2 siblings, 0 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 13:54 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-kernel, linux-snps-arc, linux-arch, Greg Kroah-Hartman, Geert Uytterhoeven, David Laight, Thomas Gleixner, Will Deacon, Greg KH, stable On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index f98a097e73f2..d65327cb83c9 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -24,8 +24,12 @@ struct devres_node { > > struct devres { > struct devres_node node; > - /* -- 3 pointers */ > - unsigned long long data[]; /* guarantee ull alignment */ > + /* > + * data[] must be 64 bit aligned even on 32 bit architectures > + * because it might be accessed by instructions that require > + * aligned memory arguments such as atomic64_t. > + */ > + u8 __aligned(8) data[]; > }; ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 13:54 ` Peter Zijlstra @ 2018-07-09 14:04 ` Mark Rutland 2018-07-09 14:04 ` Mark Rutland 2018-07-09 14:10 ` Geert Uytterhoeven 2018-07-09 14:07 ` Alexey Brodkin 2 siblings, 2 replies; 38+ messages in thread From: Mark Rutland @ 2018-07-09 14:04 UTC (permalink / raw) To: Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, linux-snps-arc, linux-arch, Greg Kroah-Hartman, Geert Uytterhoeven, David Laight, Thomas Gleixner, Will Deacon, Greg KH, stable On Mon, Jul 09, 2018 at 03:54:09PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > index f98a097e73f2..d65327cb83c9 100644 > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * data[] must be 64 bit aligned even on 32 bit architectures > > + * because it might be accessed by instructions that require > > + * aligned memory arguments such as atomic64_t. > > + */ > > + u8 __aligned(8) data[]; > > }; > > From a quick reading in Documentation/driver-model/devres.txt this > devres muck is supposed to be device memory, right? It's for associating resources (e.g. memory allocations) with a struct device. e.g. you do: devm_kmalloc(dev, size, GFP_KERNEL); ... and that allocates sizeof(struct devres) + size, putting some accounting data into that devres, and returning a pointer to the remaining size bytes. The data[] thing is a hack to ensure that the structure is padded to 64-bit alignment, in case you'd done: struct foo { atomic64_t counter; } struct foo *f = devm_kmalloc(dev, sizeof(*f), GFP_KERNEL); Mark. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:04 ` Mark Rutland @ 2018-07-09 14:04 ` Mark Rutland 2018-07-09 14:10 ` Geert Uytterhoeven 1 sibling, 0 replies; 38+ messages in thread From: Mark Rutland @ 2018-07-09 14:04 UTC (permalink / raw) To: Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, linux-snps-arc, linux-arch, Greg Kroah-Hartman, Geert Uytterhoeven, David Laight, Thomas Gleixner, Will Deacon, Greg KH, stable On Mon, Jul 09, 2018 at 03:54:09PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > index f98a097e73f2..d65327cb83c9 100644 > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * data[] must be 64 bit aligned even on 32 bit architectures > > + * because it might be accessed by instructions that require > > + * aligned memory arguments such as atomic64_t. > > + */ > > + u8 __aligned(8) data[]; > > }; > > From a quick reading in Documentation/driver-model/devres.txt this > devres muck is supposed to be device memory, right? It's for associating resources (e.g. memory allocations) with a struct device. e.g. you do: devm_kmalloc(dev, size, GFP_KERNEL); ... and that allocates sizeof(struct devres) + size, putting some accounting data into that devres, and returning a pointer to the remaining size bytes. The data[] thing is a hack to ensure that the structure is padded to 64-bit alignment, in case you'd done: struct foo { atomic64_t counter; } struct foo *f = devm_kmalloc(dev, sizeof(*f), GFP_KERNEL); Mark. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:04 ` Mark Rutland 2018-07-09 14:04 ` Mark Rutland @ 2018-07-09 14:10 ` Geert Uytterhoeven 2018-07-09 14:10 ` Geert Uytterhoeven 1 sibling, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2018-07-09 14:10 UTC (permalink / raw) To: Mark Rutland Cc: Peter Zijlstra, Alexey Brodkin, Linux Kernel Mailing List, arcml, Linux-Arch, Greg KH, David Laight, Thomas Gleixner, Will Deacon, Greg KH, stable On Mon, Jul 9, 2018 at 4:04 PM Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jul 09, 2018 at 03:54:09PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > > index f98a097e73f2..d65327cb83c9 100644 > > > --- a/drivers/base/devres.c > > > +++ b/drivers/base/devres.c > > > @@ -24,8 +24,12 @@ struct devres_node { > > > > > > struct devres { > > > struct devres_node node; > > > - /* -- 3 pointers */ > > > - unsigned long long data[]; /* guarantee ull alignment */ > > > + /* > > > + * data[] must be 64 bit aligned even on 32 bit architectures > > > + * because it might be accessed by instructions that require > > > + * aligned memory arguments such as atomic64_t. > > > + */ > > > + u8 __aligned(8) data[]; > > > }; > > > > From a quick reading in Documentation/driver-model/devres.txt this > > devres muck is supposed to be device memory, right? > > It's for associating resources (e.g. memory allocations) with a struct > device. > > e.g. you do: > > devm_kmalloc(dev, size, GFP_KERNEL); > > ... and that allocates sizeof(struct devres) + size, putting some > accounting data into that devres, and returning a pointer to the > remaining size bytes. > > The data[] thing is a hack to ensure that the structure is padded to > 64-bit alignment, in case you'd done: > > struct foo { > atomic64_t counter; > } > > struct foo *f = devm_kmalloc(dev, sizeof(*f), GFP_KERNEL); So the big issue is that the minimum alignment of a buffer allocated with devm_kmalloc() and friends is different (lower) than when allocated with kmalloc(). On 32-bit, it's only aligned to 4 bytes. Ugh. I wouldn't be surprised if some callers assume it to be cacheline-aligned... Which means blind conversions to the devm_*() versions can be dangerous. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:10 ` Geert Uytterhoeven @ 2018-07-09 14:10 ` Geert Uytterhoeven 0 siblings, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2018-07-09 14:10 UTC (permalink / raw) To: Mark Rutland Cc: Peter Zijlstra, Alexey Brodkin, Linux Kernel Mailing List, arcml, Linux-Arch, Greg KH, David Laight, Thomas Gleixner, Will Deacon, Greg KH, stable On Mon, Jul 9, 2018 at 4:04 PM Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jul 09, 2018 at 03:54:09PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > > index f98a097e73f2..d65327cb83c9 100644 > > > --- a/drivers/base/devres.c > > > +++ b/drivers/base/devres.c > > > @@ -24,8 +24,12 @@ struct devres_node { > > > > > > struct devres { > > > struct devres_node node; > > > - /* -- 3 pointers */ > > > - unsigned long long data[]; /* guarantee ull alignment */ > > > + /* > > > + * data[] must be 64 bit aligned even on 32 bit architectures > > > + * because it might be accessed by instructions that require > > > + * aligned memory arguments such as atomic64_t. > > > + */ > > > + u8 __aligned(8) data[]; > > > }; > > > > From a quick reading in Documentation/driver-model/devres.txt this > > devres muck is supposed to be device memory, right? > > It's for associating resources (e.g. memory allocations) with a struct > device. > > e.g. you do: > > devm_kmalloc(dev, size, GFP_KERNEL); > > ... and that allocates sizeof(struct devres) + size, putting some > accounting data into that devres, and returning a pointer to the > remaining size bytes. > > The data[] thing is a hack to ensure that the structure is padded to > 64-bit alignment, in case you'd done: > > struct foo { > atomic64_t counter; > } > > struct foo *f = devm_kmalloc(dev, sizeof(*f), GFP_KERNEL); So the big issue is that the minimum alignment of a buffer allocated with devm_kmalloc() and friends is different (lower) than when allocated with kmalloc(). On 32-bit, it's only aligned to 4 bytes. Ugh. I wouldn't be surprised if some callers assume it to be cacheline-aligned... Which means blind conversions to the devm_*() versions can be dangerous. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 14:04 ` Mark Rutland @ 2018-07-09 14:07 ` Alexey Brodkin 2018-07-09 14:07 ` Alexey Brodkin 2 siblings, 1 reply; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 14:07 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, daniel.vetter, airlied, greg, will.deacon, gregkh, David.Laight, l.stach, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 15:54 +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > index f98a097e73f2..d65327cb83c9 100644 > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * data[] must be 64 bit aligned even on 32 bit architectures > > + * because it might be accessed by instructions that require > > + * aligned memory arguments such as atomic64_t. > > + */ > > + u8 __aligned(8) data[]; > > }; > > From a quick reading in Documentation/driver-model/devres.txt this > devres muck is supposed to be device memory, right? > > atomics (as in atomic*_t) are not defined for use on mmio. > > wth kind of code is relying on this? It's Etnaviv (GPU/DRM) driver in etnaviv_gpu_platform_probe(), see https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/etnaviv/etnaviv_gpu.c#L1740: ---------------------->8--------------------- struct drm_gpu_scheduler { ... atomic64_t job_id_count; ... }; struct etnaviv_gpu { ... struct drm_gpu_scheduler sched; }; struct etnaviv_gpu *gpu; gpu = devm_kzalloc(dev, sizeof(*gpu), GFP_KERNEL); ---------------------->8--------------------- -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:07 ` Alexey Brodkin @ 2018-07-09 14:07 ` Alexey Brodkin 0 siblings, 0 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 14:07 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, daniel.vetter, airlied, greg, will.deacon, gregkh, David.Laight, l.stach, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 15:54 +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > > index f98a097e73f2..d65327cb83c9 100644 > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * data[] must be 64 bit aligned even on 32 bit architectures > > + * because it might be accessed by instructions that require > > + * aligned memory arguments such as atomic64_t. > > + */ > > + u8 __aligned(8) data[]; > > }; > > From a quick reading in Documentation/driver-model/devres.txt this > devres muck is supposed to be device memory, right? > > atomics (as in atomic*_t) are not defined for use on mmio. > > wth kind of code is relying on this? It's Etnaviv (GPU/DRM) driver in etnaviv_gpu_platform_probe(), see https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/etnaviv/etnaviv_gpu.c#L1740: ---------------------->8--------------------- struct drm_gpu_scheduler { ... atomic64_t job_id_count; ... }; struct etnaviv_gpu { ... struct drm_gpu_scheduler sched; }; struct etnaviv_gpu *gpu; gpu = devm_kzalloc(dev, sizeof(*gpu), GFP_KERNEL); ---------------------->8--------------------- -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 13:45 [PATCH v3] devres: Explicitly align datai[] to 64-bit Alexey Brodkin 2018-07-09 13:45 ` Alexey Brodkin 2018-07-09 13:54 ` Peter Zijlstra @ 2018-07-09 14:07 ` Peter Zijlstra 2018-07-09 14:07 ` Peter Zijlstra 2018-07-09 14:10 ` Peter Zijlstra 2 siblings, 2 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 14:07 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-arch, Greg Kroah-Hartman, Will Deacon, linux-kernel, stable, David Laight, Geert Uytterhoeven, Greg KH, Thomas Gleixner, linux-snps-arc On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -24,8 +24,12 @@ struct devres_node { > > struct devres { > struct devres_node node; > - /* -- 3 pointers */ > - unsigned long long data[]; /* guarantee ull alignment */ > + /* > + * data[] must be 64 bit aligned even on 32 bit architectures > + * because it might be accessed by instructions that require > + * aligned memory arguments such as atomic64_t. > + */ > + u8 __aligned(8) data[]; > }; Seeing that this ends up in a semi generic allocation thing, I don't feel this should be different from ARCH_KMALLOC_MINALIGN. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:07 ` Peter Zijlstra @ 2018-07-09 14:07 ` Peter Zijlstra 2018-07-09 14:10 ` Peter Zijlstra 1 sibling, 0 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 14:07 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-kernel, linux-snps-arc, linux-arch, Greg Kroah-Hartman, Geert Uytterhoeven, David Laight, Thomas Gleixner, Will Deacon, Greg KH, stable On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -24,8 +24,12 @@ struct devres_node { > > struct devres { > struct devres_node node; > - /* -- 3 pointers */ > - unsigned long long data[]; /* guarantee ull alignment */ > + /* > + * data[] must be 64 bit aligned even on 32 bit architectures > + * because it might be accessed by instructions that require > + * aligned memory arguments such as atomic64_t. > + */ > + u8 __aligned(8) data[]; > }; Seeing that this ends up in a semi generic allocation thing, I don't feel this should be different from ARCH_KMALLOC_MINALIGN. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:07 ` Peter Zijlstra 2018-07-09 14:07 ` Peter Zijlstra @ 2018-07-09 14:10 ` Peter Zijlstra 2018-07-09 14:10 ` Peter Zijlstra 2018-07-09 14:33 ` Alexey Brodkin 1 sibling, 2 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 14:10 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-arch, Greg Kroah-Hartman, Will Deacon, linux-kernel, stable, David Laight, Geert Uytterhoeven, Greg KH, Thomas Gleixner, linux-snps-arc On Mon, Jul 09, 2018 at 04:07:17PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * data[] must be 64 bit aligned even on 32 bit architectures > > + * because it might be accessed by instructions that require > > + * aligned memory arguments such as atomic64_t. > > + */ > > + u8 __aligned(8) data[]; > > }; > > Seeing that this ends up in a semi generic allocation thing, I don't > feel this should be different from ARCH_KMALLOC_MINALIGN. In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, it is impossible to guarantee a larger alignment than kmalloc does. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:10 ` Peter Zijlstra @ 2018-07-09 14:10 ` Peter Zijlstra 2018-07-09 14:33 ` Alexey Brodkin 1 sibling, 0 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 14:10 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-kernel, linux-snps-arc, linux-arch, Greg Kroah-Hartman, Geert Uytterhoeven, David Laight, Thomas Gleixner, Will Deacon, Greg KH, stable On Mon, Jul 09, 2018 at 04:07:17PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * data[] must be 64 bit aligned even on 32 bit architectures > > + * because it might be accessed by instructions that require > > + * aligned memory arguments such as atomic64_t. > > + */ > > + u8 __aligned(8) data[]; > > }; > > Seeing that this ends up in a semi generic allocation thing, I don't > feel this should be different from ARCH_KMALLOC_MINALIGN. In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, it is impossible to guarantee a larger alignment than kmalloc does. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:10 ` Peter Zijlstra 2018-07-09 14:10 ` Peter Zijlstra @ 2018-07-09 14:33 ` Alexey Brodkin 2018-07-09 14:33 ` Alexey Brodkin 2018-07-09 14:49 ` Peter Zijlstra 1 sibling, 2 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 14:33 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 16:10 +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:07:17PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > > --- a/drivers/base/devres.c > > > +++ b/drivers/base/devres.c > > > @@ -24,8 +24,12 @@ struct devres_node { > > > > > > struct devres { > > > struct devres_node node; > > > - /* -- 3 pointers */ > > > - unsigned long long data[]; /* guarantee ull alignment */ > > > + /* > > > + * data[] must be 64 bit aligned even on 32 bit architectures > > > + * because it might be accessed by instructions that require > > > + * aligned memory arguments such as atomic64_t. > > > + */ > > > + u8 __aligned(8) data[]; > > > }; > > > > Seeing that this ends up in a semi generic allocation thing, I don't > > feel this should be different from ARCH_KMALLOC_MINALIGN. > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > it is impossible to guarantee a larger alignment than kmalloc does. Well but 4-bytes [which is critical for atomic64_t] should be much less than a sane cache line length so above should work. -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:33 ` Alexey Brodkin @ 2018-07-09 14:33 ` Alexey Brodkin 2018-07-09 14:49 ` Peter Zijlstra 1 sibling, 0 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 14:33 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 16:10 +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:07:17PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote: > > > --- a/drivers/base/devres.c > > > +++ b/drivers/base/devres.c > > > @@ -24,8 +24,12 @@ struct devres_node { > > > > > > struct devres { > > > struct devres_node node; > > > - /* -- 3 pointers */ > > > - unsigned long long data[]; /* guarantee ull alignment */ > > > + /* > > > + * data[] must be 64 bit aligned even on 32 bit architectures > > > + * because it might be accessed by instructions that require > > > + * aligned memory arguments such as atomic64_t. > > > + */ > > > + u8 __aligned(8) data[]; > > > }; > > > > Seeing that this ends up in a semi generic allocation thing, I don't > > feel this should be different from ARCH_KMALLOC_MINALIGN. > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > it is impossible to guarantee a larger alignment than kmalloc does. Well but 4-bytes [which is critical for atomic64_t] should be much less than a sane cache line length so above should work. -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:33 ` Alexey Brodkin 2018-07-09 14:33 ` Alexey Brodkin @ 2018-07-09 14:49 ` Peter Zijlstra 2018-07-09 14:49 ` Peter Zijlstra ` (3 more replies) 1 sibling, 4 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 14:49 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > it is impossible to guarantee a larger alignment than kmalloc does. > > Well but 4-bytes [which is critical for atomic64_t] should be much less > than a sane cache line length so above should work. AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the thing). So unconditionally setting the alignment of devres::data to 8 seems broken. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:49 ` Peter Zijlstra @ 2018-07-09 14:49 ` Peter Zijlstra 2018-07-09 14:53 ` Alexey Brodkin ` (2 subsequent siblings) 3 siblings, 0 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 14:49 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > it is impossible to guarantee a larger alignment than kmalloc does. > > Well but 4-bytes [which is critical for atomic64_t] should be much less > than a sane cache line length so above should work. AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the thing). So unconditionally setting the alignment of devres::data to 8 seems broken. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:49 ` Peter Zijlstra 2018-07-09 14:49 ` Peter Zijlstra @ 2018-07-09 14:53 ` Alexey Brodkin 2018-07-09 14:53 ` Alexey Brodkin 2018-10-04 16:19 ` Alexey Brodkin 2018-07-09 15:02 ` David Laight 2018-07-09 15:29 ` Mark Rutland 3 siblings, 2 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 14:53 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, will.deacon, greg, gregkh, David.Laight, Vineet Gupta, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 16:49 +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > than a sane cache line length so above should work. > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > thing). > > So unconditionally setting the alignment of devres::data to 8 seems > broken. Well but then what other options do we have to fix a problem with misaligned access to atomic64_t in drm_gpu_scheduler? -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:53 ` Alexey Brodkin @ 2018-07-09 14:53 ` Alexey Brodkin 2018-10-04 16:19 ` Alexey Brodkin 1 sibling, 0 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-07-09 14:53 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, will.deacon, greg, gregkh, David.Laight, Vineet Gupta, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 16:49 +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > than a sane cache line length so above should work. > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > thing). > > So unconditionally setting the alignment of devres::data to 8 seems > broken. Well but then what other options do we have to fix a problem with misaligned access to atomic64_t in drm_gpu_scheduler? -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:53 ` Alexey Brodkin 2018-07-09 14:53 ` Alexey Brodkin @ 2018-10-04 16:19 ` Alexey Brodkin 2018-10-04 16:19 ` Alexey Brodkin 1 sibling, 1 reply; 38+ messages in thread From: Alexey Brodkin @ 2018-10-04 16:19 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, will.deacon, greg, gregkh, David.Laight, Vineet Gupta, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 17:53 +0300, Alexey Brodkin wrote: > Hi Peter, > > On Mon, 2018-07-09 at 16:49 +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > > than a sane cache line length so above should work. > > > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > > thing). > > > > So unconditionally setting the alignment of devres::data to 8 seems > > broken. > > Well but then what other options do we have to fix a problem with > misaligned access to atomic64_t in drm_gpu_scheduler? Ping! Should I send v4 with ARCH_KMALLOC_MINALIGN used for alignment if explicitly set __aligned__(8) will break x86_32? -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-10-04 16:19 ` Alexey Brodkin @ 2018-10-04 16:19 ` Alexey Brodkin 0 siblings, 0 replies; 38+ messages in thread From: Alexey Brodkin @ 2018-10-04 16:19 UTC (permalink / raw) To: peterz Cc: linux-kernel, tglx, linux-snps-arc, stable, will.deacon, greg, gregkh, David.Laight, Vineet Gupta, linux-arch, geert Hi Peter, On Mon, 2018-07-09 at 17:53 +0300, Alexey Brodkin wrote: > Hi Peter, > > On Mon, 2018-07-09 at 16:49 +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > > than a sane cache line length so above should work. > > > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > > thing). > > > > So unconditionally setting the alignment of devres::data to 8 seems > > broken. > > Well but then what other options do we have to fix a problem with > misaligned access to atomic64_t in drm_gpu_scheduler? Ping! Should I send v4 with ARCH_KMALLOC_MINALIGN used for alignment if explicitly set __aligned__(8) will break x86_32? -Alexey ^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:49 ` Peter Zijlstra 2018-07-09 14:49 ` Peter Zijlstra 2018-07-09 14:53 ` Alexey Brodkin @ 2018-07-09 15:02 ` David Laight 2018-07-09 15:02 ` David Laight 2018-07-09 15:14 ` Peter Zijlstra 2018-07-09 15:29 ` Mark Rutland 3 siblings, 2 replies; 38+ messages in thread From: David Laight @ 2018-07-09 15:02 UTC (permalink / raw) To: 'Peter Zijlstra', Alexey Brodkin Cc: linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, linux-arch, geert From: Peter Zijlstra > Sent: 09 July 2018 15:49 > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > than a sane cache line length so above should work. > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > thing). That seems broken. I wonder what the minimal alignment really is? I suspect some code expects (and gets) 8-byte alignment. The min alignment might even be 16 or 32 bytes. There aren't many x86 instructions that fault on mis-aligned addresses, but there are a few. Mostly related to the fpu - probably including the fpu save area. David ^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:02 ` David Laight @ 2018-07-09 15:02 ` David Laight 2018-07-09 15:14 ` Peter Zijlstra 1 sibling, 0 replies; 38+ messages in thread From: David Laight @ 2018-07-09 15:02 UTC (permalink / raw) To: 'Peter Zijlstra', Alexey Brodkin Cc: linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, linux-arch, geert From: Peter Zijlstra > Sent: 09 July 2018 15:49 > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > than a sane cache line length so above should work. > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > thing). That seems broken. I wonder what the minimal alignment really is? I suspect some code expects (and gets) 8-byte alignment. The min alignment might even be 16 or 32 bytes. There aren't many x86 instructions that fault on mis-aligned addresses, but there are a few. Mostly related to the fpu - probably including the fpu save area. David ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:02 ` David Laight 2018-07-09 15:02 ` David Laight @ 2018-07-09 15:14 ` Peter Zijlstra 2018-07-09 15:14 ` Peter Zijlstra 1 sibling, 1 reply; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 15:14 UTC (permalink / raw) To: David Laight Cc: linux-arch, greg, Alexey Brodkin, will.deacon, linux-kernel, stable, geert, gregkh, tglx, linux-snps-arc On Mon, Jul 09, 2018 at 03:02:08PM +0000, David Laight wrote: > Mostly related to the fpu - probably including the fpu save area. So for the FPU save area in particular I know we play some horrific games. As to the rest I really dont know. I wouldn't mind it being changed, but from a cursory look, I couldn't see it being anything other than 4 for x86_32. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:14 ` Peter Zijlstra @ 2018-07-09 15:14 ` Peter Zijlstra 0 siblings, 0 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 15:14 UTC (permalink / raw) To: David Laight Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, linux-arch, geert On Mon, Jul 09, 2018 at 03:02:08PM +0000, David Laight wrote: > Mostly related to the fpu - probably including the fpu save area. So for the FPU save area in particular I know we play some horrific games. As to the rest I really dont know. I wouldn't mind it being changed, but from a cursory look, I couldn't see it being anything other than 4 for x86_32. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 14:49 ` Peter Zijlstra ` (2 preceding siblings ...) 2018-07-09 15:02 ` David Laight @ 2018-07-09 15:29 ` Mark Rutland 2018-07-09 15:29 ` Mark Rutland 2018-07-09 15:34 ` Peter Zijlstra 3 siblings, 2 replies; 38+ messages in thread From: Mark Rutland @ 2018-07-09 15:29 UTC (permalink / raw) To: Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 04:49:25PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > than a sane cache line length so above should work. > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > thing). Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on x86_32: ---- [mark@lakrids:~]% cat test.c #include <stdio.h> #define PRINT_TYPE_INFO(t) \ printf("%10s %5d %5d\n", #t, sizeof(t), __alignof__(t)) int main(int argc, char *argv[]) { printf("%10s %5s %5s\n", "TYPE", "SIZE", "ALIGN"); PRINT_TYPE_INFO(int); PRINT_TYPE_INFO(long); PRINT_TYPE_INFO(long long); return 0; } [mark@lakrids:~]% gcc -m32 test.c -o test [mark@lakrids:~]% ./test TYPE SIZE ALIGN int 4 4 long 4 4 long long 8 8 ---- Mark. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:29 ` Mark Rutland @ 2018-07-09 15:29 ` Mark Rutland 2018-07-09 15:34 ` Peter Zijlstra 1 sibling, 0 replies; 38+ messages in thread From: Mark Rutland @ 2018-07-09 15:29 UTC (permalink / raw) To: Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 04:49:25PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote: > > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing, > > > it is impossible to guarantee a larger alignment than kmalloc does. > > > > Well but 4-bytes [which is critical for atomic64_t] should be much less > > than a sane cache line length so above should work. > > AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't > define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the > thing). Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on x86_32: ---- [mark@lakrids:~]% cat test.c #include <stdio.h> #define PRINT_TYPE_INFO(t) \ printf("%10s %5d %5d\n", #t, sizeof(t), __alignof__(t)) int main(int argc, char *argv[]) { printf("%10s %5s %5s\n", "TYPE", "SIZE", "ALIGN"); PRINT_TYPE_INFO(int); PRINT_TYPE_INFO(long); PRINT_TYPE_INFO(long long); return 0; } [mark@lakrids:~]% gcc -m32 test.c -o test [mark@lakrids:~]% ./test TYPE SIZE ALIGN int 4 4 long 4 4 long long 8 8 ---- Mark. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:29 ` Mark Rutland 2018-07-09 15:29 ` Mark Rutland @ 2018-07-09 15:34 ` Peter Zijlstra 2018-07-09 15:34 ` Peter Zijlstra 2018-07-09 15:45 ` Peter Zijlstra 1 sibling, 2 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 15:34 UTC (permalink / raw) To: Mark Rutland Cc: linux-arch, greg, Alexey Brodkin, will.deacon, linux-kernel, stable, David.Laight, geert, gregkh, tglx, linux-snps-arc On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > x86_32: Curious, I wonder why we put that align in atomic64_32 then. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:34 ` Peter Zijlstra @ 2018-07-09 15:34 ` Peter Zijlstra 2018-07-09 15:45 ` Peter Zijlstra 1 sibling, 0 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 15:34 UTC (permalink / raw) To: Mark Rutland Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > x86_32: Curious, I wonder why we put that align in atomic64_32 then. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:34 ` Peter Zijlstra 2018-07-09 15:34 ` Peter Zijlstra @ 2018-07-09 15:45 ` Peter Zijlstra 2018-07-09 15:45 ` Peter Zijlstra 2018-07-09 15:48 ` Mark Rutland 1 sibling, 2 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 15:45 UTC (permalink / raw) To: Mark Rutland Cc: linux-arch, greg, Alexey Brodkin, will.deacon, linux-kernel, stable, David.Laight, geert, gregkh, tglx, linux-snps-arc On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > > x86_32: > > Curious, I wonder why we put that align in atomic64_32 then. Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:45 ` Peter Zijlstra @ 2018-07-09 15:45 ` Peter Zijlstra 2018-07-09 15:48 ` Mark Rutland 1 sibling, 0 replies; 38+ messages in thread From: Peter Zijlstra @ 2018-07-09 15:45 UTC (permalink / raw) To: Mark Rutland Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > > x86_32: > > Curious, I wonder why we put that align in atomic64_32 then. Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:45 ` Peter Zijlstra 2018-07-09 15:45 ` Peter Zijlstra @ 2018-07-09 15:48 ` Mark Rutland 2018-07-09 15:48 ` Mark Rutland 2018-07-09 16:10 ` David Laight 1 sibling, 2 replies; 38+ messages in thread From: Mark Rutland @ 2018-07-09 15:48 UTC (permalink / raw) To: Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > > > x86_32: > > > > Curious, I wonder why we put that align in atomic64_32 then. > > Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 > Ouch. [mark@lakrids:~]% cat test.c #include <stdio.h> #define PRINT_TYPE_INFO(t) \ printf("%10s %5d %5d\n", #t, sizeof(t), __alignof__(t)) struct ull { unsigned long long v; }; int main(int argc, char *argv[]) { printf("%10s %5s %5s\n", "TYPE", "SIZE", "ALIGN"); PRINT_TYPE_INFO(int); PRINT_TYPE_INFO(long); PRINT_TYPE_INFO(long long); PRINT_TYPE_INFO(struct ull); return 0; } [mark@lakrids:~]% gcc -m32 test.c -o test [mark@lakrids:~]% ./test TYPE SIZE ALIGN int 4 4 long 4 4 long long 8 8 struct ull 8 4 Mark. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:48 ` Mark Rutland @ 2018-07-09 15:48 ` Mark Rutland 2018-07-09 16:10 ` David Laight 1 sibling, 0 replies; 38+ messages in thread From: Mark Rutland @ 2018-07-09 15:48 UTC (permalink / raw) To: Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, David.Laight, linux-arch, geert On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > > > x86_32: > > > > Curious, I wonder why we put that align in atomic64_32 then. > > Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 > Ouch. [mark@lakrids:~]% cat test.c #include <stdio.h> #define PRINT_TYPE_INFO(t) \ printf("%10s %5d %5d\n", #t, sizeof(t), __alignof__(t)) struct ull { unsigned long long v; }; int main(int argc, char *argv[]) { printf("%10s %5s %5s\n", "TYPE", "SIZE", "ALIGN"); PRINT_TYPE_INFO(int); PRINT_TYPE_INFO(long); PRINT_TYPE_INFO(long long); PRINT_TYPE_INFO(struct ull); return 0; } [mark@lakrids:~]% gcc -m32 test.c -o test [mark@lakrids:~]% ./test TYPE SIZE ALIGN int 4 4 long 4 4 long long 8 8 struct ull 8 4 Mark. ^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 15:48 ` Mark Rutland 2018-07-09 15:48 ` Mark Rutland @ 2018-07-09 16:10 ` David Laight 2018-07-09 16:10 ` David Laight 2018-07-09 20:28 ` Vineet Gupta 1 sibling, 2 replies; 38+ messages in thread From: David Laight @ 2018-07-09 16:10 UTC (permalink / raw) To: 'Mark Rutland', Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, linux-arch, geert From: Mark Rutland > Sent: 09 July 2018 16:49 > > On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: > > > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > > > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > > > > x86_32: > > > > > > Curious, I wonder why we put that align in atomic64_32 then. > > > > Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 > > > > Ouch. Indeed. changing the definition to: struct ull { unsigned long long v __attribute__((aligned(__alignof__(long long)))); }; prints 8 for the structure alignment. Time to audit uses of __alignof__(). #define actual_alignof(type) __alignof__(struct { type jsdjdhjdjh; }) David ^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 16:10 ` David Laight @ 2018-07-09 16:10 ` David Laight 2018-07-09 20:28 ` Vineet Gupta 1 sibling, 0 replies; 38+ messages in thread From: David Laight @ 2018-07-09 16:10 UTC (permalink / raw) To: 'Mark Rutland', Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, linux-arch, geert From: Mark Rutland > Sent: 09 July 2018 16:49 > > On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote: > > On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: > > > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: > > > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on > > > > x86_32: > > > > > > Curious, I wonder why we put that align in atomic64_32 then. > > > > Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 > > > > Ouch. Indeed. changing the definition to: struct ull { unsigned long long v __attribute__((aligned(__alignof__(long long)))); }; prints 8 for the structure alignment. Time to audit uses of __alignof__(). #define actual_alignof(type) __alignof__(struct { type jsdjdhjdjh; }) David ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 16:10 ` David Laight 2018-07-09 16:10 ` David Laight @ 2018-07-09 20:28 ` Vineet Gupta 2018-07-09 20:28 ` Vineet Gupta 1 sibling, 1 reply; 38+ messages in thread From: Vineet Gupta @ 2018-07-09 20:28 UTC (permalink / raw) To: David Laight, 'Mark Rutland', Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, linux-arch, geert On 07/09/2018 09:10 AM, David Laight wrote: > From: Mark Rutland >> Sent: 09 July 2018 16:49 >> >> On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote: >>> On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: >>>> On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: >>>>> Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on >>>>> x86_32: >>>> >>>> Curious, I wonder why we put that align in atomic64_32 then. >>> >>> Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 >>> >> >> Ouch. > > Indeed. > > changing the definition to: > struct ull { > unsigned long long v __attribute__((aligned(__alignof__(long long)))); > }; > > prints 8 for the structure alignment. So this will help force alignment of outer structures triggered by inner members, but only for globals and perhaps those on stack. I'm not sure how this helps aligning such a struct allocated dynamically - we are not passing this extra alignment info the the allocator API here. Surely the size will increase and we end up with extra padding in the end as intended originally and pointed to by Mark, but how does it help with alignment ? What am I missing ? > > Time to audit uses of __alignof__(). > > #define actual_alignof(type) __alignof__(struct { type jsdjdhjdjh; }) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit 2018-07-09 20:28 ` Vineet Gupta @ 2018-07-09 20:28 ` Vineet Gupta 0 siblings, 0 replies; 38+ messages in thread From: Vineet Gupta @ 2018-07-09 20:28 UTC (permalink / raw) To: David Laight, 'Mark Rutland', Peter Zijlstra Cc: Alexey Brodkin, linux-kernel, tglx, linux-snps-arc, stable, greg, will.deacon, gregkh, linux-arch, geert On 07/09/2018 09:10 AM, David Laight wrote: > From: Mark Rutland >> Sent: 09 July 2018 16:49 >> >> On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote: >>> On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote: >>>> On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote: >>>>> Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on >>>>> x86_32: >>>> >>>> Curious, I wonder why we put that align in atomic64_32 then. >>> >>> Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 >>> >> >> Ouch. > > Indeed. > > changing the definition to: > struct ull { > unsigned long long v __attribute__((aligned(__alignof__(long long)))); > }; > > prints 8 for the structure alignment. So this will help force alignment of outer structures triggered by inner members, but only for globals and perhaps those on stack. I'm not sure how this helps aligning such a struct allocated dynamically - we are not passing this extra alignment info the the allocator API here. Surely the size will increase and we end up with extra padding in the end as intended originally and pointed to by Mark, but how does it help with alignment ? What am I missing ? > > Time to audit uses of __alignof__(). > > #define actual_alignof(type) __alignof__(struct { type jsdjdhjdjh; }) ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2018-10-04 23:13 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-09 13:45 [PATCH v3] devres: Explicitly align datai[] to 64-bit Alexey Brodkin 2018-07-09 13:45 ` Alexey Brodkin 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 13:54 ` Peter Zijlstra 2018-07-09 14:04 ` Mark Rutland 2018-07-09 14:04 ` Mark Rutland 2018-07-09 14:10 ` Geert Uytterhoeven 2018-07-09 14:10 ` Geert Uytterhoeven 2018-07-09 14:07 ` Alexey Brodkin 2018-07-09 14:07 ` Alexey Brodkin 2018-07-09 14:07 ` Peter Zijlstra 2018-07-09 14:07 ` Peter Zijlstra 2018-07-09 14:10 ` Peter Zijlstra 2018-07-09 14:10 ` Peter Zijlstra 2018-07-09 14:33 ` Alexey Brodkin 2018-07-09 14:33 ` Alexey Brodkin 2018-07-09 14:49 ` Peter Zijlstra 2018-07-09 14:49 ` Peter Zijlstra 2018-07-09 14:53 ` Alexey Brodkin 2018-07-09 14:53 ` Alexey Brodkin 2018-10-04 16:19 ` Alexey Brodkin 2018-10-04 16:19 ` Alexey Brodkin 2018-07-09 15:02 ` David Laight 2018-07-09 15:02 ` David Laight 2018-07-09 15:14 ` Peter Zijlstra 2018-07-09 15:14 ` Peter Zijlstra 2018-07-09 15:29 ` Mark Rutland 2018-07-09 15:29 ` Mark Rutland 2018-07-09 15:34 ` Peter Zijlstra 2018-07-09 15:34 ` Peter Zijlstra 2018-07-09 15:45 ` Peter Zijlstra 2018-07-09 15:45 ` Peter Zijlstra 2018-07-09 15:48 ` Mark Rutland 2018-07-09 15:48 ` Mark Rutland 2018-07-09 16:10 ` David Laight 2018-07-09 16:10 ` David Laight 2018-07-09 20:28 ` Vineet Gupta 2018-07-09 20:28 ` Vineet Gupta
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).