[PATCH] dma-debug: Kconfig for PREALLOC_DMA_DEBUG_ENTRIES

[PATCH] dma-debug: Kconfig for PREALLOC_DMA_DEBUG_ENTRIES

[Qian Cai]

The amount of DMA mappings from Hisilicon HNS ethernet devices is huge,
so it could trigger "DMA-API: debugging out of memory - disabling".

hnae_get_handle [1]
  hnae_init_queue
    hnae_init_ring
      hnae_alloc_buffers [2]
        debug_dma_map_page
          dma_entry_alloc

[1] for (i = 0; i < handle->q_num; i++)
[2] for (i = 0; i < ring->desc_num; i++)

On this Huawei TaiShan 2280 aarch64 server, it has reached the limit
already,

4 (ports) x 16 (handles) x 1024 (rings) = 65536

Added a Kconfig entry for PREALLOC_DMA_DEBUG_ENTRIES, so make it easier
for users to deal with special cases like this.

Signed-off-by: Qian Cai <cai@gmx.us>
---
 kernel/dma/debug.c | 9 ++-------
 lib/Kconfig.debug  | 9 +++++++++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 231ca4628062..3752fb23f72f 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -41,11 +41,6 @@
 #define HASH_FN_SHIFT   13
 #define HASH_FN_MASK    (HASH_SIZE - 1)
 
-/* allow architectures to override this if absolutely required */
-#ifndef PREALLOC_DMA_DEBUG_ENTRIES
-#define PREALLOC_DMA_DEBUG_ENTRIES (1 << 16)
-#endif
-
 enum {
 	dma_debug_single,
 	dma_debug_page,
@@ -132,7 +127,7 @@ static u32 min_free_entries;
 static u32 nr_total_entries;
 
 /* number of preallocated entries requested by kernel cmdline */
-static u32 nr_prealloc_entries = PREALLOC_DMA_DEBUG_ENTRIES;
+static u32 nr_prealloc_entries = CONFIG_PREALLOC_DMA_DEBUG_ENTRIES;
 
 /* debugfs dentry's for the stuff above */
 static struct dentry *dma_debug_dent        __read_mostly;
@@ -1063,7 +1058,7 @@ static __init int dma_debug_entries_cmdline(char *str)
 	if (!str)
 		return -EINVAL;
 	if (!get_option(&str, &nr_prealloc_entries))
-		nr_prealloc_entries = PREALLOC_DMA_DEBUG_ENTRIES;
+		nr_prealloc_entries = CONFIG_PREALLOC_DMA_DEBUG_ENTRIES;
 	return 0;
 }
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 1af29b8224fd..2c281edcb5ad 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1659,6 +1659,15 @@ config DMA_API_DEBUG
 
 	  If unsure, say N.
 
+config PREALLOC_DMA_DEBUG_ENTRIES
+	int "Preallocated DMA-API debugging entries"
+	depends on DMA_API_DEBUG
+	default 65536
+	help
+	  The number of preallocated entries for DMA-API debugging code. One
+	  entry is required per DMA-API allocation. Increase this if the DMA-API
+	  debugging code disables itself because the default is too low.
+
 config DMA_API_DEBUG_SG
 	bool "Debug DMA scatter-gather usage"
 	default y
-- 
2.17.2 (Apple Git-113)

Re: [PATCH] dma-debug: Kconfig for PREALLOC_DMA_DEBUG_ENTRIES

[Robin Murphy]

On 30/11/2018 17:54, Qian Cai wrote:
> The amount of DMA mappings from Hisilicon HNS ethernet devices is huge,
> so it could trigger "DMA-API: debugging out of memory - disabling".
> 
> hnae_get_handle [1]
>    hnae_init_queue
>      hnae_init_ring
>        hnae_alloc_buffers [2]
>          debug_dma_map_page
>            dma_entry_alloc
> 
> [1] for (i = 0; i < handle->q_num; i++)
> [2] for (i = 0; i < ring->desc_num; i++)
> 
> On this Huawei TaiShan 2280 aarch64 server, it has reached the limit
> already,
> 
> 4 (ports) x 16 (handles) x 1024 (rings) = 65536
> 
> Added a Kconfig entry for PREALLOC_DMA_DEBUG_ENTRIES, so make it easier
> for users to deal with special cases like this.
> 
> Signed-off-by: Qian Cai <cai@gmx.us>
> ---
>   kernel/dma/debug.c | 9 ++-------
>   lib/Kconfig.debug  | 9 +++++++++
>   2 files changed, 11 insertions(+), 7 deletions(-)

Oh, right, the arch overrides actually got cleaned up already. I'd 
forgotten that...

> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 231ca4628062..3752fb23f72f 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -41,11 +41,6 @@
>   #define HASH_FN_SHIFT   13
>   #define HASH_FN_MASK    (HASH_SIZE - 1)
>   
> -/* allow architectures to override this if absolutely required */
> -#ifndef PREALLOC_DMA_DEBUG_ENTRIES
> -#define PREALLOC_DMA_DEBUG_ENTRIES (1 << 16)
> -#endif
> -
>   enum {
>   	dma_debug_single,
>   	dma_debug_page,
> @@ -132,7 +127,7 @@ static u32 min_free_entries;
>   static u32 nr_total_entries;
>   
>   /* number of preallocated entries requested by kernel cmdline */
> -static u32 nr_prealloc_entries = PREALLOC_DMA_DEBUG_ENTRIES;
> +static u32 nr_prealloc_entries = CONFIG_PREALLOC_DMA_DEBUG_ENTRIES;
>   
>   /* debugfs dentry's for the stuff above */
>   static struct dentry *dma_debug_dent        __read_mostly;
> @@ -1063,7 +1058,7 @@ static __init int dma_debug_entries_cmdline(char *str)
>   	if (!str)
>   		return -EINVAL;
>   	if (!get_option(&str, &nr_prealloc_entries))
> -		nr_prealloc_entries = PREALLOC_DMA_DEBUG_ENTRIES;
> +		nr_prealloc_entries = CONFIG_PREALLOC_DMA_DEBUG_ENTRIES;
>   	return 0;
>   }
>   
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 1af29b8224fd..2c281edcb5ad 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1659,6 +1659,15 @@ config DMA_API_DEBUG
>   
>   	  If unsure, say N.
>   
> +config PREALLOC_DMA_DEBUG_ENTRIES
> +	int "Preallocated DMA-API debugging entries"
> +	depends on DMA_API_DEBUG
> +	default 65536

