| Message ID | 20230531154836.1366225-2-catalin.marinas@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, dma, arm64: Reduce ARCH_KMALLOC_MINALIGN to 8 | expand |
On 5/31/23 17:48, Catalin Marinas wrote: > In preparation for supporting a kmalloc() minimum alignment smaller than > the arch DMA alignment, decouple the two definitions. This requires that > either the kmalloc() caches are aligned to a (run-time) cache-line size > or the DMA API bounces unaligned kmalloc() allocations. Subsequent > patches will implement both options. > > After this patch, ARCH_DMA_MINALIGN is expected to be used in static > alignment annotations and defined by an architecture to be the maximum > alignment for all supported configurations/SoCs in a single Image. > Architectures opting in to a smaller ARCH_KMALLOC_MINALIGN will need to > define its value in the arch headers. > > Since ARCH_DMA_MINALIGN is now always defined, adjust the #ifdef in > dma_get_cache_alignment() so that there is no change for architectures > not requiring a minimum DMA alignment. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Robin Murphy <robin.murphy@arm.com> > --- > include/linux/dma-mapping.h | 2 +- > include/linux/slab.h | 14 +++++++++++--- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 0ee20b764000..3288a1339271 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -545,7 +545,7 @@ static inline int dma_set_min_align_mask(struct device *dev, > > static inline int dma_get_cache_alignment(void) > { > -#ifdef ARCH_DMA_MINALIGN > +#ifdef ARCH_HAS_DMA_MINALIGN > return ARCH_DMA_MINALIGN; > #endif > return 1; > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 6b3e155b70bf..50dcf9cfbf62 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -235,12 +235,20 @@ void kmem_dump_obj(void *object); > * alignment larger than the alignment of a 64-bit integer. > * Setting ARCH_DMA_MINALIGN in arch headers allows that. > */ > -#if defined(ARCH_DMA_MINALIGN) && ARCH_DMA_MINALIGN > 8 > +#ifdef ARCH_DMA_MINALIGN > +#define ARCH_HAS_DMA_MINALIGN > +#if ARCH_DMA_MINALIGN > 8 && !defined(ARCH_KMALLOC_MINALIGN) > #define ARCH_KMALLOC_MINALIGN ARCH_DMA_MINALIGN > -#define KMALLOC_MIN_SIZE ARCH_DMA_MINALIGN > -#define KMALLOC_SHIFT_LOW ilog2(ARCH_DMA_MINALIGN) > +#endif > #else > +#define ARCH_DMA_MINALIGN __alignof__(unsigned long long) > +#endif It seems weird to make slab.h responsible for this part, especially for #define ARCH_HAS_DMA_MINALIGN, which dma-mapping.h consumes. Maybe it would be difficult to do differently due to some dependency hell, but minimally I don't see dma-mapping.h including slab.h so the result is include-order-dependent? Maybe it's included transitively, but then it's fragile and would be better to do explicitly? > + > +#ifndef ARCH_KMALLOC_MINALIGN > #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long) > +#elif ARCH_KMALLOC_MINALIGN > 8 > +#define KMALLOC_MIN_SIZE ARCH_KMALLOC_MINALIGN > +#define KMALLOC_SHIFT_LOW ilog2(KMALLOC_MIN_SIZE) > #endif > > /* >
On Fri, Jun 09, 2023 at 02:32:57PM +0200, Vlastimil Babka wrote: > On 5/31/23 17:48, Catalin Marinas wrote: > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > > index 0ee20b764000..3288a1339271 100644 > > --- a/include/linux/dma-mapping.h > > +++ b/include/linux/dma-mapping.h > > @@ -545,7 +545,7 @@ static inline int dma_set_min_align_mask(struct device *dev, > > > > static inline int dma_get_cache_alignment(void) > > { > > -#ifdef ARCH_DMA_MINALIGN > > +#ifdef ARCH_HAS_DMA_MINALIGN > > return ARCH_DMA_MINALIGN; > > #endif > > return 1; > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > index 6b3e155b70bf..50dcf9cfbf62 100644 > > --- a/include/linux/slab.h > > +++ b/include/linux/slab.h > > @@ -235,12 +235,20 @@ void kmem_dump_obj(void *object); > > * alignment larger than the alignment of a 64-bit integer. > > * Setting ARCH_DMA_MINALIGN in arch headers allows that. > > */ > > -#if defined(ARCH_DMA_MINALIGN) && ARCH_DMA_MINALIGN > 8 > > +#ifdef ARCH_DMA_MINALIGN > > +#define ARCH_HAS_DMA_MINALIGN > > +#if ARCH_DMA_MINALIGN > 8 && !defined(ARCH_KMALLOC_MINALIGN) > > #define ARCH_KMALLOC_MINALIGN ARCH_DMA_MINALIGN > > -#define KMALLOC_MIN_SIZE ARCH_DMA_MINALIGN > > -#define KMALLOC_SHIFT_LOW ilog2(ARCH_DMA_MINALIGN) > > +#endif > > #else > > +#define ARCH_DMA_MINALIGN __alignof__(unsigned long long) > > +#endif > > It seems weird to make slab.h responsible for this part, especially for > #define ARCH_HAS_DMA_MINALIGN, which dma-mapping.h consumes. Maybe it would > be difficult to do differently due to some dependency hell, but minimally I > don't see dma-mapping.h including slab.h so the result is > include-order-dependent? Maybe it's included transitively, but then it's > fragile and would be better to do explicitly? True, there's a risk that it doesn't get included with some future header refactoring. What about moving ARCH_DMA_MINALIGN to linux/cache.h? Alternatively, I could create a new linux/dma-minalign.h file but I feel since this is about caches, having it in cache.h makes more sense. asm/cache.h is also where most archs define the constant (apart from mips, sh, microblaze).
On Fri, Jun 09, 2023 at 02:44:01PM +0100, Catalin Marinas wrote: > On Fri, Jun 09, 2023 at 02:32:57PM +0200, Vlastimil Babka wrote: > > On 5/31/23 17:48, Catalin Marinas wrote: > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > > > index 0ee20b764000..3288a1339271 100644 > > > --- a/include/linux/dma-mapping.h > > > +++ b/include/linux/dma-mapping.h > > > @@ -545,7 +545,7 @@ static inline int dma_set_min_align_mask(struct device *dev, > > > > > > static inline int dma_get_cache_alignment(void) > > > { > > > -#ifdef ARCH_DMA_MINALIGN > > > +#ifdef ARCH_HAS_DMA_MINALIGN > > > return ARCH_DMA_MINALIGN; > > > #endif > > > return 1; > > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > > index 6b3e155b70bf..50dcf9cfbf62 100644 > > > --- a/include/linux/slab.h > > > +++ b/include/linux/slab.h > > > @@ -235,12 +235,20 @@ void kmem_dump_obj(void *object); > > > * alignment larger than the alignment of a 64-bit integer. > > > * Setting ARCH_DMA_MINALIGN in arch headers allows that. > > > */ > > > -#if defined(ARCH_DMA_MINALIGN) && ARCH_DMA_MINALIGN > 8 > > > +#ifdef ARCH_DMA_MINALIGN > > > +#define ARCH_HAS_DMA_MINALIGN > > > +#if ARCH_DMA_MINALIGN > 8 && !defined(ARCH_KMALLOC_MINALIGN) > > > #define ARCH_KMALLOC_MINALIGN ARCH_DMA_MINALIGN > > > -#define KMALLOC_MIN_SIZE ARCH_DMA_MINALIGN > > > -#define KMALLOC_SHIFT_LOW ilog2(ARCH_DMA_MINALIGN) > > > +#endif > > > #else > > > +#define ARCH_DMA_MINALIGN __alignof__(unsigned long long) > > > +#endif > > > > It seems weird to make slab.h responsible for this part, especially for > > #define ARCH_HAS_DMA_MINALIGN, which dma-mapping.h consumes. Maybe it would > > be difficult to do differently due to some dependency hell, but minimally I > > don't see dma-mapping.h including slab.h so the result is > > include-order-dependent? Maybe it's included transitively, but then it's > > fragile and would be better to do explicitly? > > True, there's a risk that it doesn't get included with some future > header refactoring. > > What about moving ARCH_DMA_MINALIGN to linux/cache.h? Alternatively, I > could create a new linux/dma-minalign.h file but I feel since this is > about caches, having it in cache.h makes more sense. asm/cache.h is also > where most archs define the constant (apart from mips, sh, microblaze). Something like this (still compiling): diff --git a/include/linux/cache.h b/include/linux/cache.h index 5da1bbd96154..9900d20b76c2 100644 --- a/include/linux/cache.h +++ b/include/linux/cache.h @@ -98,4 +98,10 @@ struct cacheline_padding { #define CACHELINE_PADDING(name) #endif +#ifdef ARCH_DMA_MINALIGN +#define ARCH_HAS_DMA_MINALIGN +#else +#define ARCH_DMA_MINALIGN __alignof__(unsigned long long) +#endif + #endif /* __LINUX_CACHE_H */ diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index c41019289223..e13050eb9777 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -2,6 +2,7 @@ #ifndef _LINUX_DMA_MAPPING_H #define _LINUX_DMA_MAPPING_H +#include <linux/cache.h> #include <linux/sizes.h> #include <linux/string.h> #include <linux/device.h> diff --git a/include/linux/slab.h b/include/linux/slab.h index 50dcf9cfbf62..9bdfb042d93d 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -12,6 +12,7 @@ #ifndef _LINUX_SLAB_H #define _LINUX_SLAB_H +#include <linux/cache.h> #include <linux/gfp.h> #include <linux/overflow.h> #include <linux/types.h> @@ -235,14 +236,10 @@ void kmem_dump_obj(void *object); * alignment larger than the alignment of a 64-bit integer. * Setting ARCH_DMA_MINALIGN in arch headers allows that. */ -#ifdef ARCH_DMA_MINALIGN -#define ARCH_HAS_DMA_MINALIGN -#if ARCH_DMA_MINALIGN > 8 && !defined(ARCH_KMALLOC_MINALIGN) +#if defined(ARCH_HAS_DMA_MINALIGN) && ARCH_DMA_MINALIGN > 8 && \ + !defined(ARCH_KMALLOC_MINALIGN) #define ARCH_KMALLOC_MINALIGN ARCH_DMA_MINALIGN #endif -#else -#define ARCH_DMA_MINALIGN __alignof__(unsigned long long) -#endif #ifndef ARCH_KMALLOC_MINALIGN #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
On 6/9/23 15:57, Catalin Marinas wrote: > On Fri, Jun 09, 2023 at 02:44:01PM +0100, Catalin Marinas wrote: >> On Fri, Jun 09, 2023 at 02:32:57PM +0200, Vlastimil Babka wrote: >> > On 5/31/23 17:48, Catalin Marinas wrote: >> > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h >> > > index 0ee20b764000..3288a1339271 100644 >> > > --- a/include/linux/dma-mapping.h >> > > +++ b/include/linux/dma-mapping.h >> > > @@ -545,7 +545,7 @@ static inline int dma_set_min_align_mask(struct device *dev, >> > > >> > > static inline int dma_get_cache_alignment(void) >> > > { >> > > -#ifdef ARCH_DMA_MINALIGN >> > > +#ifdef ARCH_HAS_DMA_MINALIGN >> > > return ARCH_DMA_MINALIGN; >> > > #endif >> > > return 1; >> > > diff --git a/include/linux/slab.h b/include/linux/slab.h >> > > index 6b3e155b70bf..50dcf9cfbf62 100644 >> > > --- a/include/linux/slab.h >> > > +++ b/include/linux/slab.h >> > > @@ -235,12 +235,20 @@ void kmem_dump_obj(void *object); >> > > * alignment larger than the alignment of a 64-bit integer. >> > > * Setting ARCH_DMA_MINALIGN in arch headers allows that. >> > > */ >> > > -#if defined(ARCH_DMA_MINALIGN) && ARCH_DMA_MINALIGN > 8 >> > > +#ifdef ARCH_DMA_MINALIGN >> > > +#define ARCH_HAS_DMA_MINALIGN >> > > +#if ARCH_DMA_MINALIGN > 8 && !defined(ARCH_KMALLOC_MINALIGN) >> > > #define ARCH_KMALLOC_MINALIGN ARCH_DMA_MINALIGN >> > > -#define KMALLOC_MIN_SIZE ARCH_DMA_MINALIGN >> > > -#define KMALLOC_SHIFT_LOW ilog2(ARCH_DMA_MINALIGN) >> > > +#endif >> > > #else >> > > +#define ARCH_DMA_MINALIGN __alignof__(unsigned long long) >> > > +#endif >> > >> > It seems weird to make slab.h responsible for this part, especially for >> > #define ARCH_HAS_DMA_MINALIGN, which dma-mapping.h consumes. Maybe it would >> > be difficult to do differently due to some dependency hell, but minimally I >> > don't see dma-mapping.h including slab.h so the result is >> > include-order-dependent? Maybe it's included transitively, but then it's >> > fragile and would be better to do explicitly? >> >> True, there's a risk that it doesn't get included with some future >> header refactoring. >> >> What about moving ARCH_DMA_MINALIGN to linux/cache.h? Alternatively, I >> could create a new linux/dma-minalign.h file but I feel since this is >> about caches, having it in cache.h makes more sense. asm/cache.h is also >> where most archs define the constant (apart from mips, sh, microblaze). > > Something like this (still compiling): Yeah that would be great!
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 0ee20b764000..3288a1339271 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -545,7 +545,7 @@ static inline int dma_set_min_align_mask(struct device *dev, static inline int dma_get_cache_alignment(void) { -#ifdef ARCH_DMA_MINALIGN +#ifdef ARCH_HAS_DMA_MINALIGN return ARCH_DMA_MINALIGN; #endif return 1; diff --git a/include/linux/slab.h b/include/linux/slab.h index 6b3e155b70bf..50dcf9cfbf62 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -235,12 +235,20 @@ void kmem_dump_obj(void *object); * alignment larger than the alignment of a 64-bit integer. * Setting ARCH_DMA_MINALIGN in arch headers allows that. */ -#if defined(ARCH_DMA_MINALIGN) && ARCH_DMA_MINALIGN > 8 +#ifdef ARCH_DMA_MINALIGN +#define ARCH_HAS_DMA_MINALIGN +#if ARCH_DMA_MINALIGN > 8 && !defined(ARCH_KMALLOC_MINALIGN) #define ARCH_KMALLOC_MINALIGN ARCH_DMA_MINALIGN -#define KMALLOC_MIN_SIZE ARCH_DMA_MINALIGN -#define KMALLOC_SHIFT_LOW ilog2(ARCH_DMA_MINALIGN) +#endif #else +#define ARCH_DMA_MINALIGN __alignof__(unsigned long long) +#endif + +#ifndef ARCH_KMALLOC_MINALIGN #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long) +#elif ARCH_KMALLOC_MINALIGN > 8 +#define KMALLOC_MIN_SIZE ARCH_KMALLOC_MINALIGN +#define KMALLOC_SHIFT_LOW ilog2(KMALLOC_MIN_SIZE) #endif /*
In preparation for supporting a kmalloc() minimum alignment smaller than the arch DMA alignment, decouple the two definitions. This requires that either the kmalloc() caches are aligned to a (run-time) cache-line size or the DMA API bounces unaligned kmalloc() allocations. Subsequent patches will implement both options. After this patch, ARCH_DMA_MINALIGN is expected to be used in static alignment annotations and defined by an architecture to be the maximum alignment for all supported configurations/SoCs in a single Image. Architectures opting in to a smaller ARCH_KMALLOC_MINALIGN will need to define its value in the arch headers. Since ARCH_DMA_MINALIGN is now always defined, adjust the #ifdef in dma_get_cache_alignment() so that there is no change for architectures not requiring a minimum DMA alignment. Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Robin Murphy <robin.murphy@arm.com> --- include/linux/dma-mapping.h | 2 +- include/linux/slab.h | 14 +++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-)