Re: [PATCH 1/4] block: use ARCH_KMALLOC_MINALIGN as the default dma pad mask

Andrew Morton
2008-05-20T09:31:29Z

On Tue, 20 May 2008 13:58:31 +0900 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> This sets the default dma pad mask to ARCH_KMALLOC_MINALIGN in
> blk_queue_make_request(). It affects only non-coherent platforms.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> ---
> block/blk-settings.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 8dd8641..781d1bf 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -84,6 +84,11 @@ EXPORT_SYMBOL(blk_queue_softirq_done);
> **/
> void blk_queue_make_request(struct request_queue *q, make_request_fn *mfn)
> {
> +#ifndef ARCH_KMALLOC_MINALIGN
> +#define ARCH_KMALLOC_MINALIGN 1
> +#endif
> + unsigned int min_align = ARCH_KMALLOC_MINALIGN;
> +
> /*
> * set defaults
> */
> @@ -98,6 +103,7 @@ void blk_queue_make_request(struct request_queue *q, make_request_fn *mfn)
> blk_queue_max_sectors(q, SAFE_MAX_SECTORS);
> blk_queue_hardsect_size(q, 512);
> blk_queue_dma_alignment(q, 511);
> + blk_queue_dma_pad(q, min_align - 1);
> blk_queue_congestion_threshold(q);
> q->nr_batching = BLK_BATCH_REQ;

urgh. This ARCH_KMALLOC_MINALIGN thing has the smell of an expedient
hack which is now growing.

Look at what crypto did (which seems to be a lot worse):

/*
* The macro CRYPTO_MINALIGN_ATTR (along with the void * type in the actual
* declaration) is used to ensure that the crypto_tfm context structure is
* aligned correctly for the given architecture so that there are no alignment
* faults for C data types. In particular, this is required on platforms such
* as arm where pointers are 32-bit aligned but there are data types such as
* u64 which require 64-bit alignment.
*/
#if defined(ARCH_KMALLOC_MINALIGN)
#define CRYPTO_MINALIGN ARCH_KMALLOC_MINALIGN
#elif defined(ARCH_SLAB_MINALIGN)
#define CRYPTO_MINALIGN ARCH_SLAB_MINALIGN
#else
#define CRYPTO_MINALIGN __alignof__(unsigned long long)
#endif

So here you're using it for "dma aligment" whereas crypto is using it
(or ARCH_SLAB_MINALIGN!) for "cpu 64-bit alignment".

Why does ARCH_KMALLOC_MINALIGN even exist? What is its mandate? Sigh.

It's not really related to your patch (although your patch compounds
the problem a little). But we should sit down and work out what we
actually want to do here. Something like:

In each architecture's arch/foo/Kconfig, define

CONFIG_ARCH_DMA_ALIGN

and

CONFIG_ARCH_64BIT_POINTER_ALIGN

and then use them. Note that these have nothing to do with each other,
as far as I can tell.

Which leaves the question: "what should slab use"? Maybe
CONFIG_ARCH_DMA_ALIGN? But that depends what ARCH_KMALLOC_MINALIGN is
supposed to exist for.

ick.