I was assuming the point was to also add something like

	default 131072 if HNS_ENET

so that DMA debug doesn't require too much thought from the user. If 
they still have to notice the overflow message and empirically figure 
out a value that does work, rebuilding the kernel each time is far less 
convenient than simply adding "dma_debug_entries=..." to their kernel 
command line and rebooting, which they can do today. If they do already 
know up-front that the default will need overriding and what the 
appropriate value is, then the command line still seems seems just as 
convenient.

Robin.

> +	help
> +	  The number of preallocated entries for DMA-API debugging code. One
> +	  entry is required per DMA-API allocation. Increase this if the DMA-API
> +	  debugging code disables itself because the default is too low.
> +
>   config DMA_API_DEBUG_SG
>   	bool "Debug DMA scatter-gather usage"
>   	default y
> 

Re: [PATCH] dma-debug: Kconfig for PREALLOC_DMA_DEBUG_ENTRIES

[Christoph Hellwig]

On Fri, Nov 30, 2018 at 07:39:50PM +0000, Robin Murphy wrote:
> I was assuming the point was to also add something like
>
> 	default 131072 if HNS_ENET
>
> so that DMA debug doesn't require too much thought from the user. If they 
> still have to notice the overflow message and empirically figure out a 
> value that does work, rebuilding the kernel each time is far less 
> convenient than simply adding "dma_debug_entries=..." to their kernel 
> command line and rebooting, which they can do today. If they do already 
> know up-front that the default will need overriding and what the 
> appropriate value is, then the command line still seems seems just as 
> convenient.

I'm not so fond of random drivers changing the defaults.  My idea
was rather to have the config option so that the defconfig files for
the Hisilicon SOCs with this hardware could select a larger number
without making a total mess of the kernel configuration.

If we really have to we could do different defaults, but I'd still
much rather do this on a arch/platform basis than specific drivers.

Re: [PATCH] dma-debug: Kconfig for PREALLOC_DMA_DEBUG_ENTRIES

[John Garry]

On 01/12/2018 16:36, Christoph Hellwig wrote:
> On Fri, Nov 30, 2018 at 07:39:50PM +0000, Robin Murphy wrote:
>> I was assuming the point was to also add something like
>>
>> 	default 131072 if HNS_ENET
>>
>> so that DMA debug doesn't require too much thought from the user. If they
>> still have to notice the overflow message and empirically figure out a
>> value that does work, rebuilding the kernel each time is far less
>> convenient than simply adding "dma_debug_entries=..." to their kernel
>> command line and rebooting, which they can do today. If they do already
>> know up-front that the default will need overriding and what the
>> appropriate value is, then the command line still seems seems just as
>> convenient.
>
> I'm not so fond of random drivers changing the defaults.  My idea
> was rather to have the config option so that the defconfig files for
> the Hisilicon SOCs with this hardware could select a larger number
> without making a total mess of the kernel configuration.
>
> If we really have to we could do different defaults, but I'd still
> much rather do this on a arch/platform basis than specific drivers.

As I understand, some drivers could even use much more than this 
(131072), to such a point where I can't imagine that we would want to 
set an arch default to support them. For this HNS_ENET case, it is arm64 
specific so it would be an arch defconfig.

Thanks,
John

> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
> .
>

Re: [PATCH] dma-debug: Kconfig for PREALLOC_DMA_DEBUG_ENTRIES

[Christoph Hellwig]

On Mon, Dec 03, 2018 at 11:56:11AM +0000, John Garry wrote:
> On 01/12/2018 16:36, Christoph Hellwig wrote:
>> On Fri, Nov 30, 2018 at 07:39:50PM +0000, Robin Murphy wrote:
>>> I was assuming the point was to also add something like
>>>
>>> 	default 131072 if HNS_ENET
>>>
>>> so that DMA debug doesn't require too much thought from the user. If they
>>> still have to notice the overflow message and empirically figure out a
>>> value that does work, rebuilding the kernel each time is far less
>>> convenient than simply adding "dma_debug_entries=..." to their kernel
>>> command line and rebooting, which they can do today. If they do already
>>> know up-front that the default will need overriding and what the
>>> appropriate value is, then the command line still seems seems just as
>>> convenient.
>>
>> I'm not so fond of random drivers changing the defaults.  My idea
>> was rather to have the config option so that the defconfig files for
>> the Hisilicon SOCs with this hardware could select a larger number
>> without making a total mess of the kernel configuration.
>>
>> If we really have to we could do different defaults, but I'd still
>> much rather do this on a arch/platform basis than specific drivers.
>
> As I understand, some drivers could even use much more than this (131072), 
> to such a point where I can't imagine that we would want to set an arch 
> default to support them. For this HNS_ENET case, it is arm64 specific so it 
> would be an arch defconfig.

But I'm not sure we could always do the right thing for everyone.

I think we might be better of trying to just dynamically allocate
entries when we run out of them instead of coming up with a perfect
number.