| Message ID | 20180709134550.29541-1-abrodkin@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] devres: Explicitly align datai[] to 64-bit | expand |
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?
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.
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.
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
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
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.
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
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.
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
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
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.
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.
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.
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
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.
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
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; })
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
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 {
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(-)