On Mon, 26 Jun 2023 13:13:21 +0200, Takashi Iwai wrote:
On Mon, 26 Jun 2023 13:09:00 +0200, Jaroslav Kysela wrote:
On 26. 06. 23 13:02, Takashi Iwai wrote:
On Mon, 26 Jun 2023 09:56:47 +0200, Jaroslav Kysela wrote:
On 26. 06. 23 9:33, Takashi Iwai wrote:
On Mon, 26 Jun 2023 09:31:18 +0200, Tuo Li wrote:
Hello,
Thank you for your reply!
FWIW, the simplest fix would be something like below, just extending the mutex coverage. But it'll serialize the all calls, so it might influence on the performance, while it's the safest way.
It may be better to update total_pcm_alloc_bytes before snd_dma_alloc_dir_pages() call and decrease this value when allocation fails to allow parallel allocations. Then the mutex can be held only for the total_pcm_alloc_bytes variable update.
Yes, it'd work. But a tricky part is that the actual allocation size can be bigger, and we need to correct the total_pcm_alloc_bytes after the allocation result. So the end result would be a patch like below, which is a bit more complex than the previous simpler approach. But it might be OK.
The patch looks good, but it may be better to move the "post" variable updates to an inline function (mutex lock - update - mutex unlock) for a better readability.
Sounds like a good idea. Let me cook later.
... and here it is.
If that looks OK, I'll submit a proper fix patch.
thanks,
Takashi
--- a/sound/core/pcm_memory.c +++ b/sound/core/pcm_memory.c @@ -31,15 +31,41 @@ static unsigned long max_alloc_per_card = 32UL * 1024UL * 1024UL; module_param(max_alloc_per_card, ulong, 0644); MODULE_PARM_DESC(max_alloc_per_card, "Max total allocation bytes per card.");
+static void __update_allocated_size(struct snd_card *card, ssize_t bytes) +{ + card->total_pcm_alloc_bytes += bytes; +} + +static void update_allocated_size(struct snd_card *card, ssize_t bytes) +{ + mutex_lock(&card->memory_mutex); + __update_allocated_size(card, bytes); + mutex_unlock(&card->memory_mutex); +} + +static void decrease_allocated_size(struct snd_card *card, size_t bytes) +{ + mutex_lock(&card->memory_mutex); + WARN_ON(card->total_pcm_alloc_bytes < bytes); + __update_allocated_size(card, -(ssize_t)bytes); + mutex_unlock(&card->memory_mutex); +} + static int do_alloc_pages(struct snd_card *card, int type, struct device *dev, int str, size_t size, struct snd_dma_buffer *dmab) { enum dma_data_direction dir; int err;
+ /* check and reserve the requested size */ + mutex_lock(&card->memory_mutex); if (max_alloc_per_card && - card->total_pcm_alloc_bytes + size > max_alloc_per_card) + card->total_pcm_alloc_bytes + size > max_alloc_per_card) { + mutex_unlock(&card->memory_mutex); return -ENOMEM; + } + __update_allocated_size(card, size); + mutex_unlock(&card->memory_mutex);
if (str == SNDRV_PCM_STREAM_PLAYBACK) dir = DMA_TO_DEVICE; @@ -47,9 +73,14 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev, dir = DMA_FROM_DEVICE; err = snd_dma_alloc_dir_pages(type, dev, dir, size, dmab); if (!err) { - mutex_lock(&card->memory_mutex); - card->total_pcm_alloc_bytes += dmab->bytes; - mutex_unlock(&card->memory_mutex); + /* the actual allocation size might be bigger than requested, + * and we need to correct the account + */ + if (dmab->bytes != size) + update_allocated_size(card, dmab->bytes - size); + } else { + /* take back on allocation failure */ + decrease_allocated_size(card, size); } return err; } @@ -58,10 +89,7 @@ static void do_free_pages(struct snd_card *card, struct snd_dma_buffer *dmab) { if (!dmab->area) return; - mutex_lock(&card->memory_mutex); - WARN_ON(card->total_pcm_alloc_bytes < dmab->bytes); - card->total_pcm_alloc_bytes -= dmab->bytes; - mutex_unlock(&card->memory_mutex); + decrease_allocated_size(card, dmab->bytes); snd_dma_free_pages(dmab); dmab->area = NULL; }