[PATCH] quiet down swiotlb warnings

[PATCH] quiet down swiotlb warnings

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

It turns out that the qla2xxx driver sometimes fills up the iotlb
on purpose and throttles itself when pci_map_sg() fails.  In the
case of a driver that expects and handles pci_map_sg() failures,
we should not spam the user's console with swiotlb full messages.

If the user runs a driver that cannot handle the swiotlb filling
up, 5 warnings should be enough to leave a clue.

Signed-off-by: Rik van Riel <riel@redhat.com>

[-- Attachment #2: 2.6.21-swiotlb-quiet.patch --]
[-- Type: text/x-patch, Size: 1033 bytes --]

--- linux-2.6.21.noarch/lib/swiotlb.c.quiet	2007-06-01 13:23:04.000000000 -0400
+++ linux-2.6.21.noarch/lib/swiotlb.c	2007-06-01 13:25:49.000000000 -0400
@@ -505,6 +505,7 @@ swiotlb_free_coherent(struct device *hwd
 static void
 swiotlb_full(struct device *dev, size_t size, int dir, int do_panic)
 {
+	static int warnings = 0;
 	/*
 	 * Ran out of IOMMU space for this operation. This is very bad.
 	 * Unfortunately the drivers cannot handle this operation properly.
@@ -512,8 +513,9 @@ swiotlb_full(struct device *dev, size_t 
 	 * When the mapping is small enough return a static buffer to limit
 	 * the damage, or panic when the transfer is too big.
 	 */
-	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
-	       "device %s\n", size, dev ? dev->bus_id : "?");
+	if (++warnings < 5)
+		printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
+		       "device %s\n", size, dev ? dev->bus_id : "?");
 
 	if (size > io_tlb_overflow && do_panic) {
 		if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)

Re: [PATCH] quiet down swiotlb warnings

[Christoph Hellwig]

On Fri, Jun 01, 2007 at 09:01:45PM +0200, Andi Kleen wrote:
> Bad idea imho. swiotlb mappings should always lead to printk by default
> because it is pretty dangerous.
> 
> One possible solution for this I could think of would be to define a
> new pci_map_sg_couldfail() or similar that doesn't warn and use a weak
> fallback just calling pci_map_sg on other IOMMU implementations. 

pci_map_sg is defined to be failing when running out of ressources, which
is perfectly fine.  We don't printk on kmalloc failures either (actually
in some cases which is highly annoying and leads people to stick a
__GFP_NOWARN into various places)

Re: [PATCH] quiet down swiotlb warnings

[Andrew Vasquez]

On Fri, 01 Jun 2007, Andi Kleen wrote:

> Rik van Riel <riel@redhat.com> writes:
> 
> > It turns out that the qla2xxx driver sometimes fills up the iotlb
> > on purpose and throttles itself when pci_map_sg() fails.  In the
> > case of a driver that expects and handles pci_map_sg() failures,
> > we should not spam the user's console with swiotlb full messages.
> 
> Why does it do that? Could we supply a better interface
> for whatever it is trying to do here?

The driver only calls pci_map_sg() once it's insured that all local
driver resources are available to submit an I/O to the hardware.

> > -	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
> > -	       "device %s\n", size, dev ? dev->bus_id : "?");
> > +	if (++warnings < 5)
> > +		printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
> > +		       "device %s\n", size, dev ? dev->bus_id : "?");
> 
> Bad idea imho. swiotlb mappings should always lead to printk by default
> because it is pretty dangerous.

Why?  It's just another resource which is consumed -- the qla2xxx
driver is the final consumer before I/O is submitted out on the wire.
The mappings are held for the shorted time required -- as such, are
released as soon as the I/O completes.

> One possible solution for this I could think of would be to define a
> new pci_map_sg_couldfail() or similar that doesn't warn and use a weak
> fallback just calling pci_map_sg on other IOMMU implementations. 

Re: [PATCH] quiet down swiotlb warnings

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

