[PATCH 0/3] Fix SND_HDA_PREALLOC issue

[PATCH 0/3] Fix SND_HDA_PREALLOC issue

[Amadeusz Sławiński]

For context it started with user reporting failures when running arecord
without any error or warning in dmesg (after fixing some configuration
problems thet they had).
https://bugzilla.kernel.org/show_bug.cgi?id=201251#c279

After spending time investigating the issue it was narrowed to quite big
setting of CONFIG_SND_HDA_PREALLOC_SIZE (4096).
When looking at code
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/core/pcm_memory.c?id=6417f03132a6952cd17ddd8eaddbac92b61b17e0#n30
there is a limit of memory per card:
max_alloc_per_card = 32UL * 1024UL * 1024UL

When SND_HDA_PREALLOC_SIZE is set to 4096 it only has memory to alloc
for 8 frontends, while Skylake HDA card has 10 of them (6 playback and 4
capture), so preallocated memory is exhausted while probing. In
consequence 2 of FEs end without allocated memory.

It can be workarounded on user side with setting SND_HDA_PREALLOC_SIZE
to lower value, other is changing memory limit per card.

However in order to not waste user memory, change maximum allocation
size on HDA controller to 4MB and force automatical memory allocation
insted of preallocated one.

First patch adds prints, so similar issues can be easily identified in
the future.
Second changes maximum size of hda buffer to reasonable value of 4MB.
And last one reverts patch which allowed setting prealloc size on X86
platforms.

Amadeusz Sławiński (3):
  ALSA: pcm: Add debug print on memory allocation failure
  ALSA: hda: Change AZX_MAX_BUF_SIZE from 1GB to 4MB
  ALSA: hda: Revert "ALSA: hda: Allow setting preallocation again for
    x86"

 include/sound/hda_register.h | 4 ++--
 sound/core/pcm_memory.c      | 8 ++++++++
 sound/hda/Kconfig            | 7 +++----
 3 files changed, 13 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/3] ALSA: pcm: Add debug print on memory allocation failure

[Amadeusz Sławiński]

Add debug prints after calls of do_alloc_pages. One simplification would
be to move print into do_alloc_pages, however it would cause spam in
logs, as preallocate_pcm_pages loops over do_alloc_pages trying lower
values in case of failures.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/core/pcm_memory.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c
index 289dd1fd8fe7..2878c4c23583 100644
--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -176,6 +176,10 @@ static void snd_pcm_lib_preallocate_proc_write(struct snd_info_entry *entry,
 					   substream->dma_buffer.dev.dev,
 					   size, &new_dmab) < 0) {
 				buffer->error = -ENOMEM;
+				pr_debug("ALSA pcmC%dD%d%c,%d:%s: cannot preallocate for size %zu\n",
+					 substream->pcm->card->number, substream->pcm->device,
+					 substream->stream ? 'c' : 'p', substream->number,
+					 substream->pcm->name, size);
 				return;
 			}
 			substream->buffer_bytes_max = size;
@@ -400,6 +404,10 @@ int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size)
 				   substream->dma_buffer.dev.dev,
 				   size, dmab) < 0) {
 			kfree(dmab);
+			pr_debug("ALSA pcmC%dD%d%c,%d:%s: cannot preallocate for size %zu\n",
+				 substream->pcm->card->number, substream->pcm->device,
+				 substream->stream ? 'c' : 'p', substream->number,
+				 substream->pcm->name, size);
 			return -ENOMEM;
 		}
 	}
-- 
2.25.1

[PATCH 2/3] ALSA: hda: Change AZX_MAX_BUF_SIZE from 1GB to 4MB

[Amadeusz Sławiński]

When SND_HDA_PREALLOC_SIZE is set to 0, applications can request as much
memory as there is allowed. With value of AZX_MAX_BUF_SIZE it is 1GB per
stream, which is not realistic use case. Change it 4MB.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=201251#c322
Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 include/sound/hda_register.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
index 4f987b1f32f7..68d420f3c49d 100644
--- a/include/sound/hda_register.h
+++ b/include/sound/hda_register.h
@@ -140,8 +140,8 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
 #define BDL_SIZE		4096
 #define AZX_MAX_BDL_ENTRIES	(BDL_SIZE / 16)
 #define AZX_MAX_FRAG		32
-/* max buffer size - no h/w limit, you can increase as you like */
-#define AZX_MAX_BUF_SIZE	(1024*1024*1024)
+/* max buffer size - 4MB limit per stream */
+#define AZX_MAX_BUF_SIZE	(4*1024*1024)
 
 /* RIRB int mask: overrun[2], response[0] */
 #define RIRB_INT_RESPONSE	0x01
-- 
2.25.1

Re: [PATCH 2/3] ALSA: hda: Change AZX_MAX_BUF_SIZE from 1GB to 4MB

[Takashi Iwai]

