[PATCH] powerpc: add kernel parameter iommu_alloc_quiet

[PATCH] powerpc: add kernel parameter iommu_alloc_quiet

[Mauricio Faria de Oliveira]

This patch introduces the 'iommu_alloc_quiet=driver_name' parameter
to suppress the 'iommu_alloc failures' messages for that one driver.

This is an additional approach for this 'problem' of flooding logs,
not as fine-grained and not enabled by default as DMA_ATTR_NO_WARN,
but it has the advantage that it doesn't introduce any ABI changes.

That is important/requirement for the distribution kernels - where
the DMA_ATTR_NO_WARN changes to 'enum dma_attr' are not acceptable
because it breaks the kernel ABI.

Tested on next-20160825 + nvme changed not to use DMA_ATTR_NO_WARN.

 - test-case: default / no iommu_alloc_quiet
 - result:    messages occur

    # dmesg -c | grep iommu_alloc_quiet
    #

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c
    <...>
    [   31.753230] nvme 0000:00:06.0: iommu_alloc failed, tbl c0000003f7080c00 vaddr c00000022bf30000 npages 16

 - test-case: iommu_alloc_quiet=(null)
 - result:    messages occur

    # dmesg -c | grep iommu_alloc_quiet
    [    0.000000] Kernel command line: root=<...> ro disable_ddw iommu_alloc_quiet=
    [    0.000000] iommu_alloc_quiet: driver ''

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c
    <...>
    [   29.032848] nvme 0000:00:06.0: iommu_alloc failed, tbl c0000003f7190c00 vaddr c000000238fc0000 npages 16

 - test-case: iommu_alloc_quiet=(length overflow)
 - result:    messages occur

    # dmesg -c | grep iommu_alloc_quiet
    [    0.000000] Kernel command line: root=<...> ro disable_ddw iommu_alloc_quiet=0123456789abcdef0123456789abcdef
    [    0.000000] iommu_alloc_quiet: driver '0123456789abcde'

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c
    <...>
    [   54.913279] nvme 0000:00:06.0: iommu_alloc failed, tbl c0000003f7120c00 vaddr c00000022d960000 npages 16

 - test-case: iommu_alloc_quiet=nvme
 - result:    messages do not occur

    # dmesg -c | grep iommu_alloc_quiet
    [    0.000000] Kernel command line: root=<...> ro disable_ddw iommu_alloc_quiet=nvme
    [    0.000000] iommu_alloc_quiet: driver 'nvme'

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c

    # dd if=/dev/zero of=/dev/nvme0n1p1 bs=64k count=128k 2>/dev/null; dmesg -c

Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/iommu.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 5f202a5..8524d91 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -65,6 +65,23 @@ static int __init setup_iommu(char *str)
 
 __setup("iommu=", setup_iommu);
 
