Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1230950
| From | Christophe Leroy <christophe.leroy@c-s.fr> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline |
| Date | 2015-09-22 23:00 +0200 |
| Message-ID | <qbCPo-8bs-25@gated-at.bofh.it> (permalink) |
| References | (6 earlier) <qbBTl-6Qf-41@gated-at.bofh.it> <qbCw1-7OJ-17@gated-at.bofh.it> <qbCw1-7OJ-19@gated-at.bofh.it> <qbCw1-7OJ-21@gated-at.bofh.it> <qbCw1-7OJ-15@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Le 22/09/2015 22:38, Joakim Tjernlund a écrit :
> 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 <christophe.leroy@c-s.fr>
>>>>>>>>>> ---
>>>>>>>>>> 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.
>
Here is what I get in asm. First one is with "if (i) mb();". We see gcc
puts a beqlr. This is the form that is closest to what we had in the
former misc_32.S
Second one if with "mb()". Here we get a branch to sync for a useless sync
c000e0ac <my_flush_dcache_range1>:
c000e0ac: 54 63 00 36 rlwinm r3,r3,0,0,27
c000e0b0: 38 84 00 0f addi r4,r4,15
c000e0b4: 7d 23 20 50 subf r9,r3,r4
c000e0b8: 55 29 e1 3f rlwinm. r9,r9,28,4,31
c000e0bc: 4d 82 00 20 beqlr
c000e0c0: 7d 29 03 a6 mtctr r9
c000e0c4: 7c 00 18 6c dcbst 0,r3
c000e0c8: 38 63 00 10 addi r3,r3,16
c000e0cc: 42 00 ff f8 bdnz c000e0c4
<my_flush_dcache_range1+0x18>
c000e0d0: 7c 00 04 ac sync
c000e0d4: 4e 80 00 20 blr
c000e0d8 <my_flush_dcache_range2>:
c000e0d8: 54 63 00 36 rlwinm r3,r3,0,0,27
c000e0dc: 38 84 00 0f addi r4,r4,15
c000e0e0: 7d 23 20 50 subf r9,r3,r4
c000e0e4: 55 29 e1 3f rlwinm. r9,r9,28,4,31
c000e0e8: 41 82 00 14 beq c000e0fc
<my_flush_dcache_range2+0x24>
c000e0ec: 7d 29 03 a6 mtctr r9
c000e0f0: 7c 00 18 6c dcbst 0,r3
c000e0f4: 38 63 00 10 addi r3,r3,16
c000e0f8: 42 00 ff f8 bdnz c000e0f0
<my_flush_dcache_range2+0x18>
c000e0fc: 7c 00 04 ac sync
c000e100: 4e 80 00 20 blr
Christophe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Christophe Leroy <christophe.leroy@c-s.fr> - 2015-09-22 19:00 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Joakim Tjernlund <joakim.tjernlund@transmode.se> - 2015-09-22 20:20 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Scott Wood <scottwood@freescale.com> - 2015-09-22 21:00 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Joakim Tjernlund <joakim.tjernlund@transmode.se> - 2015-09-22 21:40 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Joakim Tjernlund <joakim.tjernlund@transmode.se> - 2015-09-22 22:00 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Joakim Tjernlund <joakim.tjernlund@transmode.se> - 2015-09-22 22:10 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Joakim Tjernlund <joakim.tjernlund@transmode.se> - 2015-09-22 22:40 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Christophe Leroy <christophe.leroy@c-s.fr> - 2015-09-22 23:00 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Christophe Leroy <christophe.leroy@c-s.fr> - 2015-09-23 00:50 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Joakim Tjernlund <joakim.tjernlund@transmode.se> - 2015-09-22 22:40 +0200
Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline Scott Wood <scottwood@freescale.com> - 2015-09-29 02:30 +0200
csiph-web