On Thu, 18 Mar 2021 16:11:21 +0100,
Amadeusz Sławiński wrote:
> 
> When SND_HDA_PREALLOC_SIZE is set to 0, applications can request as much
> memory as there is allowed. With value of AZX_MAX_BUF_SIZE it is 1GB per
> stream, which is not realistic use case. Change it 4MB.
> 
> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=201251#c322
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> ---
>  include/sound/hda_register.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
> index 4f987b1f32f7..68d420f3c49d 100644
> --- a/include/sound/hda_register.h
> +++ b/include/sound/hda_register.h
> @@ -140,8 +140,8 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
>  #define BDL_SIZE		4096
>  #define AZX_MAX_BDL_ENTRIES	(BDL_SIZE / 16)
>  #define AZX_MAX_FRAG		32
> -/* max buffer size - no h/w limit, you can increase as you like */
> -#define AZX_MAX_BUF_SIZE	(1024*1024*1024)
> +/* max buffer size - 4MB limit per stream */
> +#define AZX_MAX_BUF_SIZE	(4*1024*1024)

This deserves for a comment that it's an artificial limit just for
avoiding a pitfall.


thanks,

Takashi

[PATCH 3/3] ALSA: hda: Revert "ALSA: hda: Allow setting preallocation again for x86"

[Amadeusz Sławiński]

This reverts commit f8e4ae10de43fbb7ce85f79e04eca2988b6b2c40.

On systems where there is a lot of FrontEnds, when
CONFIG_SND_HDA_PREALLOC_SIZE != 0  ALSA core allocates memory for each
FE, which may cause out of memory problems due to per card limit. Force
config to 0 on X86, so memory will be allocated on as needed basis.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=201251#c322
Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/hda/Kconfig | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig
index 57595f1552c9..741179ccbd4e 100644
--- a/sound/hda/Kconfig
+++ b/sound/hda/Kconfig
@@ -21,17 +21,16 @@ config SND_HDA_EXT_CORE
        select SND_HDA_CORE
 
 config SND_HDA_PREALLOC_SIZE
-	int "Pre-allocated buffer size for HD-audio driver"
+	int "Pre-allocated buffer size for HD-audio driver" if !SND_DMA_SGBUF
 	range 0 32768
-	default 2048 if SND_DMA_SGBUF
+	default 0 if SND_DMA_SGBUF
 	default 64 if !SND_DMA_SGBUF
 	help
 	  Specifies the default pre-allocated buffer-size in kB for the
 	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
 	  for systems using PulseAudio.  The default 64 is chosen just
 	  for compatibility reasons.
-	  On x86 systems, the default is 2048 as a reasonable value for
-	  most of modern systems.
+	  On x86 systems, the default is zero as we need no preallocation.
 
 	  Note that the pre-allocation size can be changed dynamically
 	  via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too.
-- 
2.25.1

Re: [PATCH 3/3] ALSA: hda: Revert "ALSA: hda: Allow setting preallocation again for x86"

[Takashi Iwai]

On Thu, 18 Mar 2021 16:11:22 +0100,
Amadeusz Sławiński wrote:
> 
> This reverts commit f8e4ae10de43fbb7ce85f79e04eca2988b6b2c40.

It should be a format of commit ("description ...") as checkpatch.pl
would complain.  The commit f8e4ae10de43 itself is a revert of commit
c31427d0d21e, and you need to give an explanation why the
revert-of-a-revert can work better at this time.


thanks,

Takashi

> On systems where there is a lot of FrontEnds, when
> CONFIG_SND_HDA_PREALLOC_SIZE != 0  ALSA core allocates memory for each
> FE, which may cause out of memory problems due to per card limit. Force
> config to 0 on X86, so memory will be allocated on as needed basis.
> 
> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=201251#c322
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> ---
>  sound/hda/Kconfig | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig
> index 57595f1552c9..741179ccbd4e 100644
> --- a/sound/hda/Kconfig
> +++ b/sound/hda/Kconfig
> @@ -21,17 +21,16 @@ config SND_HDA_EXT_CORE
>         select SND_HDA_CORE
>  
>  config SND_HDA_PREALLOC_SIZE
> -	int "Pre-allocated buffer size for HD-audio driver"
> +	int "Pre-allocated buffer size for HD-audio driver" if !SND_DMA_SGBUF
>  	range 0 32768
> -	default 2048 if SND_DMA_SGBUF
> +	default 0 if SND_DMA_SGBUF
>  	default 64 if !SND_DMA_SGBUF
>  	help
>  	  Specifies the default pre-allocated buffer-size in kB for the
>  	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
>  	  for systems using PulseAudio.  The default 64 is chosen just
>  	  for compatibility reasons.
> -	  On x86 systems, the default is 2048 as a reasonable value for
> -	  most of modern systems.
> +	  On x86 systems, the default is zero as we need no preallocation.
>  
>  	  Note that the pre-allocation size can be changed dynamically
>  	  via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too.
> -- 
> 2.25.1
> 

