[PATCH] dma-contiguous: fix the Kconfig entry for CONFIG_DMA_NUMA_CMA

[PATCH] dma-contiguous: fix the Kconfig entry for CONFIG_DMA_NUMA_CMA

[Christoph Hellwig]

It makes no sense to expose CONFIG_DMA_NUMA_CMA if CONFIG_NUMA is not
enabled, and random config options shouldn't be default unless there
is a good reason.  Replace the default NUMA with a depends on to fix both
issues.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/dma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index 4c1e9a3c0ab6f0..f488997b071712 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -160,7 +160,7 @@ if  DMA_CMA
 
 config DMA_NUMA_CMA
 	bool "Enable separate DMA Contiguous Memory Area for NUMA Node"
-	default NUMA
+	depends on NUMA
 	help
 	  Enable this option to get numa CMA areas so that NUMA devices
 	  can get local memory by DMA coherent APIs.
-- 
2.39.2

Re: [PATCH] dma-contiguous: fix the Kconfig entry for CONFIG_DMA_NUMA_CMA

[Robin Murphy]

On 2023-08-30 10:27, Christoph Hellwig wrote:
> It makes no sense to expose CONFIG_DMA_NUMA_CMA if CONFIG_NUMA is not
> enabled, and random config options shouldn't be default unless there
> is a good reason.  Replace the default NUMA with a depends on to fix both
> issues.

I think the "good reason" is that folks who do have NUMA systems are 
likely to be more interested in getting more consistent DMA performance 
than losing a bit more non-movable memory capacity, and the runtime 
impact on non-NUMA systems should be negligible, so I would definitely 
be inclined to make it "default y". But it's not critically important.

For the fundamental dependency change which absolutely makes sense,

Reviewed-by: Robin Murphy <roin.murphy@arm.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   kernel/dma/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
> index 4c1e9a3c0ab6f0..f488997b071712 100644
> --- a/kernel/dma/Kconfig
> +++ b/kernel/dma/Kconfig
> @@ -160,7 +160,7 @@ if  DMA_CMA
>   
>   config DMA_NUMA_CMA
>   	bool "Enable separate DMA Contiguous Memory Area for NUMA Node"
> -	default NUMA
> +	depends on NUMA
>   	help
>   	  Enable this option to get numa CMA areas so that NUMA devices
>   	  can get local memory by DMA coherent APIs.

Re: [PATCH] dma-contiguous: fix the Kconfig entry for CONFIG_DMA_NUMA_CMA

[Christoph Hellwig]

On Wed, Aug 30, 2023 at 11:11:29AM +0100, Robin Murphy wrote:
> I think the "good reason" is that folks who do have NUMA systems are likely 
> to be more interested in getting more consistent DMA performance than 
> losing a bit more non-movable memory capacity, and the runtime impact on 
> non-NUMA systems should be negligible, so I would definitely be inclined to 
> make it "default y". But it's not critically important.

Well, Linus hates these default y, and usually for a good reason.
If it's that useful/importanst for numa systems we should just drop
the option and make the code conditional on CONFIG_NUMA only.

Re: [PATCH] dma-contiguous: fix the Kconfig entry for CONFIG_DMA_NUMA_CMA

[Robin Murphy]

On 2023-08-30 12:54, Christoph Hellwig wrote:
> On Wed, Aug 30, 2023 at 11:11:29AM +0100, Robin Murphy wrote:
>> I think the "good reason" is that folks who do have NUMA systems are likely
>> to be more interested in getting more consistent DMA performance than
>> losing a bit more non-movable memory capacity, and the runtime impact on
>> non-NUMA systems should be negligible, so I would definitely be inclined to
>> make it "default y". But it's not critically important.
> 
> Well, Linus hates these default y, and usually for a good reason.
> If it's that useful/importanst for numa systems we should just drop
> the option and make the code conditional on CONFIG_NUMA only.

Hmm, good point. In fact, after double-checking, the amount of code 
saved by disabling it looks pretty trivial, and it does need an explicit 
command-line option to activate the "special" behaviour anyway, so 
indeed I'm now starting to doubt whether the separate config option is 
worthwhile at all.

Cheers,
Robin.