Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320424 > unrolled thread

Re: [PATCH v2] x86/mm: avoid premature success when changing page attributes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] x86/mm: avoid premature success when changing page  attributes Ingo Molnar <mingo@kernel.org> - 2016-01-28 09:50 +0100
    Re: [PATCH v2] x86/mm: avoid premature success when changing  page attributes "Jan Beulich" <JBeulich@suse.com> - 2016-02-02 09:50 +0100

#1320424 — Re: [PATCH v2] x86/mm: avoid premature success when changing page attributes

* Jan Beulich <JBeulich@suse.com> wrote:

> When __change_page_attr() finds it necessary to call
> __cpa_process_fault(), it passes its return value directly up to its
> own caller, even if this indicates success. Success to the callers,
> however, means that whatever ->numpages currently holds is the count
> of successfully processed pages. The cases when __change_page_attr()
> calls __cpa_process_fault(), otoh, don't generally mean the entire
> range got processed (as can be seen from one of the two success return
> paths in __cpa_process_fault() adjusting ->numpages).
> 
> When a top level caller, like in the case of change_page_attr_set_clr()
> only meaning to alter _PAGE_NX, wants to suppress alias processing, the
> boolean value to indicate so results in __cpa_process_fault() taking
> its other successful exit path. Since ->numpages so far didn't get
> adjusted there, hitting either of the conditions that cause
> __cpa_process_fault() to get called meant early termination of the
> processing without having processed the entire range, yet still
> reporting success.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Completely re-written description.

So maybe it's just me, but I'm still quite unhappy about this changelog, it's hard 
to parse and doesn't really do what a good changelog should do :-/

First I'd like to quote from a mail of Andrew Morton:

 "Please update the changelog to describe the current behavior.

  Please also describe why you think that behavior should be changed.
  ie: what's the reason for this patch.

  Please update Documentation/ for this feature.  Probably that's
  kernel-parameters.txt for the boot option and sysctl/kernel.txt for the
  procfs addition."

Alternatively:

 1- first describe the symptoms of the bug - how does a user notice? 

 2- then describe how the code behaves today and how that is causing the bug

 3- and then only describe how it's fixed.

Or:

  " Current code does (A), this has a problem when (B).
    We can improve this doing (C), because (D)."

This changelog concentrates excessively on implementational details, without 
providing context and without touching upon the practical effects - nor does it do 
a clear before/after description.

I.e. what you describe in the changelog is 90% of what a developer intimate with 
this code finds interesting about the patch - but that's not what good changelogs 
are about!

Could we try a v3?

Thanks,

        Ingo

[toc] | [next] | [standalone]

#1323886 — Re: [PATCH v2] x86/mm: avoid premature success when changing page attributes

>>> On 28.01.16 at 09:42, <mingo@kernel.org> wrote:
> Could we try a v3?

Okay, I withdraw the patch: Upon further consideration it is note
really clear what the intended behavior of set_memory_*() on
address ranges with mapping holes is supposed to be. The original
issue was with set_memory_nx() (called from mark_rodata_ro())
stumbling across an unmapped region (resulting from an out of
tree change completely unmapping the kernel mappings of
address ranges passed to free_init_pages()). I simply don't have
the time to check whether the unmapping done with
CONFIG_DEBUG_PAGEALLOC would have a similar effect. The
net result in any event were pages (past the hole) reported as
problematic when CONFIG_DEBUG_WX is enabled.

Jan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web