RE: [PATCH AUTOSEL 4.14 72/95] devres: Align data[] to ARCH_KMALLOC_MINALIGN

[Alexey Brodkin]

Hi Greg,

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, May 7, 2019 8:52 AM
> To: Sasha Levin <sashal@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx; Alexey Brodkin <abrodkin@xxxxxxxxxxxx>;
> Alexey Brodkin <abrodkin@xxxxxxxxxxxx>; Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>; David Laight
> <David.Laight@xxxxxxxxxx>; Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Thomas Gleixner
> <tglx@xxxxxxxxxxxxx>; Vineet Gupta <vgupta@xxxxxxxxxxxx>; Will Deacon <will.deacon@xxxxxxx>; Sasha
> Levin <alexander.levin@xxxxxxxxxxxxx>
> Subject: Re: [PATCH AUTOSEL 4.14 72/95] devres: Align data[] to ARCH_KMALLOC_MINALIGN
> 
> On Tue, May 07, 2019 at 01:38:01AM -0400, Sasha Levin wrote:
> > From: Alexey Brodkin <alexey.brodkin@xxxxxxxxxxxx>
> >
> > [ Upstream commit a66d972465d15b1d89281258805eb8b47d66bd36 ]
> >
> > Initially we bumped into problem with 32-bit aligned atomic64_t
> > on ARC, see [1]. And then during quite lengthly discussion Peter Z.
> > mentioned ARCH_KMALLOC_MINALIGN which IMHO makes perfect sense.
> > If allocation is done by plain kmalloc() obtained buffer will be
> > ARCH_KMALLOC_MINALIGN aligned and then why buffer obtained via
> > devm_kmalloc() should have any other alignment?
> >
> > This way we at least get the same behavior for both types of
> > allocation.
> >
> > [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_pipermail_linux-2Dsnps-
> 2Darc_2018-
> 2DJuly_004009.html&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=A
> YtkWKU38pzVfJMBuK0lUwxRyKT6dDfHoD3yO6OIB5k&s=e7e2sXKcjHDQdGSrKWM0jmpSOfhe0MFk4-nMZJe9En8&e=
> > [2] https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_pipermail_linux-2Dsnps-
> 2Darc_2018-
> 2DJuly_004036.html&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=A
> YtkWKU38pzVfJMBuK0lUwxRyKT6dDfHoD3yO6OIB5k&s=L23zrl8rf2MmReUI8rT3FQpMiZU9H3Xjh9uVxJQe8dw&e=
> >
> > Signed-off-by: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Cc: David Laight <David.Laight@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Vineet Gupta <vgupta@xxxxxxxxxxxx>
> > Cc: Will Deacon <will.deacon@xxxxxxx>
> > Cc: Greg KH <greg@xxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # 4.8+
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxxxx>
> > ---
> >  drivers/base/devres.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > index 71d577025285..e43a04a495a3 100644
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -25,8 +25,14 @@ struct devres_node {
> >
> >  struct devres {
> >  	struct devres_node		node;
> > -	/* -- 3 pointers */
> > -	unsigned long long		data[];	/* guarantee ull alignment */
> > +	/*
> > +	 * Some archs want to perform DMA into kmalloc caches
> > +	 * and need a guaranteed alignment larger than
> > +	 * the alignment of a 64-bit integer.
> > +	 * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same
> > +	 * buffer alignment as if it was allocated by plain kmalloc().
> > +	 */
> > +	u8 __aligned(ARCH_KMALLOC_MINALIGN) data[];
> >  };
> >
> >  struct devres_group {
> 
> This is not needed in any of the older kernels, despite what the stable@
> line said, as it ends up taking a lot of memory up for all other arches.
> That's why I only applied it to the one kernel version.  I'm betting
> that it will be eventually reverted when people notice it as well :)

That very well might become the case but then we're back to the initial problem,
right? So maybe some other more future-proof solution should be implemented?

See initially we discussed simple explicit 8-byte alignment which won't change
data layout for most of arches while fixing our issue on ARC but for some reason
people were not happy with that proposal and that's how we ended-up with what we
discuss here now.

-Alexey