Christoph Hellwig wrote:
> On Fri, Jun 01, 2007 at 09:01:45PM +0200, Andi Kleen wrote:
>> Bad idea imho. swiotlb mappings should always lead to printk by default
>> because it is pretty dangerous.
>>
>> One possible solution for this I could think of would be to define a
>> new pci_map_sg_couldfail() or similar that doesn't warn and use a weak
>> fallback just calling pci_map_sg on other IOMMU implementations. 
> 
> pci_map_sg is defined to be failing when running out of ressources, which
> is perfectly fine.  We don't printk on kmalloc failures either (actually
> in some cases which is highly annoying and leads people to stick a
> __GFP_NOWARN into various places)

Andi, I could see your "pretty dangerous" case applying
when do_panic is set, but not in any other circumstances.

Does the patch below look better to you?

Signed-off-by: Rik van Riel <riel@redhat.com>

[-- Attachment #2: 2.6.21-swiotlb-quiet.patch --]
[-- Type: text/x-patch, Size: 1045 bytes --]

--- linux-2.6.21.noarch/lib/swiotlb.c.quiet	2007-06-01 13:23:04.000000000 -0400
+++ linux-2.6.21.noarch/lib/swiotlb.c	2007-06-01 14:18:26.000000000 -0400
@@ -505,6 +505,7 @@ swiotlb_free_coherent(struct device *hwd
 static void
 swiotlb_full(struct device *dev, size_t size, int dir, int do_panic)
 {
+	static int warnings = 0;
 	/*
 	 * Ran out of IOMMU space for this operation. This is very bad.
 	 * Unfortunately the drivers cannot handle this operation properly.
@@ -512,8 +513,9 @@ swiotlb_full(struct device *dev, size_t 
 	 * When the mapping is small enough return a static buffer to limit
 	 * the damage, or panic when the transfer is too big.
 	 */
-	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
-	       "device %s\n", size, dev ? dev->bus_id : "?");
+	if (do_panic || ++warnings < 5)
+		printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
+		       "device %s\n", size, dev ? dev->bus_id : "?");
 
 	if (size > io_tlb_overflow && do_panic) {
 		if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)

Re: [PATCH] quiet down swiotlb warnings

[Andi Kleen]

Rik van Riel <riel@redhat.com> writes:

> It turns out that the qla2xxx driver sometimes fills up the iotlb
> on purpose and throttles itself when pci_map_sg() fails.  In the
> case of a driver that expects and handles pci_map_sg() failures,
> we should not spam the user's console with swiotlb full messages.

Why does it do that? Could we supply a better interface
for whatever it is trying to do here?

>  	 */
> -	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
> -	       "device %s\n", size, dev ? dev->bus_id : "?");
> +	if (++warnings < 5)
> +		printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
> +		       "device %s\n", size, dev ? dev->bus_id : "?");

Bad idea imho. swiotlb mappings should always lead to printk by default
because it is pretty dangerous.

One possible solution for this I could think of would be to define a
new pci_map_sg_couldfail() or similar that doesn't warn and use a weak
fallback just calling pci_map_sg on other IOMMU implementations. 

 -Andi

Re: [PATCH] quiet down swiotlb warnings

[Andi Kleen]

On Fri, Jun 01, 2007 at 07:12:25PM +0100, Christoph Hellwig wrote:
> On Fri, Jun 01, 2007 at 09:01:45PM +0200, Andi Kleen wrote:
> > Bad idea imho. swiotlb mappings should always lead to printk by default
> > because it is pretty dangerous.
> > 
> > One possible solution for this I could think of would be to define a
> > new pci_map_sg_couldfail() or similar that doesn't warn and use a weak
> > fallback just calling pci_map_sg on other IOMMU implementations. 
> 
> pci_map_sg is defined to be failing when running out of ressources, which
> is perfectly fine.  We don't printk on kmalloc failures either (actually

Actually we do as you point out.

> in some cases which is highly annoying and leads people to stick a
> __GFP_NOWARN into various places)

An pci_map_sg failing typically leads to an IO error and we've
always printk'ed those. Otherwise people will wonder why they
get EIO.

-Andi

Re: [PATCH] quiet down swiotlb warnings

