From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967101AbcA1Iml (ORCPT ); Thu, 28 Jan 2016 03:42:41 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36644 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934121AbcA1Imh (ORCPT ); Thu, 28 Jan 2016 03:42:37 -0500 Date: Thu, 28 Jan 2016 09:42:32 +0100 From: Ingo Molnar To: Jan Beulich Cc: mingo@elte.hu, Thomas Gleixner , hpa@zytor.com, linux-kernel@vger.kernel.org, Peter Zijlstra Subject: Re: [PATCH v2] x86/mm: avoid premature success when changing page attributes Message-ID: <20160128084232.GA24464@gmail.com> References: <56A6615E02000078000CAC96@prv-mh.provo.novell.com> <56A8AD9E02000078000CB7CA@prv-mh.provo.novell.com> <56A8B64A02000078000CB830@prv-mh.provo.novell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56A8B64A02000078000CB830@prv-mh.provo.novell.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Jan Beulich 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 > --- > 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