[PATCH v2] powerpc/mm: move a KERN_WARNING message to pr_debug()

[PATCH v2] powerpc/mm: move a KERN_WARNING message to pr_debug()

[Laurent Vivier]

resize_hpt_for_hotplug() reports a warning when it cannot
increase the hash page table ("Unable to resize hash page
table to target order") but this is not blocking and
can make user thinks something has not worked properly.
As we move the message to the debug area, report again the
ENODEV error.

If the operation cannot be done the real error message
will be reported by arch_add_memory() if create_section_mapping()
fails.

Fixes: 7339390d772dd
       powerpc/pseries: Don't give a warning when HPT resizing isn't available
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---

Notes:
    v2:
     - use pr_debug instead of printk(KERN_DEBUG
     - remove check for ENODEV

 arch/powerpc/mm/hash_utils_64.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index 0cc7fbc3bd1c..6a0cc4eb2c83 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -777,10 +777,9 @@ void resize_hpt_for_hotplug(unsigned long new_mem_size)
 		int rc;
 
 		rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
-		if (rc && (rc != -ENODEV))
-			printk(KERN_WARNING
-			       "Unable to resize hash page table to target order %d: %d\n",
-			       target_hpt_shift, rc);
+		if (rc)
+			pr_debug("Unable to resize hash page table to target order %d: %d\n",
+				 target_hpt_shift, rc);
 	}
 }
 
-- 
2.20.1

Re: [PATCH v2] powerpc/mm: move a KERN_WARNING message to pr_debug()

[David Gibson]

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

On Tue, Feb 05, 2019 at 09:21:33PM +0100, Laurent Vivier wrote:
> resize_hpt_for_hotplug() reports a warning when it cannot
> increase the hash page table ("Unable to resize hash page
> table to target order") but this is not blocking and
> can make user thinks something has not worked properly.
> As we move the message to the debug area, report again the
> ENODEV error.
> 
> If the operation cannot be done the real error message
> will be reported by arch_add_memory() if create_section_mapping()
> fails.
> 
> Fixes: 7339390d772dd
>        powerpc/pseries: Don't give a warning when HPT resizing isn't available
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Sorry, I'm pretty dubious about this.  It's true that in the case for
which this bug was filed this is a harmless situation which deserves a
pr_debug() at most.

But that's not necessarily true in all paths leading to this message.
It will also trip if we fail to reshrink the HPT after genuinely
hotunplugging a bunch of memory, in which case failing to release
expected resources does deserve a warning.

> ---
> 
> Notes:
>     v2:
>      - use pr_debug instead of printk(KERN_DEBUG
>      - remove check for ENODEV
> 
>  arch/powerpc/mm/hash_utils_64.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index 0cc7fbc3bd1c..6a0cc4eb2c83 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -777,10 +777,9 @@ void resize_hpt_for_hotplug(unsigned long new_mem_size)
>  		int rc;
>  
>  		rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
> -		if (rc && (rc != -ENODEV))
> -			printk(KERN_WARNING
> -			       "Unable to resize hash page table to target order %d: %d\n",
> -			       target_hpt_shift, rc);
> +		if (rc)
> +			pr_debug("Unable to resize hash page table to target order %d: %d\n",
> +				 target_hpt_shift, rc);
>  	}
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] powerpc/mm: move a KERN_WARNING message to pr_debug()

[Michael Ellerman]

Hi Laurent,

I'm not sure I'm convinced about this one. It seems like we're just
throwing away the warning because it's annoying.

