From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <1454079412.2576.33.camel@hpe.com> Subject: Re: [PATCH 1/2] x86/lib/copy_user_64.S: Handle 4-byte uncached copy From: Toshi Kani Date: Fri, 29 Jan 2016 07:56:52 -0700 In-Reply-To: <20160129082756.GA4326@gmail.com> References: <1454004770-6318-1-git-send-email-toshi.kani@hpe.com> <1454004770-6318-2-git-send-email-toshi.kani@hpe.com> <20160129082756.GA4326@gmail.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org To: Ingo Molnar Cc: tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, bp@suse.de, dan.j.williams@intel.com, ross.zwisler@linux.intel.com, vishal.l.verma@intel.com, micah.parrish@hpe.com, brian.boylston@hpe.com, x86@kernel.org, linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, Andy Lutomirski , Denys Vlasenko List-ID: On Fri, 2016-01-29 at 09:27 +0100, Ingo Molnar wrote: > * Toshi Kani wrote: : > > --- > > arch/x86/lib/copy_user_64.S | 44 ++++++++++++++++++++++++++++++++--- > > -------- > > 1 file changed, 33 insertions(+), 11 deletions(-) > > > > diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S > > index 982ce34..84b5578 100644 > > --- a/arch/x86/lib/copy_user_64.S > > +++ b/arch/x86/lib/copy_user_64.S > > @@ -232,12 +232,17 @@ ENDPROC(copy_user_enhanced_fast_string) > > > > /* > > * copy_user_nocache - Uncached memory copy with exception handling > > - * This will force destination/source out of cache for more > > performance. > > + * This will force destination out of cache for more performance. > > + * > > + * Note: Cached memory copy is used when destination or size is not > > + * naturally aligned. That is: > > + * - Require 8-byte alignment when size is 8 bytes or larger. > > + * - Require 4-byte alignment when size is 4 bytes. > > */ > > ENTRY(__copy_user_nocache) : > So at minimum this patch needs to add quite a few comments to explain the > alignment dependent control flow. > > Assembly code is hard enough to read as-is. Adding 20 more lines with > zero in-line comments is a mistake. > > Btw., while at it, please add comments for the control flow of the whole > function. Above a certain complexity that is a must for assembly > functions. Agreed. I will add comments for the whole function. Thanks, -Toshi From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756361AbcA2O54 (ORCPT ); Fri, 29 Jan 2016 09:57:56 -0500 Received: from g9t5009.houston.hp.com ([15.240.92.67]:47933 "EHLO g9t5009.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756322AbcA2O5z (ORCPT ); Fri, 29 Jan 2016 09:57:55 -0500 Message-ID: <1454079412.2576.33.camel@hpe.com> Subject: Re: [PATCH 1/2] x86/lib/copy_user_64.S: Handle 4-byte uncached copy From: Toshi Kani To: Ingo Molnar Cc: tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, bp@suse.de, dan.j.williams@intel.com, ross.zwisler@linux.intel.com, vishal.l.verma@intel.com, micah.parrish@hpe.com, brian.boylston@hpe.com, x86@kernel.org, linux-nvdimm@ml01.01.org, linux-kernel@vger.kernel.org, Andy Lutomirski , Denys Vlasenko Date: Fri, 29 Jan 2016 07:56:52 -0700 In-Reply-To: <20160129082756.GA4326@gmail.com> References: <1454004770-6318-1-git-send-email-toshi.kani@hpe.com> <1454004770-6318-2-git-send-email-toshi.kani@hpe.com> <20160129082756.GA4326@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.5 (3.16.5-3.fc22) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2016-01-29 at 09:27 +0100, Ingo Molnar wrote: > * Toshi Kani wrote: : > > --- > > arch/x86/lib/copy_user_64.S | 44 ++++++++++++++++++++++++++++++++--- > > -------- > > 1 file changed, 33 insertions(+), 11 deletions(-) > > > > diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S > > index 982ce34..84b5578 100644 > > --- a/arch/x86/lib/copy_user_64.S > > +++ b/arch/x86/lib/copy_user_64.S > > @@ -232,12 +232,17 @@ ENDPROC(copy_user_enhanced_fast_string) > > > > /* > > * copy_user_nocache - Uncached memory copy with exception handling > > - * This will force destination/source out of cache for more > > performance. > > + * This will force destination out of cache for more performance. > > + * > > + * Note: Cached memory copy is used when destination or size is not > > + * naturally aligned. That is: > > + * - Require 8-byte alignment when size is 8 bytes or larger. > > + * - Require 4-byte alignment when size is 4 bytes. > > */ > > ENTRY(__copy_user_nocache) : > So at minimum this patch needs to add quite a few comments to explain the > alignment dependent control flow. > > Assembly code is hard enough to read as-is. Adding 20 more lines with > zero in-line comments is a mistake. > > Btw., while at it, please add comments for the control flow of the whole > function. Above a certain complexity that is a must for assembly > functions. Agreed. I will add comments for the whole function. Thanks, -Toshi