Re: regression with SG DMA buf allocations and IOMMU in low-mem

Re: regression with SG DMA buf allocations and IOMMU in low-mem

[Kai Vehmanen]

Hi Takashi and list,

I've been debugging a SOF DSP load fail on VT-D/IOMMU systems [1] and 
that brought me to these two commits from you:

"ALSA: memalloc: Add fallback SG-buffer allocations for x86"
and

"ALSA: memalloc: Revive x86-specific WC page allocations again"

We have an allocation with:
snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG, &pci->dev, size, dmab);

.. but in rare low-memory cases, the dma_alloc_noncontiguous()
call will fail in core/memalloc.c and we go to the fallback path.

So we go to snd_dma_sg_fallback_alloc(), but here it seems something is 
missing. The pages are not allocated with DMA mapping API anymore, so 
IOMMU won't know about the memory and in our case the DSP load will fail 
to a IOMMU fault. 

Looking at 5.15 code, the fallback looks very different, but 
in fallback we still use the DMA mapping API via snd_dma_dev_alloc()
so even if we go to fallback path, mapping is still ok.

Any ideas which way this should be fixed? Given the many changes
I thought it's better to ask early on the list about this.

[1] https://github.com/thesofproject/linux/issues/3915

Br, Kai

Re: regression with SG DMA buf allocations and IOMMU in low-mem

[Kai Vehmanen]

Hi,

[ and sorry, I had the bounces address in cc originally, routing
  reply to list as well ]

On Fri, 4 Nov 2022, Takashi Iwai wrote:

> On Fri, 04 Nov 2022 15:56:50 +0100, Kai Vehmanen wrote:
> > I've been debugging a SOF DSP load fail on VT-D/IOMMU systems [1] and 
> > that brought me to these two commits from you:
> > .. but in rare low-memory cases, the dma_alloc_noncontiguous()
> > call will fail in core/memalloc.c and we go to the fallback path.
> > 
> > So we go to snd_dma_sg_fallback_alloc(), but here it seems something is 
> > missing. The pages are not allocated with DMA mapping API anymore, so 
> > IOMMU won't know about the memory and in our case the DSP load will fail 
> > to a IOMMU fault. 
[...]
> 
> Hrm, that's a tough issue.  Basically if dma_alloc_noncontiguous()
> fails, it's really out of memory -- at least under the situation where
> IOMMU is enabled.  The fallback path was introduced for the case where
> there is no IOMMU and noncontiguous allocation fails from the
> beginning.

ack. This matches with the bug reports. These happen rarely and on systems 
with a lot of memory pressure. We have recently submitted a few 
features&fixes that will further reduce these allocs, so it probably is 
better outcome to have a plain out of memory error.

> And, what makes things more complicated is that snd_dma_dev_alloc()
> cannot be used for SG pages when IOMMU is involved.  We can't get the
> proper pages for mapping SG from the DMA address in that case; some
> systems may work with virt_to_page() but it's not guranteed to work.

Aa, ok, so we need to use dma_alloc_noncontiguous() to do this properly 
and if it returns ENOMEM, that's what it is then, right.

> So, I believe there are two issues:
> 1. The firmware allocation fails with dma_alloc_noncontiguous() with
>   IOMMU enabled
> 
> 2. The helper goes to the fallback path and occasionally it worked,
>   although the pages can't be used with IOMMU.
> 
> Basically when 1 happens, we should just return an error without going
> to fallback path.  But it won't "fix" your case?

I think an explicit error would be best. The problem now is that the 
driver will think the allocation (and mapping to device) is fine and 
proceeds to program the hardware to use the address. This will then create 
an IOMMU fault down the line that is not so straighforward to recover from 
(worst case is that a full device level reset needs to be done). And given 
driver doesn't know it got a faulty mapping, it's hard to make the 
decision why the fault happened.

Br, Kai

Re: regression with SG DMA buf allocations and IOMMU in low-mem

[Takashi Iwai]

