Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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; })
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
On 07/09/2018 03:23 AM, Alexey Brodkin wrote:
> Hi David,
>
> On Mon, 2018-07-09 at 10:18 +, David Laight wrote:
>> From: Alexey Brodkin
>>> Sent: 09 July 2018 11:00
>>
>> ...
>>> That's a good idea indeed but it doesn't solve the problem with
>>> struct devres_node. Consider the following snippet:
>>> >8---
>>> struct mystruct {
>>> atomic64_t myvar;
>>> }
>>>
>>> struct mystruct *p;
>>> p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
>>> >8---
>>>
>>> Here myvar address will match address of "data" member of struct
>>> devres_node.
>>> So if "data" is has offset of 12 bytes from the beginning of a page then
>>> myvar won't be 64-bit aligned regardless of myvar's attribute, right?
>>
>> ...
> - unsigned long long data[]; /* guarantee ull alignment */
>>
>> Ahh, that line should be:
>> u8 data[] __aligned(8); /* Guarantee 64bit alignment */
>
> And that pretty much what I suggested in my initial patch :)
>
> For the record x86 has exactly the same atomic64_t as you suggested,
> see
> https://elixir.bootlin.com/linux/latest/source/arch/x86/include/asm/atomic64_32.h#L13:
> -->8--
> typedef struct {
> u64 __aligned(8) counter;
> } atomic64_t;
> -->8--
And so does the ARC version since when the atomic64_t support was added by
commit
ce6365270ecd
Also, you should consider using the pre-canned type aligned_u64.
typedef struct {
aligned_u64 counter;
^^^
} atomic64_t;
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
Hi Alexey,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc4 next-20180709]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Alexey-Brodkin/atomic-64-_t-Explicitly-specify-data-storage-length-and-alignment/20180709-223920
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=s390
All error/warnings (new ones prefixed by >>):
In file included from include/linux/atomic.h:5:0,
from include/linux/debug_locks.h:6,
from include/linux/lockdep.h:28,
from include/linux/spinlock_types.h:18,
from kernel/bounds.c:14:
arch/s390/include/asm/atomic.h: In function 'atomic64_add_return':
>> arch/s390/include/asm/atomic.h:129:35: error: passing argument 2 of
>> '__atomic64_add_barrier' from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
return __atomic64_add_barrier(i, >counter) + i;
^
In file included from arch/s390/include/asm/bitops.h:38:0,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from arch/s390/include/asm/bug.h:5,
from include/linux/bug.h:5,
from include/linux/page-flags.h:10,
from kernel/bounds.c:10:
arch/s390/include/asm/atomic_ops.h:35:14: note: expected 'long int *' but
argument is of type 'u64 * {aka long long unsigned int *}'
__ATOMIC_OPS(__atomic64_add, long, "laag")
^
arch/s390/include/asm/atomic_ops.h:14:23: note: in definition of macro
'__ATOMIC_OP'
static inline op_type op_name(op_type val, op_type *ptr) \
^~~
>> arch/s390/include/asm/atomic_ops.h:35:1: note: in expansion of macro
>> '__ATOMIC_OPS'
__ATOMIC_OPS(__atomic64_add, long, "laag")
^~~~
In file included from include/linux/atomic.h:5:0,
from include/linux/debug_locks.h:6,
from include/linux/lockdep.h:28,
from include/linux/spinlock_types.h:18,
from kernel/bounds.c:14:
arch/s390/include/asm/atomic.h: In function 'atomic64_fetch_add':
arch/s390/include/asm/atomic.h:134:35: error: passing argument 2 of
'__atomic64_add_barrier' from incompatible pointer type
[-Werror=incompatible-pointer-types]
return __atomic64_add_barrier(i, >counter);
^
In file included from arch/s390/include/asm/bitops.h:38:0,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from arch/s390/include/asm/bug.h:5,
from include/linux/bug.h:5,
from include/linux/page-flags.h:10,
from kernel/bounds.c:10:
arch/s390/include/asm/atomic_ops.h:35:14: note: expected 'long int *' but
argument is of type 'u64 * {aka long long unsigned int *}'
__ATOMIC_OPS(__atomic64_add, long, "laag")
^
arch/s390/include/asm/atomic_ops.h:14:23: note: in definition of macro
'__ATOMIC_OP'
static inline op_type op_name(op_type val, op_type *ptr) \
^~~
>> arch/s390/include/asm/atomic_ops.h:35:1: note: in expansion of macro
>> '__ATOMIC_OPS'
__ATOMIC_OPS(__atomic64_add, long, "laag")
^~~~
In file included from include/linux/atomic.h:5:0,
from include/linux/debug_locks.h:6,
from include/linux/lockdep.h:28,
from include/linux/spinlock_types.h:18,
from kernel/bounds.c:14:
arch/s390/include/asm/atomic.h: In function 'atomic64_add':
>> arch/s390/include/asm/atomic.h:141:27: error: passing argument 2 of
>> '__atomic64_add_const' from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
__atomic64_add_const(i, >counter);
^
In file included from arch/s390/include/asm/bitops.h:38:0,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from arch/s390/include/asm/bug.h:5,
from include/linux/bug.h:5,
from include/linux/page-flags.h:10,
from kernel/bounds.c:10:
arch/s390/include/asm/atomic_ops.h:57:20: note: expected 'long int *' but
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
Hi Alexey, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v4.18-rc4 next-20180709] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Alexey-Brodkin/atomic-64-_t-Explicitly-specify-data-storage-length-and-alignment/20180709-223920 config: x86_64-acpi-redef (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): In file included from tools/objtool/arch/x86/include/asm/orc_types.h:21:0, from orc.h:21, from orc_dump.c:19: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ In file included from tools/objtool/arch/x86/include/asm/orc_types.h:21:0, from orc.h:21, from orc_gen.c:21: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ In file included from tools/include/linux/string.h:5:0, from ../lib/string.c:19: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ cc1: all warnings being treated as errors >> mv: cannot stat 'tools/objtool/.libstring.o.tmp': No such file or directory make[4]: *** [tools/objtool/libstring.o] Error 1 In file included from tools/include/linux/string.h:5:0, from ../lib/str_error_r.c:5: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ cc1: all warnings being treated as errors In file included from tools/include/linux/list.h:5:0, from elf.h:23, from elf.c:31: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ In file included from tools/include/linux/list.h:5:0, from elf.h:23, from special.h:22, from special.c:26: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ mv: cannot stat 'tools/objtool/.str_error_r.o.tmp': No such file or directory make[4]: *** [tools/objtool/str_error_r.o] Error 1 In file included from tools/include/linux/string.h:5:0, from run-command.c:7: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ cc1: all warnings being treated as errors mv: cannot stat 'tools/objtool/.orc_dump.o.tmp': No such file or directory make[4]: *** [tools/objtool/orc_dump.o] Error 1 In file included from tools/include/linux/string.h:5:0, from help.c:5: >> tools/include/linux/types.h:62:16: error: expected declaration specifiers or >> '...' before numeric constant u32 __aligned(4) counter; ^ tools/include/linux/types.h:63:1: error: no semicolon at end of struct or union [-Werror] } atomic_t; ^ cc1: all warnings being treated as errors mv: cannot stat 'tools/objtool/.special.o.tmp': No such file or directory make[4]: *** [tools/objtool/special.o] Error 1 cc1: all warnings being treated as errors cc1: all warnings being treated as errors mv: cannot stat 'tools/objtool/.run-command.o.tmp': No such file or directory make[5]: *** [tools/objtool/run-command.o] Error 1 mv: cannot stat 'tools/objtool/.orc_gen.o.tmp': No such file or directory
RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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
#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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
On Mon, Jul 09, 2018 at 04:49:25PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 02:33:26PM +, 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
#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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
On Mon, Jul 09, 2018 at 02:30:41PM +, Alexey Brodkin wrote: > Hm, any thoughts on why it's "u64" for 32-bit x86? Accident probably. It probably doesn't really matter all that much because the kernel hard assumes 2s complement, but it is somewhat inconsistent. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
On Mon, Jul 09, 2018 at 02:33:26PM +, 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. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit
From: Peter Zijlstra > Sent: 09 July 2018 15:49 > On Mon, Jul 09, 2018 at 02:33:26PM +, 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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
Hi Peter, On Mon, 2018-07-09 at 16:49 +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 02:33:26PM +, 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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
Hi Peter,
On Mon, 2018-07-09 at 15:35 +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 03:47:41PM +0300, Alexey Brodkin wrote:
> > diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h
> > index 8d28eb010d0d..b94b749b5952 100644
> > --- a/include/asm-generic/atomic64.h
> > +++ b/include/asm-generic/atomic64.h
> > @@ -13,7 +13,7 @@
> > #define _ASM_GENERIC_ATOMIC64_H
> >
> > typedef struct {
> > - long long counter;
> > + u64 __aligned(8) counter;
> > } atomic64_t;
>
> The type is wrong, atomic is signed, the alignment also really doesn't
> matter, generic atomic64 is utter crap.
Hm, any thoughts on why it's "u64" for 32-bit x86?
See
https://elixir.bootlin.com/linux/latest/source/arch/x86/include/asm/atomic64_32.h#L12
->8---
/* An 64bit atomic type */
typedef struct {
u64 __aligned(8) counter;
} atomic64_t;
->8---
> > #define ATOMIC64_INIT(i) { (i) }
> > diff --git a/include/linux/types.h b/include/linux/types.h
> > index 9834e90aa010..e2f631782621 100644
> > --- a/include/linux/types.h
> > +++ b/include/linux/types.h
> > @@ -174,12 +174,12 @@ typedef phys_addr_t resource_size_t;
> > typedef unsigned long irq_hw_number_t;
> >
> > typedef struct {
> > - int counter;
> > + u32 __aligned(4) counter;
> > } atomic_t;
>
> u32 is wrong, the atomic type is signed.
>
> Also, if an architecture doesn't properly align its native machine word
> size but requires alignment for atomics it's a broken architecture.
Ok we may not touch 32-bit atomics as there's a hope most of arches
properly align native machine words.
> >
> > #ifdef CONFIG_64BIT
> > typedef struct {
> > - long counter;
> > + u64 __aligned(8) counter;
> > } atomic64_t;
> > #endif
> >
>
> Similar for this one, on 64bit archs that support atomics the native
> 64bit types (long included) had better already imply this alignment.
Ok agree.
-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
On Mon, Jul 9, 2018 at 4:04 PM Mark Rutland 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 -- ge...@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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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_schedulersched;
};
struct etnaviv_gpu *gpu;
gpu = devm_kzalloc(dev, sizeof(*gpu), GFP_KERNEL);
-->8-
-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
On Mon, Jul 09, 2018 at 03:47:41PM +0300, Alexey Brodkin wrote:
> Atomic instructions require data they operate on to be aligned
> according to data size. I.e. 32-bit atomic values must be 32-bit
> aligned while 64-bit values must be 64-bit aligned.
>
> Otherwise even if CPU may handle not-aligend normal data access,
> still atomic instructions fail and typically raise an exception
> leaving us dead in the water.
>
> This came-up during lengthly discussion here:
> http://lists.infradead.org/pipermail/linux-snps-arc/2018-July/004022.html
>
> Signed-off-by: Alexey Brodkin
> Cc: Will Deacon
> Cc: Peter Zijlstra
> Cc: Boqun Feng
> Cc: Russell King
> Cc: Arnd Bergmann
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Darren Hart
> Cc: Shuah Khan
> Cc: "Paul E. McKenney"
> Cc: Josh Triplett
> Cc: Steven Rostedt
> Cc: Mathieu Desnoyers
> Cc: Lai Jiangshan
> Cc: David Laight
> Cc: Geert Uytterhoeven
> Cc: Greg Kroah-Hartman
> ---
> arch/arm/include/asm/atomic.h | 2 +-
> include/asm-generic/atomic64.h| 2 +-
> include/linux/types.h | 4 ++--
> tools/include/linux/types.h | 2 +-
> tools/testing/selftests/futex/include/atomic.h| 2 +-
> .../rcutorture/formal/srcu-cbmc/include/linux/types.h | 4 ++--
> 6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index 66d0e215a773..2ed6d7cf1407 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -267,7 +267,7 @@ ATOMIC_OPS(xor, ^=, eor)
>
> #ifndef CONFIG_GENERIC_ATOMIC64
> typedef struct {
> - long long counter;
> + u64 __aligned(8) counter;
> } atomic64_t;
ARM doesn't need or require this change, and you're changing the type
from signed to unsigned, which is likely to break stuff. So, NAK on
this from the ARM point of view.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit
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?
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
On Mon, Jul 9, 2018 at 3:29 PM David Laight wrote:
> From: Alexey Brodkin
> > Sent: 09 July 2018 13:48
> > Atomic instructions require data they operate on to be aligned
> > according to data size. I.e. 32-bit atomic values must be 32-bit
> > aligned while 64-bit values must be 64-bit aligned.
> >
> > Otherwise even if CPU may handle not-aligend normal data access,
> > still atomic instructions fail and typically raise an exception
> > leaving us dead in the water.
> ...
> > diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h
> > index 8d28eb010d0d..b94b749b5952 100644
> > --- a/include/asm-generic/atomic64.h
> > +++ b/include/asm-generic/atomic64.h
> > @@ -13,7 +13,7 @@
> > #define _ASM_GENERIC_ATOMIC64_H
> >
> > typedef struct {
> > - long long counter;
> > + u64 __aligned(8) counter;
> > } atomic64_t;
>
> Apart from the fact that this changes the value from signed to unsigned
> should most of the architectures be using this generic definition?
64-bit architectures use the one from include/linux/types.h instead.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3] devres: Explicitly align datai[] to 64-bit
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
Cc: Greg Kroah-Hartman
Cc: Geert Uytterhoeven
Cc: David Laight
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Cc: Will Deacon
Cc: Greg KH
Cc: # 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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
On Mon, Jul 09, 2018 at 03:47:41PM +0300, Alexey Brodkin wrote:
> diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h
> index 8d28eb010d0d..b94b749b5952 100644
> --- a/include/asm-generic/atomic64.h
> +++ b/include/asm-generic/atomic64.h
> @@ -13,7 +13,7 @@
> #define _ASM_GENERIC_ATOMIC64_H
>
> typedef struct {
> - long long counter;
> + u64 __aligned(8) counter;
> } atomic64_t;
The type is wrong, atomic is signed, the alignment also really doesn't
matter, generic atomic64 is utter crap.
> #define ATOMIC64_INIT(i) { (i) }
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 9834e90aa010..e2f631782621 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -174,12 +174,12 @@ typedef phys_addr_t resource_size_t;
> typedef unsigned long irq_hw_number_t;
>
> typedef struct {
> - int counter;
> + u32 __aligned(4) counter;
> } atomic_t;
u32 is wrong, the atomic type is signed.
Also, if an architecture doesn't properly align its native machine word
size but requires alignment for atomics it's a broken architecture.
>
> #ifdef CONFIG_64BIT
> typedef struct {
> - long counter;
> + u64 __aligned(8) counter;
> } atomic64_t;
> #endif
>
Similar for this one, on 64bit archs that support atomics the native
64bit types (long included) had better already imply this alignment.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
On Mon, Jul 09, 2018 at 03:47:41PM +0300, Alexey Brodkin wrote:
> Atomic instructions require data they operate on to be aligned
> according to data size. I.e. 32-bit atomic values must be 32-bit
> aligned while 64-bit values must be 64-bit aligned.
>
> Otherwise even if CPU may handle not-aligend normal data access,
> still atomic instructions fail and typically raise an exception
> leaving us dead in the water.
>
> This came-up during lengthly discussion here:
> http://lists.infradead.org/pipermail/linux-snps-arc/2018-July/004022.html
>
> Signed-off-by: Alexey Brodkin
> Cc: Will Deacon
> Cc: Peter Zijlstra
> Cc: Boqun Feng
> Cc: Russell King
> Cc: Arnd Bergmann
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Darren Hart
> Cc: Shuah Khan
> Cc: "Paul E. McKenney"
> Cc: Josh Triplett
> Cc: Steven Rostedt
> Cc: Mathieu Desnoyers
> Cc: Lai Jiangshan
> Cc: David Laight
> Cc: Geert Uytterhoeven
> Cc: Greg Kroah-Hartman
> ---
> arch/arm/include/asm/atomic.h | 2 +-
> include/asm-generic/atomic64.h| 2 +-
> include/linux/types.h | 4 ++--
> tools/include/linux/types.h | 2 +-
> tools/testing/selftests/futex/include/atomic.h| 2 +-
> .../rcutorture/formal/srcu-cbmc/include/linux/types.h | 4 ++--
> 6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
> index 66d0e215a773..2ed6d7cf1407 100644
> --- a/arch/arm/include/asm/atomic.h
> +++ b/arch/arm/include/asm/atomic.h
> @@ -267,7 +267,7 @@ ATOMIC_OPS(xor, ^=, eor)
>
> #ifndef CONFIG_GENERIC_ATOMIC64
> typedef struct {
> - long long counter;
> + u64 __aligned(8) counter;
> } atomic64_t;
Long long is 8-byte aligned per EABI ARM, and we use the generic atomic64
infrastructure for OABI, so we don't need to change anything here afaict.
Will
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH] atomic{64}_t: Explicitly specify data storage length and alignment
Atomic instructions require data they operate on to be aligned
according to data size. I.e. 32-bit atomic values must be 32-bit
aligned while 64-bit values must be 64-bit aligned.
Otherwise even if CPU may handle not-aligend normal data access,
still atomic instructions fail and typically raise an exception
leaving us dead in the water.
This came-up during lengthly discussion here:
http://lists.infradead.org/pipermail/linux-snps-arc/2018-July/004022.html
Signed-off-by: Alexey Brodkin
Cc: Will Deacon
Cc: Peter Zijlstra
Cc: Boqun Feng
Cc: Russell King
Cc: Arnd Bergmann
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Darren Hart
Cc: Shuah Khan
Cc: "Paul E. McKenney"
Cc: Josh Triplett
Cc: Steven Rostedt
Cc: Mathieu Desnoyers
Cc: Lai Jiangshan
Cc: David Laight
Cc: Geert Uytterhoeven
Cc: Greg Kroah-Hartman
---
arch/arm/include/asm/atomic.h | 2 +-
include/asm-generic/atomic64.h| 2 +-
include/linux/types.h | 4 ++--
tools/include/linux/types.h | 2 +-
tools/testing/selftests/futex/include/atomic.h| 2 +-
.../rcutorture/formal/srcu-cbmc/include/linux/types.h | 4 ++--
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index 66d0e215a773..2ed6d7cf1407 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -267,7 +267,7 @@ ATOMIC_OPS(xor, ^=, eor)
#ifndef CONFIG_GENERIC_ATOMIC64
typedef struct {
- long long counter;
+ u64 __aligned(8) counter;
} atomic64_t;
#define ATOMIC64_INIT(i) { (i) }
diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h
index 8d28eb010d0d..b94b749b5952 100644
--- a/include/asm-generic/atomic64.h
+++ b/include/asm-generic/atomic64.h
@@ -13,7 +13,7 @@
#define _ASM_GENERIC_ATOMIC64_H
typedef struct {
- long long counter;
+ u64 __aligned(8) counter;
} atomic64_t;
#define ATOMIC64_INIT(i) { (i) }
diff --git a/include/linux/types.h b/include/linux/types.h
index 9834e90aa010..e2f631782621 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -174,12 +174,12 @@ typedef phys_addr_t resource_size_t;
typedef unsigned long irq_hw_number_t;
typedef struct {
- int counter;
+ u32 __aligned(4) counter;
} atomic_t;
#ifdef CONFIG_64BIT
typedef struct {
- long counter;
+ u64 __aligned(8) counter;
} atomic64_t;
#endif
diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h
index 154eb4e3ca7c..c913e26ea4eb 100644
--- a/tools/include/linux/types.h
+++ b/tools/include/linux/types.h
@@ -59,7 +59,7 @@ typedef __u64 __bitwise __le64;
typedef __u64 __bitwise __be64;
typedef struct {
- int counter;
+ u32 __aligned(4) counter;
} atomic_t;
#ifndef __aligned_u64
diff --git a/tools/testing/selftests/futex/include/atomic.h
b/tools/testing/selftests/futex/include/atomic.h
index f861da3e31ab..34e14295e492 100644
--- a/tools/testing/selftests/futex/include/atomic.h
+++ b/tools/testing/selftests/futex/include/atomic.h
@@ -23,7 +23,7 @@
#define _ATOMIC_H
typedef struct {
- volatile int val;
+ volatile u32 __aligned(4) val;
} atomic_t;
#define ATOMIC_INITIALIZER { 0 }
diff --git
a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/include/linux/types.h
b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/include/linux/types.h
index 891ad13e95b2..32ce965187b3 100644
--- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/include/linux/types.h
+++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/include/linux/types.h
@@ -100,12 +100,12 @@ typedef phys_addr_t resource_size_t;
typedef unsigned long irq_hw_number_t;
typedef struct {
- int counter;
+ u32 __aligned(4) counter;
} atomic_t;
#ifdef CONFIG_64BIT
typedef struct {
- long counter;
+ u64 __aligned(8) counter;
} atomic64_t;
#endif
--
2.17.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [RESEND PATCH v2] devres: Really align data field to unsigned long long
From: Alexey Brodkin
> Sent: 09 July 2018 11:00
...
> That's a good idea indeed but it doesn't solve the problem with
> struct devres_node. Consider the following snippet:
> >8---
> struct mystruct {
> atomic64_t myvar;
> }
>
> struct mystruct *p;
> p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
> >8---
>
> Here myvar address will match address of "data" member of struct devres_node.
> So if "data" is has offset of 12 bytes from the beginning of a page then
> myvar won't be 64-bit aligned regardless of myvar's attribute, right?
...
> > > - unsigned long long data[]; /* guarantee ull alignment */
Ahh, that line should be:
u8 data[] __aligned(8); /* Guarantee 64bit alignment */
David
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
Hi David,
On Mon, 2018-07-09 at 09:16 +, David Laight wrote:
> From: Alexey Brodkin
> > Sent: 09 July 2018 05:45
> > Depending on ABI "long long" type of a particular 32-bit CPU
> > might be aligned by either word (32-bits) or double word (64-bits).
> > Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> >
> > At least for 32-bit ARC cores ABI requires "long long" types
> > to be aligned by normal 32-bit word. This makes "data" field aligned to
> > 12 bytes. Which is still OK as long as we use 32-bit data only.
> >
> > But once we want to use native atomic64_t type (i.e. when we use special
> > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> > misaligned access exception.
>
> Shouldn't there be a typedef for the actual type.
> Perhaps it is even atomic64_t ?
> And have the __aligned(8) applied to that typedef ??
That's a good idea indeed but it doesn't solve the problem with
struct devres_node. Consider the following snippet:
>8---
struct mystruct {
atomic64_t myvar;
}
struct mystruct *p;
p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
>8---
Here myvar address will match address of "data" member of struct devres_node.
So if "data" is has offset of 12 bytes from the beginning of a page then
myvar won't be 64-bit aligned regardless of myvar's attribute, right?
> > That's because even on CPUs capable of non-aligned data access LL/SC
> > instructions require strict alignment.
>
> ...
> > --- 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 */
> > + /*
> > +* Depending on ABI "long long" type of a particular 32-bit CPU
> > +* might be aligned by either word (32-bits) or double word (64-bits).
> > +* Make sure "data" is really 64-bit aligned for any 32-bit CPU.
>
> Just:
> /* data[] must be 64 bit aligned even on 32 bit architectures
>* because it might be accessed by instructions that require
>* aligned memory arguments.
>
> > +*/
> > + unsigned long long data[] __aligned(sizeof(unsigned long
> > long));
>
> One day assuming that 'unsigned long long' is exactly 64 bits will bite.
> This probably ought to be u64 (or similar).
I agree. Initially I wanted to keep as few changes as possible but
IMHO switching to more predictable data type makes sense.
-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [RESEND PATCH v2] devres: Really align data field to unsigned long long
From: Geert Uytterhoeven
> Sent: 09 July 2018 10:23
> On Mon, Jul 9, 2018 at 11:15 AM David Laight wrote:
> > From: Alexey Brodkin
> > > Sent: 09 July 2018 05:45
> > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > might be aligned by either word (32-bits) or double word (64-bits).
> > > Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> > >
> > > At least for 32-bit ARC cores ABI requires "long long" types
> > > to be aligned by normal 32-bit word. This makes "data" field aligned to
> > > 12 bytes. Which is still OK as long as we use 32-bit data only.
> > >
> > > But once we want to use native atomic64_t type (i.e. when we use special
> > > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> > > misaligned access exception.
> >
> > Shouldn't there be a typedef for the actual type.
> > Perhaps it is even atomic64_t ?
> > And have the __aligned(8) applied to that typedef ??
>
> That indeed sounds like the best thing to do, as it will fix this issue in
> other
> places, too.
Something like:
typedef struct {
u64 val __aligned(8);
} atomic64_t;
would pick up most errors.
Including all the places that fail to use atomic_read().
David
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
On Mon, Jul 9, 2018 at 11:15 AM David Laight wrote:
> From: Alexey Brodkin
> > Sent: 09 July 2018 05:45
> > Depending on ABI "long long" type of a particular 32-bit CPU
> > might be aligned by either word (32-bits) or double word (64-bits).
> > Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> >
> > At least for 32-bit ARC cores ABI requires "long long" types
> > to be aligned by normal 32-bit word. This makes "data" field aligned to
> > 12 bytes. Which is still OK as long as we use 32-bit data only.
> >
> > But once we want to use native atomic64_t type (i.e. when we use special
> > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> > misaligned access exception.
>
> Shouldn't there be a typedef for the actual type.
> Perhaps it is even atomic64_t ?
> And have the __aligned(8) applied to that typedef ??
That indeed sounds like the best thing to do, as it will fix this issue in other
places, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [RESEND PATCH v2] devres: Really align data field to unsigned long long
From: Alexey Brodkin
> Sent: 09 July 2018 05:45
> Depending on ABI "long long" type of a particular 32-bit CPU
> might be aligned by either word (32-bits) or double word (64-bits).
> Make sure "data" is really 64-bit aligned for any 32-bit CPU.
>
> At least for 32-bit ARC cores ABI requires "long long" types
> to be aligned by normal 32-bit word. This makes "data" field aligned to
> 12 bytes. Which is still OK as long as we use 32-bit data only.
>
> But once we want to use native atomic64_t type (i.e. when we use special
> instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> misaligned access exception.
Shouldn't there be a typedef for the actual type.
Perhaps it is even atomic64_t ?
And have the __aligned(8) applied to that typedef ??
> That's because even on CPUs capable of non-aligned data access LL/SC
> instructions require strict alignment.
...
> --- 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 */
> + /*
> + * Depending on ABI "long long" type of a particular 32-bit CPU
> + * might be aligned by either word (32-bits) or double word (64-bits).
> + * Make sure "data" is really 64-bit aligned for any 32-bit CPU.
Just:
/* data[] must be 64 bit aligned even on 32 bit architectures
* because it might be accessed by instructions that require
* aligned memory arguments.
> + */
> + unsigned long long data[] __aligned(sizeof(unsigned long
> long));
One day assuming that 'unsigned long long' is exactly 64 bits will bite.
This probably ought to be u64 (or similar).
(If not atomic64_t)
David
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
Hi Alexey,
On Mon, Jul 9, 2018 at 10:37 AM Alexey Brodkin
wrote:
> On Mon, 2018-07-09 at 09:52 +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 9, 2018 at 9:22 AM Alexey Brodkin
> > wrote:
> > > On Mon, 2018-07-09 at 09:07 +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Jul 9, 2018 at 7:49 AM Greg KH wrote:
> > > > > On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > > > > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > > > > might be aligned by either word (32-bits) or double word (64-bits).
> > > >
> > > > Or even 16-bit (on e.g. m68k).
> > >
> > > Indeed, thanks for the note!
> > > Will add this in my v3.
> >
> > Note that in this particular case, the field will probably always be
> > aligned to 4 bytes,
> > as the struct will be allocated using *alloc().
>
> Well I think maybe it worth mentioning only 32-bit and 64-bit alignments in
> this particular case because as it was mentioned initially in the comment
> there're
> 3 pointers before "data" field and for 32-bit machines I guess we may safely
> assume that a pointer size is 32-bit.
>
> This leaves us only one problematic situation 32-bit instead of 64-bit
> alignment.
>
> > > For ARC I'd like this fix to be back-ported starting
> > > from v4.8 where we added support of "native" atomic64_t, see commit
> > > ce6365270ecd (" ARCv2: Implement atomic64 based on LLOCKD/SCONDD
> > > instructions").
> > >
> > > What about m68k, do you have any preference of earliest kernel version
> > > where this fix might be useful?
> >
> > Given m68k is 32-bit, it will access atomic64_t variables while
> > holding a spinlock,
> > so it should still be safe without this change.
>
> Well ARC and ARMv7 are 32-bit machines as well still both have
> a special instructions to read/write 64-bit data.
Sure.
> > Not to mention no one will try etnaviv on m68k ;-)
>
> See it was just a trigger case but other GPUs that use or will use
> DRM GPU scheduler are good candidates to it that problem and not to
> mention some other users of atomic64_t.
>
> But from you above comment I may deduce that m68k doesn't have
> instructions for 64-bit data; in that case there's no reason to worry
> at least about struct devres_node :)
That's correct: m68k has no instructions for 64-bit data.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
Hi Geert,
On Mon, 2018-07-09 at 09:52 +0200, Geert Uytterhoeven wrote:
> Hi Alexey,
>
> On Mon, Jul 9, 2018 at 9:22 AM Alexey Brodkin
> wrote:
> > On Mon, 2018-07-09 at 09:07 +0200, Geert Uytterhoeven wrote:
> > > On Mon, Jul 9, 2018 at 7:49 AM Greg KH wrote:
> > > > On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > > > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > > > might be aligned by either word (32-bits) or double word (64-bits).
> > >
> > > Or even 16-bit (on e.g. m68k).
> >
> > Indeed, thanks for the note!
> > Will add this in my v3.
>
> Note that in this particular case, the field will probably always be
> aligned to 4 bytes,
> as the struct will be allocated using *alloc().
Well I think maybe it worth mentioning only 32-bit and 64-bit alignments in
this particular case because as it was mentioned initially in the comment
there're
3 pointers before "data" field and for 32-bit machines I guess we may safely
assume that a pointer size is 32-bit.
This leaves us only one problematic situation 32-bit instead of 64-bit
alignment.
> > For ARC I'd like this fix to be back-ported starting
> > from v4.8 where we added support of "native" atomic64_t, see commit
> > ce6365270ecd (" ARCv2: Implement atomic64 based on LLOCKD/SCONDD
> > instructions").
> >
> > What about m68k, do you have any preference of earliest kernel version
> > where this fix might be useful?
>
> Given m68k is 32-bit, it will access atomic64_t variables while
> holding a spinlock,
> so it should still be safe without this change.
Well ARC and ARMv7 are 32-bit machines as well still both have
a special instructions to read/write 64-bit data.
> Not to mention no one will try etnaviv on m68k ;-)
See it was just a trigger case but other GPUs that use or will use
DRM GPU scheduler are good candidates to it that problem and not to
mention some other users of atomic64_t.
But from you above comment I may deduce that m68k doesn't have
instructions for 64-bit data; in that case there's no reason to worry
at least about struct devres_node :)
-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
Hi Alexey,
On Mon, Jul 9, 2018 at 9:22 AM Alexey Brodkin
wrote:
> On Mon, 2018-07-09 at 09:07 +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 9, 2018 at 7:49 AM Greg KH wrote:
> > > On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > > might be aligned by either word (32-bits) or double word (64-bits).
> >
> > Or even 16-bit (on e.g. m68k).
>
> Indeed, thanks for the note!
> Will add this in my v3.
Note that in this particular case, the field will probably always be
aligned to 4 bytes,
as the struct will be allocated using *alloc().
> For ARC I'd like this fix to be back-ported starting
> from v4.8 where we added support of "native" atomic64_t, see commit
> ce6365270ecd (" ARCv2: Implement atomic64 based on LLOCKD/SCONDD
> instructions").
>
> What about m68k, do you have any preference of earliest kernel version
> where this fix might be useful?
Given m68k is 32-bit, it will access atomic64_t variables while
holding a spinlock,
so it should still be safe without this change.
Not to mention no one will try etnaviv on m68k ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
On Mon, Jul 09, 2018 at 07:17:11AM +, Alexey Brodkin wrote:
> Hi Greg,
>
> On Mon, 2018-07-09 at 09:06 +0200, g...@kroah.com wrote:
> > On Mon, Jul 09, 2018 at 06:46:50AM +, Alexey Brodkin wrote:
> > > Hi Greg,
> > >
> > > On Mon, 2018-07-09 at 07:48 +0200, Greg KH wrote:
> > > > On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > > > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > > > might be aligned by either word (32-bits) or double word (64-bits).
> > > > > Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> > > > >
> > > > > At least for 32-bit ARC cores ABI requires "long long" types
> > > > > to be aligned by normal 32-bit word. This makes "data" field aligned
> > > > > to
> > > > > 12 bytes. Which is still OK as long as we use 32-bit data only.
> > > > >
> > > > > But once we want to use native atomic64_t type (i.e. when we use
> > > > > special
> > > > > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> > > > > misaligned access exception.
> > > >
> > > > So is this something you hit today? If not, why is this needed for
> > > > stable kernels?
> > >
> > > Indeed we hit that problem recently when Etnaviv driver was switched to
> > > DRM GPU scheduler, see
> > > commit e93b6deeb45a ("drm/etnaviv: hook up DRM GPU scheduler").
> > > The most important part of DRM GPU scheduler is "job_id_count" member of
> > > "drm_gpu_scheduler" structure of type "atomic64_t". This structure is put
> > > in a buffer allocated by devm_kzalloc() and if "job_id_count" is not
> > > 64-bit
> > > aligned atomic instruction fails with an exception.
> > >
> > > As for stable requirements - mentioned commit was a part of 4.17 kernel
> > > which broke GPU driver for one of our HSDK board so I guess back-porting
> > > to 4.17 is a no-brainer.
> >
> > Ok, so 4.17 is as far back as you need? Please try to be specific when
> > asking for stable backports.
>
> Well in that particular case I really wanted to get this fix back-ported
> starting from v4.8 so I guess that's what I need to add in Cc tag, right?
> ->8-
> Cc: # 4.8+
> ->8-
Yes.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
Hi Geert,
On Mon, 2018-07-09 at 09:07 +0200, Geert Uytterhoeven wrote:
> On Mon, Jul 9, 2018 at 7:49 AM Greg KH wrote:
> > On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > might be aligned by either word (32-bits) or double word (64-bits).
>
> Or even 16-bit (on e.g. m68k).
Indeed, thanks for the note!
Will add this in my v3.
For ARC I'd like this fix to be back-ported starting
from v4.8 where we added support of "native" atomic64_t, see commit
ce6365270ecd (" ARCv2: Implement atomic64 based on LLOCKD/SCONDD instructions").
What about m68k, do you have any preference of earliest kernel version
where this fix might be useful?
-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
On Mon, Jul 9, 2018 at 7:49 AM Greg KH wrote:
> On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > Depending on ABI "long long" type of a particular 32-bit CPU
> > might be aligned by either word (32-bits) or double word (64-bits).
Or even 16-bit (on e.g. m68k).
> > --- 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 */
> > + /*
> > + * Depending on ABI "long long" type of a particular 32-bit CPU
> > + * might be aligned by either word (32-bits) or double word (64-bits).
or even 16-bit (on e.g. m68k).
> > + * Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> > + */
> > + unsigned long long data[] __aligned(sizeof(unsigned long
> > long));
> > };
>
> Does this change the padding today for any other arches? If so, does
> the increased memory usage cause any noticable issues?
Yes it does. struct devres_node contains an odd number of pointers, so
there will be a hole between node and data on all 32-bit architectures.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
On Mon, Jul 09, 2018 at 06:46:50AM +, Alexey Brodkin wrote:
> Hi Greg,
>
> On Mon, 2018-07-09 at 07:48 +0200, Greg KH wrote:
> > On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > might be aligned by either word (32-bits) or double word (64-bits).
> > > Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> > >
> > > At least for 32-bit ARC cores ABI requires "long long" types
> > > to be aligned by normal 32-bit word. This makes "data" field aligned to
> > > 12 bytes. Which is still OK as long as we use 32-bit data only.
> > >
> > > But once we want to use native atomic64_t type (i.e. when we use special
> > > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> > > misaligned access exception.
> >
> > So is this something you hit today? If not, why is this needed for
> > stable kernels?
>
> Indeed we hit that problem recently when Etnaviv driver was switched to
> DRM GPU scheduler, see
> commit e93b6deeb45a ("drm/etnaviv: hook up DRM GPU scheduler").
> The most important part of DRM GPU scheduler is "job_id_count" member of
> "drm_gpu_scheduler" structure of type "atomic64_t". This structure is put
> in a buffer allocated by devm_kzalloc() and if "job_id_count" is not 64-bit
> aligned atomic instruction fails with an exception.
>
> As for stable requirements - mentioned commit was a part of 4.17 kernel
> which broke GPU driver for one of our HSDK board so I guess back-porting
> to 4.17 is a no-brainer.
Ok, so 4.17 is as far back as you need? Please try to be specific when
asking for stable backports.
> > > That's because even on CPUs capable of non-aligned data access LL/SC
> > > instructions require strict alignment.
> >
> > Are you going to hit this code with all types of structures?
>
> If there're other cases which lead to 4-byte aligned "atomic64_t" variables
> there will be a problem as well but it's quite hard to predict those cases.
> That said if we manage to reproduce more similar issues there will be more
> patches with fixes :)
>
> > What happens when you do have an unaligned access?
>
> Atomic instructions are a bit special as 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.
>
> And that's not something special for ARC, I guess all CPUs are the same in
> that regard, see here's an extract from ARM(r) Architecture Reference
> Manual ARMv7-A and ARMv7-R edition: https://lkml.org/lkml/2017/12/5/440
> From "Table A3-1 Alignment requirements of load/store instructions"
> it's seen that LDREXD, STREXD instructions will cause alignment fault
> even if SCTLR.A=0 (strict alignment fault checking disabled) for non
> double-word-aligned data.
Thanks for the better explaination, that helps out a lot. Can you redo
the patch with all of that information so that others do not have the
same questions as I did?
thanks,
greg k-h
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
Hi Greg,
On Mon, 2018-07-09 at 07:48 +0200, Greg KH wrote:
> On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > Depending on ABI "long long" type of a particular 32-bit CPU
> > might be aligned by either word (32-bits) or double word (64-bits).
> > Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> >
> > At least for 32-bit ARC cores ABI requires "long long" types
> > to be aligned by normal 32-bit word. This makes "data" field aligned to
> > 12 bytes. Which is still OK as long as we use 32-bit data only.
> >
> > But once we want to use native atomic64_t type (i.e. when we use special
> > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> > misaligned access exception.
>
> So is this something you hit today? If not, why is this needed for
> stable kernels?
Indeed we hit that problem recently when Etnaviv driver was switched to
DRM GPU scheduler, see
commit e93b6deeb45a ("drm/etnaviv: hook up DRM GPU scheduler").
The most important part of DRM GPU scheduler is "job_id_count" member of
"drm_gpu_scheduler" structure of type "atomic64_t". This structure is put
in a buffer allocated by devm_kzalloc() and if "job_id_count" is not 64-bit
aligned atomic instruction fails with an exception.
As for stable requirements - mentioned commit was a part of 4.17 kernel
which broke GPU driver for one of our HSDK board so I guess back-porting
to 4.17 is a no-brainer.
Now given a nature of the problem at hand I may expect other breakages
whenever another driver/fs etc that use "atomic64_t" gets enabled.
> > That's because even on CPUs capable of non-aligned data access LL/SC
> > instructions require strict alignment.
>
> Are you going to hit this code with all types of structures?
If there're other cases which lead to 4-byte aligned "atomic64_t" variables
there will be a problem as well but it's quite hard to predict those cases.
That said if we manage to reproduce more similar issues there will be more
patches with fixes :)
> What happens when you do have an unaligned access?
Atomic instructions are a bit special as 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.
And that's not something special for ARC, I guess all CPUs are the same in
that regard, see here's an extract from ARM(r) Architecture Reference
Manual ARMv7-A and ARMv7-R edition: https://lkml.org/lkml/2017/12/5/440
>From "Table A3-1 Alignment requirements of load/store instructions"
it's seen that LDREXD, STREXD instructions will cause alignment fault
even if SCTLR.A=0 (strict alignment fault checking disabled) for non
double-word-aligned data.
> >
> > Signed-off-by: Alexey Brodkin
> > Cc: Greg Kroah-Hartman
>
> You didn't cc: this address :(
Sorry, which one?
That what I have in .mbox, see https://patchwork.ozlabs.org/patch/941092/mbox/:
-->8--
Cc: linux-a...@vger.kernel.org, Greg KH ,
Alexey Brodkin , sta...@vger.kernel.org,
Greg Kroah-Hartman ,
Thomas Gleixner , linux-snps-arc@lists.infradead.org
-->8--
-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