Laurent Vivier <lvivier@redhat.com> writes:
> resize_hpt_for_hotplug() reports a warning when it cannot
> increase the hash page table ("Unable to resize hash page
> table to target order") but this is not blocking and
> can make user thinks something has not worked properly.

Something did not work properly, the resize didn't work properly. Right?

> As we move the message to the debug area, report again the
> ENODEV error.
>
> If the operation cannot be done the real error message
> will be reported by arch_add_memory() if create_section_mapping()
> fails.

Can you explain that more. Isn't the fact that the resize failed "the
real error message"?


> Fixes: 7339390d772dd
>        powerpc/pseries: Don't give a warning when HPT resizing isn't available

This should all be on one line, and formatted as:

Fixes: 7339390d772d ("powerpc/pseries: Don't give a warning when HPT resizing isn't available")

See Documentation/process/submitting-patches.rst for more info and how
to configure git to do it automatically for you.

cheers

Re: [PATCH v2] powerpc/mm: move a KERN_WARNING message to pr_debug()

[Laurent Vivier]

On 07/02/2019 05:33, Michael Ellerman wrote:
> Hi Laurent,
> 
> I'm not sure I'm convinced about this one. It seems like we're just
> throwing away the warning because it's annoying.
> 
> Laurent Vivier <lvivier@redhat.com> writes:
>> resize_hpt_for_hotplug() reports a warning when it cannot
>> increase the hash page table ("Unable to resize hash page
>> table to target order") but this is not blocking and
>> can make user thinks something has not worked properly.
> 
> Something did not work properly, the resize didn't work properly. Right?
> 
>> As we move the message to the debug area, report again the
>> ENODEV error.
>>
>> If the operation cannot be done the real error message
>> will be reported by arch_add_memory() if create_section_mapping()
>> fails.
> 
> Can you explain that more. Isn't the fact that the resize failed "the
> real error message"?

In arch_add_memory(), after calling resize_hpt_for_hotplug() it calls
create_section_mapping() and if create_section_mapping() fails we will
have the error message "Unable to create mapping for hot added memory"
and the real exit (EFAULT).

> 
>> Fixes: 7339390d772dd
>>        powerpc/pseries: Don't give a warning when HPT resizing isn't available
> 
> This should all be on one line, and formatted as:
> 
> Fixes: 7339390d772d ("powerpc/pseries: Don't give a warning when HPT resizing isn't available")
> 
> See Documentation/process/submitting-patches.rst for more info and how
> to configure git to do it automatically for you.

Thank you, I didn't know the format was documented.

Thanks,
Laurent

Re: [PATCH v2] powerpc/mm: move a KERN_WARNING message to pr_debug()

[Laurent Vivier]

On 07/02/2019 04:03, David Gibson wrote:
> On Tue, Feb 05, 2019 at 09:21:33PM +0100, Laurent Vivier wrote:
>> resize_hpt_for_hotplug() reports a warning when it cannot
>> increase the hash page table ("Unable to resize hash page
>> table to target order") but this is not blocking and
>> can make user thinks something has not worked properly.
>> As we move the message to the debug area, report again the
>> ENODEV error.
>>
>> If the operation cannot be done the real error message
>> will be reported by arch_add_memory() if create_section_mapping()
>> fails.
>>
>> Fixes: 7339390d772dd
>>        powerpc/pseries: Don't give a warning when HPT resizing isn't available
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Sorry, I'm pretty dubious about this.  It's true that in the case for
> which this bug was filed this is a harmless situation which deserves a
> pr_debug() at most.
> 
> But that's not necessarily true in all paths leading to this message.
> It will also trip if we fail to reshrink the HPT after genuinely
> hotunplugging a bunch of memory, in which case failing to release
> expected resources does deserve a warning.

But if there is a real problem this function should return an error and
this error should be managed by the caller.

Moreover, the function that can fail (pseries_lpar_resize_hpt()) has
already a warning for each error case:

ETIMEDOUT: "Unexpected error %d cancelling timed out HPT resize\n"
EIO:       "Unexpected error %d from H_RESIZE_HPT_PREPARE\n"
           "Unexpected error %d from H_RESIZE_HPT_COMMIT\n
ENOSPC:    "Hash collision while resizing HPT\n"

ENODEV has no error message but is already silently ignored.
EINVAL and EPERM have no message but this happens if hcall is not used
correctly and deserve only a pr_debug() I think.

Thanks,
Laurent