[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160128084232.GA24464@gmail.com>
Date: Thu, 28 Jan 2016 09:42:32 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Jan Beulich <JBeulich@...e.com>
Cc: mingo@...e.hu, Thomas Gleixner <tglx@...utronix.de>, hpa@...or.com,
linux-kernel@...r.kernel.org,
Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH v2] x86/mm: avoid premature success when changing page
attributes
* Jan Beulich <JBeulich@...e.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@...e.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
Powered by blists - more mailing lists