From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934762AbbIVUiL (ORCPT ); Tue, 22 Sep 2015 16:38:11 -0400 Received: from smtp.transmode.se ([31.15.61.139]:54025 "EHLO smtp.transmode.se" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934699AbbIVUiI convert rfc822-to-8bit (ORCPT ); Tue, 22 Sep 2015 16:38:08 -0400 From: Joakim Tjernlund To: "scottwood@freescale.com" CC: "christophe.leroy@c-s.fr" , "paulus@samba.org" , "mpe@ellerman.id.au" , "benh@kernel.crashing.org" , "linux-kernel@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" Subject: Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Thread-Topic: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Thread-Index: AQHQ9VspxrgpWLe1w0WBxWBczXIu3p5IuDyAgAANAICAAAnwgIAAAhwAgAAD2QCAAAUfAIAABTaAgAAAm4CAAADbgA== Date: Tue, 22 Sep 2015 20:38:05 +0000 Message-ID: <1442954285.29498.79.camel@transmode.se> References: <1442945547.29498.50.camel@transmode.se> <1442948339.19102.270.camel@freescale.com> <1442950473.29498.54.camel@transmode.se> <1442950926.19102.280.camel@freescale.com> <1442951752.29498.58.camel@transmode.se> <1442952852.19102.281.camel@freescale.com> <1442953971.29498.76.camel@transmode.se> <1442954101.19102.286.camel@freescale.com> In-Reply-To: <1442954101.19102.286.camel@freescale.com> Accept-Language: en-US, sv-SE Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-mailer: Evolution 3.16.5 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [192.168.200.4] Content-Type: text/plain; charset="iso-8859-15" Content-ID: <6DA9034EEEB5984ABEF942DAD7E3A64B@transmode.com> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2015-09-22 at 15:35 -0500, Scott Wood wrote: > On Tue, 2015-09-22 at 20:32 +0000, Joakim Tjernlund wrote: > > On Tue, 2015-09-22 at 15:14 -0500, Scott Wood wrote: > > > On Tue, 2015-09-22 at 19:55 +0000, Joakim Tjernlund wrote: > > > > On Tue, 2015-09-22 at 14:42 -0500, Scott Wood wrote: > > > > > On Tue, 2015-09-22 at 19:34 +0000, Joakim Tjernlund wrote: > > > > > > On Tue, 2015-09-22 at 13:58 -0500, Scott Wood wrote: > > > > > > > On Tue, 2015-09-22 at 18:12 +0000, Joakim Tjernlund wrote: > > > > > > > > On Tue, 2015-09-22 at 18:51 +0200, Christophe Leroy wrote: > > > > > > > > > flush/clean/invalidate _dcache_range() functions are all very > > > > > > > > > similar and are quite short. They are mainly used in > > > > > > > > > __dma_sync() > > > > > > > > > perf_event locate them in the top 3 consumming functions > > > > > > > > > during > > > > > > > > > heavy ethernet activity > > > > > > > > > > > > > > > > > > They are good candidate for inlining, as __dma_sync() does > > > > > > > > > almost nothing but calling them > > > > > > > > > > > > > > > > > > Signed-off-by: Christophe Leroy > > > > > > > > > --- > > > > > > > > > New in v2 > > > > > > > > > > > > > > > > > > arch/powerpc/include/asm/cacheflush.h | 55 > > > > > > > > > +++++++++++++++++++++++++++-- > > > > > > > > > arch/powerpc/kernel/misc_32.S | 65 ------------------ > > > > > > > > > ---- > > > > > > > > > ---- > > > > > > > > > ---- > > > > > > > > > ----- > > > > > > > > > arch/powerpc/kernel/ppc_ksyms.c | 2 ++ > > > > > > > > > 3 files changed, 54 insertions(+), 68 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/arch/powerpc/include/asm/cacheflush.h > > > > > > > > > b/arch/powerpc/include/asm/cacheflush.h > > > > > > > > > index 6229e6b..6169604 100644 > > > > > > > > > --- a/arch/powerpc/include/asm/cacheflush.h > > > > > > > > > +++ b/arch/powerpc/include/asm/cacheflush.h > > > > > > > > > @@ -47,12 +47,61 @@ static inline void > > > > > > > > > __flush_dcache_icache_phys(unsigned long physaddr) > > > > > > > > > } > > > > > > > > > #endif > > > > > > > > > > > > > > > > > > -extern void flush_dcache_range(unsigned long start, unsigned > > > > > > > > > long > > > > > > > > > stop); > > > > > > > > > #ifdef CONFIG_PPC32 > > > > > > > > > -extern void clean_dcache_range(unsigned long start, unsigned > > > > > > > > > long > > > > > > > > > stop); > > > > > > > > > -extern void invalidate_dcache_range(unsigned long start, > > > > > > > > > unsigned > > > > > > > > > long > > > > > > > > > stop); > > > > > > > > > +/* > > > > > > > > > + * Write any modified data cache blocks out to memory and > > > > > > > > > invalidate > > > > > > > > > them. > > > > > > > > > + * Does not invalidate the corresponding instruction cache > > > > > > > > > blocks. > > > > > > > > > + */ > > > > > > > > > +static inline void flush_dcache_range(unsigned long start, > > > > > > > > > unsigned > > > > > > > > > long > > > > > > > > > stop) > > > > > > > > > +{ > > > > > > > > > + void *addr = (void *)(start & ~(L1_CACHE_BYTES - 1)); > > > > > > > > > + unsigned int size = stop - (unsigned long)addr + > > > > > > > > > (L1_CACHE_BYTES - > > > > > > > > > 1); > > > > > > > > > + unsigned int i; > > > > > > > > > + > > > > > > > > > + for (i = 0; i < size >> L1_CACHE_SHIFT; i++, addr += > > > > > > > > > L1_CACHE_BYTES) > > > > > > > > > + dcbf(addr); > > > > > > > > > + if (i) > > > > > > > > > + mb(); /* sync */ > > > > > > > > > +} > > > > > > > > > > > > > > > > This feels optimized for the uncommon case when there is no > > > > > > > > invalidation. > > > > > > > > > > > > > > If you mean the "if (i)", yes, that looks odd. > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > > > > > > > > I THINK it would be better to bail early > > > > > > > > > > > > > > Bail under what conditions? > > > > > > > > > > > > test for "i = 0" and return. > > > > > > > > > > Why bother? > > > > > > > > I usally find it better to dela with special cases upfront så the rest > > > > doesn't need to > > > > bother. i=0 is a NOP and it is clearer to show that upfront. > > > > > > No, I mean why bother special casing this at all? > > > > I just said why? > > You to found the if(i) mb() a bit odd and it took a little time to see why > > it was there. > > Ahh, you mean just skip the if(i) and have mb() unconditionally? > > Yes. > > > That changes the semantics a little from the ASM version but perhaps that > > doesn't matter? > > Adding more barriers than strictly necessary is a performance concern, not a > semantic change. Semantics :) > How often are we really calling this function over an empty > range? Never hopefully, it does not make much sense. > > Not that it matters much one way or another... probably not.