Messages in this thread | | | Date | Mon, 26 Jun 2023 15:32:40 +0200 | From | Jaroslav Kysela <> | Subject | Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages() |
| |
On 26. 06. 23 15:15, Takashi Iwai wrote: > 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)
Missing inline ? May be also used for
> +static void update_allocated_size(struct snd_card *card, ssize_t bytes) > +static void decrease_allocated_size(struct snd_card *card, size_t bytes)
The rest is fine in my eyes.
Reviewed-by: Jaroslav Kysela <perex@perex.cz>
Thanks, Jaroslav
-- Jaroslav Kysela <perex@perex.cz> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
| |