Received-SPF: pass (google.com: domain of linux-kernel+bounces-71226-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <235ab5aa-90a4-4dd7-b2c6-70469605bcfb@suse.cz>
Date: Mon, 19 Feb 2024 12:40:01 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Stall at page allocations with __GFP_RETRY_MAYFAIL (Re: [PATCH
 v1] ALSA: memalloc: Fix indefinite hang in non-iommu case)
Content-Language: en-US
To: Takashi Iwai <tiwai@suse.de>,
 Karthikeyan Ramasubramanian <kramasub@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
 Sven van Ashbrook <svenva@chromium.org>, Brian Geffon <bgeffon@google.com>,
 stable@vger.kernel.org, Curtis Malainey <cujomalainey@chromium.org>,
 Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
 linux-sound@vger.kernel.org, linux-mm@kvack.org
References: <20240214170720.v1.1.Ic3de2566a7fd3de8501b2f18afa9f94eadb2df0a@changeid>
 <87jzn0ofdb.wl-tiwai@suse.de>
From: Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <87jzn0ofdb.wl-tiwai@suse.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2/19/24 12:36, Takashi Iwai wrote:
> On Thu, 15 Feb 2024 01:07:25 +0100,
> Karthikeyan Ramasubramanian wrote:
>> 
>> Before 9d8e536 ("ALSA: memalloc: Try dma_alloc_noncontiguous() at first")
>> the alsa non-contiguous allocator always called the alsa fallback
>> allocator in the non-iommu case. This allocated non-contig memory
>> consisting of progressively smaller contiguous chunks. Allocation was
>> fast due to the OR-ing in of __GFP_NORETRY.
>> 
>> After 9d8e536 ("ALSA: memalloc: Try dma_alloc_noncontiguous() at first")
>> the code tries the dma non-contig allocator first, then falls back to
>> the alsa fallback allocator. In the non-iommu case, the former supports
>> only a single contiguous chunk.
>> 
>> We have observed experimentally that under heavy memory fragmentation,
>> allocating a large-ish contiguous chunk with __GFP_RETRY_MAYFAIL
>> triggers an indefinite hang in the dma non-contig allocator. This has
>> high-impact, as an occurrence will trigger a device reboot, resulting in
>> loss of user state.
>> 
>> Fix the non-iommu path by letting dma_alloc_noncontiguous() fail quickly
>> so it does not get stuck looking for that elusive large contiguous chunk,
>> in which case we will fall back to the alsa fallback allocator.
>> 
>> Note that the iommu dma non-contiguous allocator is not affected. While
>> assembling an array of pages, it tries consecutively smaller contiguous
>> allocations, and lets higher-order chunk allocations fail quickly.
>> 
>> Suggested-by: Sven van Ashbrook <svenva@chromium.org>
>> Suggested-by: Brian Geffon <bgeffon@google.com>
>> Fixes: 9d8e536d36e7 ("ALSA: memalloc: Try dma_alloc_noncontiguous() at first")
>> Cc: stable@vger.kernel.org
>> Cc: Sven van Ashbrook <svenva@chromium.org>
>> Cc: Brian Geffon <bgeffon@google.com>
>> Cc: Curtis Malainey <cujomalainey@chromium.org>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@chromium.org>
> 
> After chatting with Vlastimil, he recommended to get linux-mm people
> involved, as basically such a problem shouldn't happen in the page
> allocator side.  So let's widen the audience.
> 
> To recap the thread: the problem is that dma_alloc_contiguous() call
> with high order pages and __GFP_FAIL_MAYRETRY leads to indefinite
> stall.  (It was __GFP_NORETRY beforehand.)  This looks like the code
> path with the direct page allocation where no IOMMU is involved.
> 
> Karthikeyan, Sven, and co: could you guys show the stack trace at the
> stall?  This may give us more clear light.

Yeah, if the inifinite loop with __GFP_RETRY_MAYFAIL happens in a call to
__alloc_pages and not in some retry loop around it in an upper layer (I
tried to check the dma functions but got lost quickly so the exact call
stack would be useful), we definitely want to know the details. It shouldn't
happen for costly orders (>3) because the retries are hard limited for those
despite apparent progress or reclaim or compaction.

> Also, Vlastimil suggested that tracepoints would be helpful if that's
> really in the page allocator, too.
> 
> 
> Thanks!
> 
> Takashi
> 
>> ---
>> 
>>  sound/core/memalloc.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
>> index f901504b5afc1..5f6526a0d731c 100644
>> --- a/sound/core/memalloc.c
>> +++ b/sound/core/memalloc.c
>> @@ -540,13 +540,18 @@ static void *snd_dma_noncontig_alloc(struct snd_dma_buffer *dmab, size_t size)
>>  {
>>  	struct sg_table *sgt;
>>  	void *p;
>> +	gfp_t gfp_flags = DEFAULT_GFP;
>>  
>>  #ifdef CONFIG_SND_DMA_SGBUF
>>  	if (cpu_feature_enabled(X86_FEATURE_XENPV))
>>  		return snd_dma_sg_fallback_alloc(dmab, size);
>> +
>> +	/* Non-IOMMU case: prevent allocator from searching forever */
>> +	if (!get_dma_ops(dmab->dev.dev))
>> +		gfp_flags |= __GFP_NORETRY;
>>  #endif
>>  	sgt = dma_alloc_noncontiguous(dmab->dev.dev, size, dmab->dev.dir,
>> -				      DEFAULT_GFP, 0);
>> +				      gfp_flags, 0);
>>  #ifdef CONFIG_SND_DMA_SGBUF
>>  	if (!sgt && !get_dma_ops(dmab->dev.dev))
>>  		return snd_dma_sg_fallback_alloc(dmab, size);
>> -- 
>> 2.43.0.687.g38aa6559b0-goog
>>