[Rik van Riel]

Andi Kleen wrote:

> An pci_map_sg failing typically leads to an IO error and we've
> always printk'ed those. Otherwise people will wonder why they
> get EIO.

In some situations.  In this case the qla2xxx driver uses
the pci_map_sg() failure as a throttling mechanism and
printing out all the warnings will actually slow down the
system.

Andi, what do you propose as a solution?

-- 
Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.

Re: [PATCH] quiet down swiotlb warnings

[Andi Kleen]

On Fri, Jun 01, 2007 at 03:38:57PM -0400, Rik van Riel wrote:
> Andi Kleen wrote:
> 
> >An pci_map_sg failing typically leads to an IO error and we've
> >always printk'ed those. Otherwise people will wonder why they
> >get EIO.
> 
> In some situations.  In this case the qla2xxx driver uses
> the pci_map_sg() failure as a throttling mechanism and

First WTF does it need swiotlb anyways? QA hardware should
be definitely DAC capable, shouldn't it?

> printing out all the warnings will actually slow down the
> system.

Another reason is that there is a lot of code that
still doesn't check the return values and when that
happens you might get data corruption too.

> 
> Andi, what do you propose as a solution?

A different interface; like I wrote in my earlier mail.

Another probabibility would be to have a blocking interface
to swiotlb that won't fail. That would be the better solution
long term, but i was told it is hard to fit into some current
driver interfaces.

-Andi

Re: [PATCH] quiet down swiotlb warnings

[Christoph Hellwig]

On Fri, Jun 01, 2007 at 09:37:03PM +0200, Andi Kleen wrote:
> > in some cases which is highly annoying and leads people to stick a
> > __GFP_NOWARN into various places)
> 
> An pci_map_sg failing typically leads to an IO error and we've
> always printk'ed those. Otherwise people will wonder why they
> get EIO.

Not in a scsi driver.  It will get requeued and the midlayer will submit
it again when an outstanding command has finished.

Re: [PATCH] quiet down swiotlb warnings

[Andrew Vasquez]

On Fri, 01 Jun 2007, Andi Kleen wrote:

> On Fri, Jun 01, 2007 at 03:38:57PM -0400, Rik van Riel wrote:
> > Andi Kleen wrote:
> > 
> > >An pci_map_sg failing typically leads to an IO error and we've
> > >always printk'ed those. Otherwise people will wonder why they
> > >get EIO.
> > 
> > In some situations.  In this case the qla2xxx driver uses
> > the pci_map_sg() failure as a throttling mechanism and
> 
> First WTF does it need swiotlb anyways? QA hardware should
> be definitely DAC capable, shouldn't it?

yes, the card can support 64bit DMA transfers. but in this case the
'required' DMA mask returned from dma_get_required_mask() states that a
32bit mask would suffice.

Here's a snippet from the bugzilla report
(https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=219216):

        QLogic Fibre Channel HBA Driver
        PCI: Enabling device 0000:1f:00.0 (0140 -> 0143)
        ACPI: PCI Interrupt 0000:1f:00.0[A] -> GSI 16 (level, low) -> IRQ 16
        qla2xxx 0000:1f:00.0: Found an ISP2432, irq 16, iobase 0xffffc20000020000
        *** qla2x00_config_dma_addressing: required_mask set to 000000007fffffff.
        *** qla2x00_config_dma_addressing: required_mask has no high-dword bits set.
        *** qla2x00_config_dma_addressing: set consistent 64bit mask returned 0.
        *** qla2x00_config_dma_addressing: defaulting to 32bit mask/consistent-mask.
        qla2xxx 0000:1f:00.0: Configuring PCI space...

	Which tells me that a 32bit DMA mask is being set for dma_set_mask()
	and pci_set_consistent_dma_mask() since dma_get_required_mask() is
	returning back 7fffffff -- no upper-dword bits set...
	...

