Messages in this thread Patch in this message | | | Date | Fri, 16 Feb 2024 08:47:49 +0100 | From | Takashi Iwai <> | Subject | Re: [PATCH v1] ALSA: memalloc: Fix indefinite hang in non-iommu case |
| |
On Thu, 15 Feb 2024 20:32:22 +0100, Karthikeyan Ramasubramanian wrote: > > On Thu, Feb 15, 2024 at 10:03 AM Takashi Iwai <tiwai@suse.de> wrote: > > > > On Thu, 15 Feb 2024 17:07:38 +0100, > > Sven van Ashbrook wrote: > > > > > > Hi Takashi, > > > > > > On Thu, Feb 15, 2024 at 3:40 AM Takashi Iwai <tiwai@suse.de> wrote: > > > > > > > > Yes, the main problem is the indefinite hang from > > > > dma_alloc_noncontiguous(). > > > > > > We have a publicly-visible test [1] which readily triggers the > > > indefinite hang on non-iommu Intel SoCs such as JasperLake. > > > As noted in the commit message, iommu SoCs are not currently > > > affected. > > > > > > > So, is the behavior more or less same even if you pass > > > > __GFP_RETRY_MAYFAIL to dma_alloc_noncontiguous()? Or is this flag > > > > already implicitly set somewhere in the middle? It shouldn't hang > > > > indefinitely, but the other impact to the system like OOM-killer > > > > kickoff may be seen. > > > > > > My incomplete understanding: > > > > > > Alsa specifies __GFP_RETRY_MAYFAIL because it wants to prevent triggering > > > the OOM killer. > > > > Ah I forgot that we set that bit commonly in the flag. > > > > > This was __GFP_NORETRY in the not-too-distant past [2], > > > but that failed too quickly, which resulted in permanent loss of audio due > > > to failed firmware dma sg allocations. > > > > Hm, the change in commit a61c7d88d38c assumed that __GFP_RETRY_MAYFAIL > > shouldn't have that big impact. If the hang really happens with a > > high order allocation, it's dangerous to use it in other code > > allocations than noncontiguous case (i.e. SNDRV_DMA_TYPE_DEV and co). > > In the tight memory situation, a similar problem can be triggered > > quite easily, then. > > > > > In the iommu case, dma_alloc_noncontiguous() implements a backoff [3] loop > > > which ORs in __GFP_NORETRY except for minimum order allocations. We observe > > > experimentally that __GFP_RETRY_MAYFAIL does not get "stuck" on minimum order > > > allocations. So the iommu case is not affected. > > > > > > In the non-iommu case however, dma_alloc_noncontiguous() actually becomes a > > > contiguous allocator, with no backoff loop. The commit introducing it [4] > > > states "This API is only properly implemented for dma-iommu and will simply > > > return a contigious chunk as a fallback." In this case we observe the indefinite > > > hang. > > > > > > The alsa fallback allocator is also not affected by the problem, as it does > > > not specify __GFP_RETRY_MAYFAIL. Except in the XENPV case. > > > > So it sounds like that we should go back for __GFP_NORETRY in general > > for non-zero order allocations, not only the call you changed, as > > __GFP_RETRY_MAYFAIL doesn't guarantee the stuck. > > > > How about the changes like below? > > We are concerned that the below proposed change might break the fix > introduced by commit a61c7d88d38c ("FROMGIT: ALSA: memalloc: use > __GFP_RETRY_MAYFAIL for DMA mem allocs"). More specifically in the > IOMMU case with a large allocation, the fallback algorithm in the DMA > IOMMU allocator[1] will not try really hard and hence may fail > prematurely when it gets to minimum order allocations. This will > result in no audio when there is significant load on physical memory.
Hm, then we may give __GFP_RETRY_MAYFAIL specially for iommu case, too? Something like v2 patch below.
thanks,
Takashi
--- a/sound/core/memalloc.c +++ b/sound/core/memalloc.c @@ -21,7 +21,11 @@ #define DEFAULT_GFP \ (GFP_KERNEL | \ - __GFP_RETRY_MAYFAIL | /* don't trigger OOM-killer */ \ + __GFP_NORETRY | /* don't trigger OOM-killer */ \ + __GFP_NOWARN) /* no stack trace print - this call is non-critical */ +#define DEFAULT_GFP_RETRY \ + (GFP_KERNEL | \ + __GFP_RETRY_MAYFAIL | /* try a bit harder */ \ __GFP_NOWARN) /* no stack trace print - this call is non-critical */ static const struct snd_malloc_ops *snd_dma_get_ops(struct snd_dma_buffer *dmab); @@ -30,6 +34,13 @@ static const struct snd_malloc_ops *snd_dma_get_ops(struct snd_dma_buffer *dmab) static void *snd_dma_sg_fallback_alloc(struct snd_dma_buffer *dmab, size_t size); #endif +/* default GFP bits for our allocations */ +static gfp_t default_gfp(size_t size) +{ + /* don't allocate intensively for high-order pages */ + return (size > PAGE_SIZE) ? DEFAULT_GFP : DEFAULT_GFP_RETRY; +} + static void *__snd_dma_alloc_pages(struct snd_dma_buffer *dmab, size_t size) { const struct snd_malloc_ops *ops = snd_dma_get_ops(dmab); @@ -281,7 +292,7 @@ static void *do_alloc_pages(struct device *dev, size_t size, dma_addr_t *addr, bool wc) { void *p; - gfp_t gfp = GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN; + gfp_t gfp = default_gfp(size); again: p = alloc_pages_exact(size, gfp); @@ -466,7 +477,7 @@ static const struct snd_malloc_ops snd_dma_iram_ops = { */ static void *snd_dma_dev_alloc(struct snd_dma_buffer *dmab, size_t size) { - return dma_alloc_coherent(dmab->dev.dev, size, &dmab->addr, DEFAULT_GFP); + return dma_alloc_coherent(dmab->dev.dev, size, &dmab->addr, default_gfp(size)); } static void snd_dma_dev_free(struct snd_dma_buffer *dmab) @@ -511,7 +522,7 @@ static int snd_dma_wc_mmap(struct snd_dma_buffer *dmab, #else static void *snd_dma_wc_alloc(struct snd_dma_buffer *dmab, size_t size) { - return dma_alloc_wc(dmab->dev.dev, size, &dmab->addr, DEFAULT_GFP); + return dma_alloc_wc(dmab->dev.dev, size, &dmab->addr, default_gfp(size)); } static void snd_dma_wc_free(struct snd_dma_buffer *dmab) @@ -539,14 +550,20 @@ static const struct snd_malloc_ops snd_dma_wc_ops = { static void *snd_dma_noncontig_alloc(struct snd_dma_buffer *dmab, size_t size) { struct sg_table *sgt; + gfp_t gfp = default_gfp(size); void *p; #ifdef CONFIG_SND_DMA_SGBUF if (cpu_feature_enabled(X86_FEATURE_XENPV)) return snd_dma_sg_fallback_alloc(dmab, size); + /* dma_alloc_noncontiguous() with IOMMU is safe to pass + * __GFP_RETRY_MAYFAIL option for more intensive allocations + */ + if (get_dma_ops(dmab->dev.dev)) + gfp = DEFAULT_GFP_RETRY; #endif sgt = dma_alloc_noncontiguous(dmab->dev.dev, size, dmab->dev.dir, - DEFAULT_GFP, 0); + gfp, 0); #ifdef CONFIG_SND_DMA_SGBUF if (!sgt && !get_dma_ops(dmab->dev.dev)) return snd_dma_sg_fallback_alloc(dmab, size); @@ -783,7 +800,7 @@ static void *snd_dma_sg_fallback_alloc(struct snd_dma_buffer *dmab, size_t size) while (size > 0) { chunk = min(size, chunk); if (sgbuf->use_dma_alloc_coherent) - p = dma_alloc_coherent(dmab->dev.dev, chunk, &addr, DEFAULT_GFP); + p = dma_alloc_coherent(dmab->dev.dev, chunk, &addr, default_gfp(size)); else p = do_alloc_pages(dmab->dev.dev, chunk, &addr, false); if (!p) { @@ -871,7 +888,7 @@ static void *snd_dma_noncoherent_alloc(struct snd_dma_buffer *dmab, size_t size) void *p; p = dma_alloc_noncoherent(dmab->dev.dev, size, &dmab->addr, - dmab->dev.dir, DEFAULT_GFP); + dmab->dev.dir, default_gfp(size)); if (p) dmab->dev.need_sync = dma_need_sync(dmab->dev.dev, dmab->addr); return p;
| |