On Fri, 04 Nov 2022 16:42:29 +0100,
Kai Vehmanen wrote:
> 
> Hi,
> 
> [ and sorry, I had the bounces address in cc originally, routing
>   reply to list as well ]
> 
> On Fri, 4 Nov 2022, Takashi Iwai wrote:
> 
> > On Fri, 04 Nov 2022 15:56:50 +0100, Kai Vehmanen wrote:
> > > I've been debugging a SOF DSP load fail on VT-D/IOMMU systems [1] and 
> > > that brought me to these two commits from you:
> > > .. but in rare low-memory cases, the dma_alloc_noncontiguous()
> > > call will fail in core/memalloc.c and we go to the fallback path.
> > > 
> > > So we go to snd_dma_sg_fallback_alloc(), but here it seems something is 
> > > missing. The pages are not allocated with DMA mapping API anymore, so 
> > > IOMMU won't know about the memory and in our case the DSP load will fail 
> > > to a IOMMU fault. 
> [...]
> > 
> > Hrm, that's a tough issue.  Basically if dma_alloc_noncontiguous()
> > fails, it's really out of memory -- at least under the situation where
> > IOMMU is enabled.  The fallback path was introduced for the case where
> > there is no IOMMU and noncontiguous allocation fails from the
> > beginning.
> 
> ack. This matches with the bug reports. These happen rarely and on systems 
> with a lot of memory pressure. We have recently submitted a few 
> features&fixes that will further reduce these allocs, so it probably is 
> better outcome to have a plain out of memory error.
> 
> > And, what makes things more complicated is that snd_dma_dev_alloc()
> > cannot be used for SG pages when IOMMU is involved.  We can't get the
> > proper pages for mapping SG from the DMA address in that case; some
> > systems may work with virt_to_page() but it's not guranteed to work.
> 
> Aa, ok, so we need to use dma_alloc_noncontiguous() to do this properly 
> and if it returns ENOMEM, that's what it is then, right.
> 
> > So, I believe there are two issues:
> > 1. The firmware allocation fails with dma_alloc_noncontiguous() with
> >   IOMMU enabled
> > 
> > 2. The helper goes to the fallback path and occasionally it worked,
> >   although the pages can't be used with IOMMU.
> > 
> > Basically when 1 happens, we should just return an error without going
> > to fallback path.  But it won't "fix" your case?
> 
> I think an explicit error would be best. The problem now is that the 
> driver will think the allocation (and mapping to device) is fine and 
> proceeds to program the hardware to use the address. This will then create 
> an IOMMU fault down the line that is not so straighforward to recover from 
> (worst case is that a full device level reset needs to be done). And given 
> driver doesn't know it got a faulty mapping, it's hard to make the 
> decision why the fault happened.

OK, then what I posted in another mail (it went to nirvana) might
work.  Attached below again.


Takashi

-- 8< --
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -9,6 +9,7 @@
 #include <linux/slab.h>
 #include <linux/mm.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma-map-ops.h>
 #include <linux/genalloc.h>
 #include <linux/highmem.h>
 #include <linux/vmalloc.h>
@@ -541,19 +542,20 @@ static void *snd_dma_noncontig_alloc(struct snd_dma_buffer *dmab, size_t size)
 	struct sg_table *sgt;
 	void *p;
 
