[PATCH v3] devres: Explicitly align datai[] to 64-bit

[PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

[PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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?

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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[];
>  };

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Mark Rutland]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Mark Rutland]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Geert Uytterhoeven]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Geert Uytterhoeven]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Alexey Brodkin]

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

RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[David Laight]

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

RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[David Laight]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Mark Rutland]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Mark Rutland]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Peter Zijlstra]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Mark Rutland]

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.

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Mark Rutland]

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.

RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[David Laight]

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

RE: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[David Laight]

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

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Vineet Gupta]

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; })

Re: [PATCH v3] devres: Explicitly align datai[] to 64-bit

[Vineet Gupta]

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; })