| Message ID | 20190205202133.5048-1-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] powerpc/mm: move a KERN_WARNING message to pr_debug() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
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); > } > } >
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
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
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
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); } }
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(-)