-	sgt = dma_alloc_noncontiguous(dmab->dev.dev, size, dmab->dev.dir,
-				      DEFAULT_GFP, 0);
-	if (!sgt) {
 #ifdef CONFIG_SND_DMA_SGBUF
+	if (!get_dma_ops(dmab->dev.dev)) {
 		if (dmab->dev.type == SNDRV_DMA_TYPE_DEV_WC_SG)
 			dmab->dev.type = SNDRV_DMA_TYPE_DEV_WC_SG_FALLBACK;
 		else
 			dmab->dev.type = SNDRV_DMA_TYPE_DEV_SG_FALLBACK;
 		return snd_dma_sg_fallback_alloc(dmab, size);
-#else
-		return NULL;
-#endif
 	}
+#endif
+
+	sgt = dma_alloc_noncontiguous(dmab->dev.dev, size, dmab->dev.dir,
+				      DEFAULT_GFP, 0);
+	if (!sgt)
+		return NULL;
 
 	dmab->dev.need_sync = dma_need_sync(dmab->dev.dev,
 					    sg_dma_address(sgt->sgl));
@@ -857,7 +859,7 @@ static const struct snd_malloc_ops snd_dma_noncoherent_ops = {
 /*
  * Entry points
  */
-static const struct snd_malloc_ops *dma_ops[] = {
+static const struct snd_malloc_ops *snd_dma_ops[] = {
 	[SNDRV_DMA_TYPE_CONTINUOUS] = &snd_dma_continuous_ops,
 	[SNDRV_DMA_TYPE_VMALLOC] = &snd_dma_vmalloc_ops,
 #ifdef CONFIG_HAS_DMA
@@ -883,7 +885,7 @@ static const struct snd_malloc_ops *snd_dma_get_ops(struct snd_dma_buffer *dmab)
 	if (WARN_ON_ONCE(!dmab))
 		return NULL;
 	if (WARN_ON_ONCE(dmab->dev.type <= SNDRV_DMA_TYPE_UNKNOWN ||
-			 dmab->dev.type >= ARRAY_SIZE(dma_ops)))
+			 dmab->dev.type >= ARRAY_SIZE(snd_dma_ops)))
 		return NULL;
-	return dma_ops[dmab->dev.type];
+	return snd_dma_ops[dmab->dev.type];
 }

Re: regression with SG DMA buf allocations and IOMMU in low-mem

[Kai Vehmanen]

Hi,

On Fri, 4 Nov 2022, Takashi Iwai wrote:

> On Fri, 04 Nov 2022 16:42:29 +0100, Kai Vehmanen wrote:
> > I think an explicit error would be best. The problem now is that the 
> > driver will think the allocation (and mapping to device) is fine and 
> > proceeds to program the hardware to use the address. This will then create 
> > an IOMMU fault down the line that is not so straighforward to recover from 
> > (worst case is that a full device level reset needs to be done). And given 
> > driver doesn't know it got a faulty mapping, it's hard to make the 
> > decision why the fault happened.
> 
> OK, then what I posted in another mail (it went to nirvana) might
> work.  Attached below again.

thanks! Let me put this through testing and get back to you next 
week. We'll also debug a bit more what exactly goes on that leads to 
dma_alloc_noncontiguous failing.

Br, Kai

Re: regression with SG DMA buf allocations and IOMMU in low-mem

[Kai Vehmanen]

Hi,

On Fri, 4 Nov 2022, Kai Vehmanen wrote:

> On Fri, 4 Nov 2022, Takashi Iwai wrote:
> 
> > On Fri, 04 Nov 2022 16:42:29 +0100, Kai Vehmanen wrote:
> > > I think an explicit error would be best. The problem now is that the 
> > > driver will think the allocation (and mapping to device) is fine and 
> > > proceeds to program the hardware to use the address. This will then create 
> > > an IOMMU fault down the line that is not so straighforward to recover from 
> > > (worst case is that a full device level reset needs to be done). And given 
> > > driver doesn't know it got a faulty mapping, it's hard to make the 
> > > decision why the fault happened.
> > 
> > OK, then what I posted in another mail (it went to nirvana) might
> > work.  Attached below again.
> 
> thanks! Let me put this through testing and get back to you next 
> week. We'll also debug a bit more what exactly goes on that leads to 
> dma_alloc_noncontiguous failing.

Takashi, the patch seems good. We've included it in multiple test runs and
seems to be working as intented, so:

Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

... for the patch. Rootcausing why dma_alloc_noncontiguous() fails is 
still ongoing (reproduction rate is very, very low). I've been looking at 
snd_dma_sg_fallback_alloc() and comparing to iommu dma allocator code, 
and it's curious how can we have a case where former succeeds but latter 
not. We e.g. now pass __GFP_RETRY_MAYFAIL to dma_alloc_noncontiguous(), 
but snd_dma_sg_fallback_alloc() does the alloc_pages_exact() call with 
__GFP_NORETRY. But of course it can fail when doing the mapping and 
there's more code involved. But alas this is a separate issue for 
us to track down.

Br, Kai

Re: regression with SG DMA buf allocations and IOMMU in low-mem

[Kai Vehmanen]

Hi,

On Fri, 4 Nov 2022, Takashi Iwai wrote:

> On Fri, 04 Nov 2022 15:56:50 +0100, Kai Vehmanen wrote:
> > Looking at 5.15 code, the fallback looks very different, but 
> > in fallback we still use the DMA mapping API via snd_dma_dev_alloc()
> > so even if we go to fallback path, mapping is still ok.
> > 
> > Any ideas which way this should be fixed? Given the many changes
> > I thought it's better to ask early on the list about this.
> 
> Hrm, that's a tough issue.  Basically if dma_alloc_noncontiguous()
> fails, it's really out of memory -- at least under the situation where
> IOMMU is enabled.  The fallback path was introduced for the case where
> there is no IOMMU and noncontiguous allocation fails from the
> beginning.

follow-up to this problem. It turns out there was additional problem
with dma_alloc_noncontiguous that led to ENOMEM much more frequently
than in the past. This required following:
  - your rework in 5.16 to sound/core/memalloc.c for x86 DMA SG
  - IOMMU/DMAR enabled for the device  
  - CONFIG_DMA_REMAP=n (default 'n' on most/all x86 systems before 5.18)

... with above combination, iommu_dma_ops in drivers/iommu/dma-iommu.c 
were not defined for noncontiguous alloc/free, so the 
dma_alloc_noncontiguous() in the end went back to plain 
dma_common_alloc_pages() (single contiguous alloc), which would fail very 
easily when system was in low-mem condition.

In 5.18, this patch was added

  commit f5ff79fddf0efecca538046b5cc20fb3ded2ec4f
  Author: Christoph Hellwig <hch@lst.de>
  Date:   Sat Feb 26 16:40:21 2022 +0100

      dma-mapping: remove CONFIG_DMA_REMAP

... and this basicly fixes the issue again, even when IOMMU is enabled.

Br, Kai