+/*
+ * iommu_alloc_quiet: string with one driver name
+ * not to print 'iommu_alloc failed' messages for.
+ */
+#define IOMMU_ALLOC_QUIET_LEN	16 /* includes '\0' */
+static char iommu_alloc_quiet[IOMMU_ALLOC_QUIET_LEN];
+
+static int __init setup_iommu_alloc_quiet(char *str)
+{
+	strncpy(iommu_alloc_quiet, str, IOMMU_ALLOC_QUIET_LEN);
+	iommu_alloc_quiet[IOMMU_ALLOC_QUIET_LEN - 1] = '\0';
+	pr_info("iommu_alloc_quiet: driver '%s'\n", iommu_alloc_quiet);
+	return 1;
+}
+
+__setup("iommu_alloc_quiet=", setup_iommu_alloc_quiet);
+
 static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
 
 /*
@@ -479,8 +496,8 @@ int ppc_iommu_map_sg(struct device *dev, struct iommu_table *tbl,
 
 		/* Handle failure */
 		if (unlikely(entry == DMA_ERROR_CODE)) {
-			if (!(attrs & DMA_ATTR_NO_WARN) &&
-			    printk_ratelimit())
+			if (strncmp(iommu_alloc_quiet, dev->driver->name, IOMMU_ALLOC_QUIET_LEN) &&
+			    !(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit())
 				dev_info(dev, "iommu_alloc failed, tbl %p "
 					 "vaddr %lx npages %lu\n", tbl, vaddr,
 					 npages);
@@ -777,8 +794,8 @@ dma_addr_t iommu_map_page(struct device *dev, struct iommu_table *tbl,
 					 mask >> tbl->it_page_shift, align,
 					 attrs);
 		if (dma_handle == DMA_ERROR_CODE) {
-			if (!(attrs & DMA_ATTR_NO_WARN) &&
-			    printk_ratelimit())  {
+			if (strncmp(iommu_alloc_quiet, dev->driver->name, IOMMU_ALLOC_QUIET_LEN) &&
+			    !(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
 				dev_info(dev, "iommu_alloc failed, tbl %p "
 					 "vaddr %p npages %d\n", tbl, vaddr,
 					 npages);
-- 
1.8.3.1

Re: [PATCH] powerpc: add kernel parameter iommu_alloc_quiet

[Mauricio Faria de Oliveira]

Michael / Ben,

On 09/01/2016 09:56 AM, Mauricio Faria de Oliveira wrote:
> +#define IOMMU_ALLOC_QUIET_LEN	16 /* includes '\0' */

Guilherme suggested MAX_PARAM_PREFIX_LEN for this, which looks
better (a few extra bytes).

Would you mind to s/16/MAX_PARAM_PREFIX_LEN/ if you like that?
I can send a v2 too; just let me know.

Thanks,

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [PATCH] powerpc: add kernel parameter iommu_alloc_quiet

[Torsten Duwe]

On Thu, Sep 01, 2016 at 09:56:42AM -0300, Mauricio Faria de Oliveira wrote:
> This patch introduces the 'iommu_alloc_quiet=driver_name' parameter
> to suppress the 'iommu_alloc failures' messages for that one driver.
> 
> This is an additional approach for this 'problem' of flooding logs,
> not as fine-grained and not enabled by default as DMA_ATTR_NO_WARN,
> but it has the advantage that it doesn't introduce any ABI changes.
> 
> That is important/requirement for the distribution kernels - where
> the DMA_ATTR_NO_WARN changes to 'enum dma_attr' are not acceptable
> because it breaks the kernel ABI.
> 
[...]
> @@ -479,8 +496,8 @@ int ppc_iommu_map_sg(struct device *dev, struct iommu_table *tbl,
>  
>  		/* Handle failure */
>  		if (unlikely(entry == DMA_ERROR_CODE)) {
> -			if (!(attrs & DMA_ATTR_NO_WARN) &&
> -			    printk_ratelimit())
> +			if (strncmp(iommu_alloc_quiet, dev->driver->name, IOMMU_ALLOC_QUIET_LEN) &&
> +			    !(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit())
>  				dev_info(dev, "iommu_alloc failed, tbl %p "
>  					 "vaddr %lx npages %lu\n", tbl, vaddr,
>  					 npages);
> @@ -777,8 +794,8 @@ dma_addr_t iommu_map_page(struct device *dev, struct iommu_table *tbl,
>  					 mask >> tbl->it_page_shift, align,
>  					 attrs);
>  		if (dma_handle == DMA_ERROR_CODE) {
> -			if (!(attrs & DMA_ATTR_NO_WARN) &&
> -			    printk_ratelimit())  {
> +			if (strncmp(iommu_alloc_quiet, dev->driver->name, IOMMU_ALLOC_QUIET_LEN) &&
> +			    !(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) {
>  				dev_info(dev, "iommu_alloc failed, tbl %p "
>  					 "vaddr %p npages %d\n", tbl, vaddr,
>  					 npages);

JFYI, my strongly preferred solution would still be to just dev_dbg() the whole thing.
Which group of people would be interested in these messages, after all?

	Torsten

Re: [PATCH] powerpc: add kernel parameter iommu_alloc_quiet

[Mauricio Faria de Oliveira]

On 09/01/2016 10:39 AM, Torsten Duwe wrote:
> JFYI, my strongly preferred solution would still be to just dev_dbg() the whole thing.
> Which group of people would be interested in these messages, after all?

Certainly understandable.

Ben didn't like the idea to convert the messages to dynamic debug [1],
(with which I agree/understand nowadays) and suggested us to look for
a different approach.

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-June/144196.html

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: powerpc: add kernel parameter iommu_alloc_quiet

[Michael Ellerman]

On Thu, 2016-01-09 at 12:56:42 UTC, Mauricio Faria de Oliveira wrote:
> This patch introduces the 'iommu_alloc_quiet=driver_name' parameter
> to suppress the 'iommu_alloc failures' messages for that one driver.
> 
> This is an additional approach for this 'problem' of flooding logs,
> not as fine-grained and not enabled by default as DMA_ATTR_NO_WARN,
> but it has the advantage that it doesn't introduce any ABI changes.
> 
> That is important/requirement for the distribution kernels - where
> the DMA_ATTR_NO_WARN changes to 'enum dma_attr' are not acceptable
> because it breaks the kernel ABI.

Adding an entry to an enum does not break the ABI.

In fact AFAIK the enum doesn't exist in the ABI, at the binary level an enum
just becomes an int (possibly excluding debug info).

Removing an entry from an enum would break the API (Note _P_). But I don't see
how adding one does.

Also kernel command line parameters are really poor in terms of usability. Folks
really don't want to have to change their kernel command line.

If we really need a hack for distros then I'd suggest we add a global struct
driver * in the powerpc iommu code, and then when nvme loads it sets that to
point at itself, and then the check becomes:

  		if (dma_handle == DMA_ERROR_CODE) {
 			if (dev->driver == ppc_no_iommu_warn_driver && printk_ratelimit()) {
  				dev_info(dev, "iommu_alloc failed, tbl %p vaddr %p npages %d\n", tbl, vaddr, npages);

cheers

Re: powerpc: add kernel parameter iommu_alloc_quiet

[Mauricio Faria de Oliveira]

Hi Michael,

Thanks for the review.

On 09/21/2016 10:51 AM, Michael Ellerman wrote:
>> That is important/requirement for the distribution kernels - where
>> the DMA_ATTR_NO_WARN changes to 'enum dma_attr' are not acceptable
>> because it breaks the kernel ABI.
[snip]
 > Removing an entry from an enum would break the API (Note _P_). But I 
don't see
 > how adding one does.

Agree.. I should have worded 'already-built out-of-tree modules'.

If I understand it correctly, this chunk will change the value of
DMA_ATTR_MAX, and thus break the ABI for out-of-tree modules that
depend on it. (Not that I know of any that use it, but that's the
reason that causes distro kernel builds to fail w/ this applied.)

@@ -19,6 +19,7 @@ enum dma_attr {
  	DMA_ATTR_SKIP_CPU_SYNC,
  	DMA_ATTR_FORCE_CONTIGUOUS,
  	DMA_ATTR_ALLOC_SINGLE_PAGES,
+	DMA_ATTR_NO_WARN,
  	DMA_ATTR_MAX,
  };

> Also kernel command line parameters are really poor in terms of usability. Folks
> really don't want to have to change their kernel command line.
>
> If we really need a hack for distros then I'd suggest we add a global struct
> driver * in the powerpc iommu code, and then when nvme loads it sets that to
> point at itself, and then the check becomes:

Agree w/ cmdline args are poor for usability, and that this new hack is
certainly smarter -- however, it does not provide any option/choice for
the user of whether enable/disable it (ie driver automatically sets it).

Although that isn't a problem for older downstream nvme drivers, which
have a single OK-to-fail dma mapping callsite (so the message doesn't
matter at all), the newer downstream drivers also have a Not-OK-to-fail
callsite, which is still worth to get the 'iommu_alloc failed' message
(per Ben's point of 'message useful for debugging').

For the latter, I think an upstream patch like this suggestion is not
a generally acceptable approach.

So, the intent is to have a single/common hack that upstream is OK w/,
then apply that downstream in the multiple distros.  Of course, if you
are not OK w/ this patch (which is obviously fair) we'll have to try it
downstream only, and there we can diverge in the implementations.

Please let me know your thoughts on it.

Thanks!

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: powerpc: add kernel parameter iommu_alloc_quiet

[Mauricio Faria de Oliveira]

Hi Michael,

On 09/21/2016 11:34 AM, Mauricio Faria de Oliveira wrote:
> So, the intent is to have a single/common hack that upstream is OK w/,
> then apply that downstream in the multiple distros.  Of course, if you
> are not OK w/ this patch (which is obviously fair) we'll have to try it
> downstream only, and there we can diverge in the implementations.
>
> Please let me know your thoughts on it.

Thanks!

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center