> > printing out all the warnings will actually slow down the
> > system.
> 
> Another reason is that there is a lot of code that
> still doesn't check the return values and when that
> happens you might get data corruption too.
> 
> > 
> > Andi, what do you propose as a solution?
> 
> A different interface; like I wrote in my earlier mail.
> 
> Another probabibility would be to have a blocking interface
> to swiotlb that won't fail. That would be the better solution
> long term, but i was told it is hard to fit into some current
> driver interfaces.

Re: [PATCH] quiet down swiotlb warnings

[Andi Kleen]

On Fri, Jun 01, 2007 at 01:00:49PM -0700, Andrew Vasquez wrote:
> yes, the card can support 64bit DMA transfers. but in this case the
> 'required' DMA mask returned from dma_get_required_mask() states that a
> 32bit mask would suffice.

That's a bug in the kernel then that needs to be fixed. Find out
why it does that and change it. Don't fix the symptoms, fix the root cause.

-Andi

Re: [PATCH] quiet down swiotlb warnings

[Rik van Riel]

Andi Kleen wrote:
> On Fri, Jun 01, 2007 at 01:00:49PM -0700, Andrew Vasquez wrote:
>> yes, the card can support 64bit DMA transfers. but in this case the
>> 'required' DMA mask returned from dma_get_required_mask() states that a
>> 32bit mask would suffice.
> 
> That's a bug in the kernel then that needs to be fixed. Find out
> why it does that and change it. Don't fix the symptoms, fix the root cause.

The system in question only has 2GB of memory.

Why would it need to do 64 bit DMA?

-- 
Politics is the struggle between those who want to make their country
the best in the world, and those who believe it already is.  Each group
calls the other unpatriotic.

Re: [PATCH] quiet down swiotlb warnings

[Andi Kleen]

On Fri, Jun 01, 2007 at 04:20:38PM -0400, Rik van Riel wrote:
> Andi Kleen wrote:
> >On Fri, Jun 01, 2007 at 01:00:49PM -0700, Andrew Vasquez wrote:
> >>yes, the card can support 64bit DMA transfers. but in this case the
> >>'required' DMA mask returned from dma_get_required_mask() states that a
> >>32bit mask would suffice.
> >
> >That's a bug in the kernel then that needs to be fixed. Find out
> >why it does that and change it. Don't fix the symptoms, fix the root cause.
> 
> The system in question only has 2GB of memory.
> 
> Why would it need to do 64 bit DMA?

It doesn't. But it definitely doesn't need swiotlb either.

Normally swiotlb doesn't even try to bounce when dma mask is <= end_pfn
so something must be very wrong in your kernel. It definitely isn't
a mainline kernel. If this happens in Xen then Xen just needs fixing --
it should not try to bounce when the normal kernel wouldn't.

-Andi

Re: [PATCH] quiet down swiotlb warnings

[Muli Ben-Yehuda]

On Fri, Jun 01, 2007 at 10:26:01PM +0200, Andi Kleen wrote:

> Normally swiotlb doesn't even try to bounce when dma mask is <=
> end_pfn so something must be very wrong in your kernel. It
> definitely isn't a mainline kernel. If this happens in Xen then Xen
> just needs fixing -- it should not try to bounce when the normal
> kernel wouldn't.

Xen needs to bounce when the requested buffer is not contiguous in
machine memory (and indeed uses swiotlb for that).

Cheers,
Muli

Re: [PATCH] quiet down swiotlb warnings

[Andi Kleen]

On Sat, Jun 02, 2007 at 06:21:46PM +0300, Muli Ben-Yehuda wrote:
> On Fri, Jun 01, 2007 at 10:26:01PM +0200, Andi Kleen wrote:
> 
> > Normally swiotlb doesn't even try to bounce when dma mask is <=
> > end_pfn so something must be very wrong in your kernel. It
> > definitely isn't a mainline kernel. If this happens in Xen then Xen
> > just needs fixing -- it should not try to bounce when the normal
> > kernel wouldn't.
> 
> Xen needs to bounce when the requested buffer is not contiguous in
> machine memory (and indeed uses swiotlb for that).

Then it should just restrict the sg list merging at the block layer
to never merge into anything larger than a page. Then this cannot
happen or only very rarely.

-Andi