Re: [PATCH 3/3] ALSA: hda: Revert "ALSA: hda: Allow setting preallocation again for x86"

[Amadeusz Sławiński]

On 3/18/2021 4:32 PM, Takashi Iwai wrote:
> On Thu, 18 Mar 2021 16:11:22 +0100,
> Amadeusz Sławiński wrote:
>>
>> This reverts commit f8e4ae10de43fbb7ce85f79e04eca2988b6b2c40.
> 
> It should be a format of commit ("description ...") as checkpatch.pl
> would complain.  The commit f8e4ae10de43 itself is a revert of commit
> c31427d0d21e, and you need to give an explanation why the
> revert-of-a-revert can work better at this time.
> 

I don't mind improving it, however it seems like there is exception done 
in checkpatch for "This reverts commit", so there is no warning on this 
line.

Will add explanation.

> 
> thanks,
> 
> Takashi
> 
>> On systems where there is a lot of FrontEnds, when
>> CONFIG_SND_HDA_PREALLOC_SIZE != 0  ALSA core allocates memory for each
>> FE, which may cause out of memory problems due to per card limit. Force
>> config to 0 on X86, so memory will be allocated on as needed basis.
>>
>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=201251#c322
>> Suggested-by: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>> ---
>>   sound/hda/Kconfig | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig
>> index 57595f1552c9..741179ccbd4e 100644
>> --- a/sound/hda/Kconfig
>> +++ b/sound/hda/Kconfig
>> @@ -21,17 +21,16 @@ config SND_HDA_EXT_CORE
>>          select SND_HDA_CORE
>>   
>>   config SND_HDA_PREALLOC_SIZE
>> -	int "Pre-allocated buffer size for HD-audio driver"
>> +	int "Pre-allocated buffer size for HD-audio driver" if !SND_DMA_SGBUF
>>   	range 0 32768
>> -	default 2048 if SND_DMA_SGBUF
>> +	default 0 if SND_DMA_SGBUF
>>   	default 64 if !SND_DMA_SGBUF
>>   	help
>>   	  Specifies the default pre-allocated buffer-size in kB for the
>>   	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
>>   	  for systems using PulseAudio.  The default 64 is chosen just
>>   	  for compatibility reasons.
>> -	  On x86 systems, the default is 2048 as a reasonable value for
>> -	  most of modern systems.
>> +	  On x86 systems, the default is zero as we need no preallocation.
>>   
>>   	  Note that the pre-allocation size can be changed dynamically
>>   	  via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too.
>> -- 
>> 2.25.1
>>

Re: [PATCH 0/3] Fix SND_HDA_PREALLOC issue

[Cezary Rojewski]

On 2021-03-18 4:11 PM, Amadeusz Sławiński wrote:
> For context it started with user reporting failures when running arecord
> without any error or warning in dmesg (after fixing some configuration
> problems thet they had).
> https://bugzilla.kernel.org/show_bug.cgi?id=201251#c279
> 
> After spending time investigating the issue it was narrowed to quite big
> setting of CONFIG_SND_HDA_PREALLOC_SIZE (4096).
> When looking at code
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/core/pcm_memory.c?id=6417f03132a6952cd17ddd8eaddbac92b61b17e0#n30
> there is a limit of memory per card:
> max_alloc_per_card = 32UL * 1024UL * 1024UL
> 
> When SND_HDA_PREALLOC_SIZE is set to 4096 it only has memory to alloc
> for 8 frontends, while Skylake HDA card has 10 of them (6 playback and 4
> capture), so preallocated memory is exhausted while probing. In
> consequence 2 of FEs end without allocated memory.
> 
> It can be workarounded on user side with setting SND_HDA_PREALLOC_SIZE
> to lower value, other is changing memory limit per card.
> 
> However in order to not waste user memory, change maximum allocation
> size on HDA controller to 4MB and force automatical memory allocation
> insted of preallocated one.
> 
> First patch adds prints, so similar issues can be easily identified in
> the future.
> Second changes maximum size of hda buffer to reasonable value of 4MB.
> And last one reverts patch which allowed setting prealloc size on X86
> platforms.
> 
> Amadeusz Sławiński (3):
>    ALSA: pcm: Add debug print on memory allocation failure
>    ALSA: hda: Change AZX_MAX_BUF_SIZE from 1GB to 4MB
>    ALSA: hda: Revert "ALSA: hda: Allow setting preallocation again for
>      x86"
> 
>   include/sound/hda_register.h | 4 ++--
>   sound/core/pcm_memory.c      | 8 ++++++++
>   sound/hda/Kconfig            | 7 +++----
>   3 files changed, 13 insertions(+), 6 deletions(-)
> 

For the series:
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>

Nice work Amadeo.

Regards,
Czarek