| Message ID | alpine.LFD.2.02.1211061610070.21033@xanadu.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 06, 2012 at 09:12:27PM +0000, Nicolas Pitre wrote: > > ... instead of flush_cache_all(). The later unconditionally flushes > the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which > is costly and unnecessary in some scenarios where setup_mm_for_reboot() > is used. If L2 has to be flushed as well, it should already be done > separately on other architectures anyway. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c > index ab88ed4f8e..2c61085a10 100644 > --- a/arch/arm/mm/idmap.c > +++ b/arch/arm/mm/idmap.c > @@ -104,7 +104,7 @@ early_initcall(init_static_idmap); > void setup_mm_for_reboot(void) > { > /* Clean and invalidate L1. */ > - flush_cache_all(); > + flush_cache_louis(); > > /* Switch to the identity mapping. */ > cpu_switch_mm(idmap_pgd, &init_mm); We could probably even predicate this on the half-implemented TLB_CAN_READ_FROM_L1_CACHE option, but that can come later. Acked-by: Will Deacon <will.deacon@arm.com> Will
On Tuesday 06 November 2012 03:12 PM, Nicolas Pitre wrote: > > ... instead of flush_cache_all(). The later unconditionally flushes > the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which > is costly and unnecessary in some scenarios where setup_mm_for_reboot() > is used. If L2 has to be flushed as well, it should already be done > separately on other architectures anyway. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c > index ab88ed4f8e..2c61085a10 100644 > --- a/arch/arm/mm/idmap.c > +++ b/arch/arm/mm/idmap.c > @@ -104,7 +104,7 @@ early_initcall(init_static_idmap); > void setup_mm_for_reboot(void) > { > /* Clean and invalidate L1. */ > - flush_cache_all(); > + flush_cache_louis(); > > /* Switch to the identity mapping. */ > cpu_switch_mm(idmap_pgd, &init_mm); > Nice. Just one difference is that the I-cache invalidation won't happen with this change. Not that it is needed here but capturing that in change-log would be good. Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote: > > ... instead of flush_cache_all(). The later unconditionally flushes > the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which > is costly and unnecessary in some scenarios where setup_mm_for_reboot() > is used. If L2 has to be flushed as well, it should already be done > separately on other architectures anyway. Why does the cost at reboot count? It's a relatively slow operation as it is anyway, because you have to wait for the system to shut down, call the boot loader, etc. However, the opposite argument is that the state of the L2 _shouldn't_ matter - except for one small little detail. Dirty data, which could get evicted and overwrite something that matters. Generally there won't be any dirty data in the L2 cache on normal boot, so this is a situation which boot loaders probably don't expect. So all in all, I'm not sure of the wiseness of your change. It's likely to cause regressions.
On Tue, 6 Nov 2012, Russell King - ARM Linux wrote: > On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote: > > > > ... instead of flush_cache_all(). The later unconditionally flushes > > the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which > > is costly and unnecessary in some scenarios where setup_mm_for_reboot() > > is used. If L2 has to be flushed as well, it should already be done > > separately on other architectures anyway. > > Why does the cost at reboot count? It's a relatively slow operation as > it is anyway, because you have to wait for the system to shut down, call > the boot loader, etc. Because I have a use case with the big.LITTLE switcher where the full boot is bypassed but the kernel must be reintered as if the CPU was powered up. This is of course something that _could_ happen multiple times in a second, and therefore minimizing its unneeded costs is a good thing(tm). > However, the opposite argument is that the state of the L2 _shouldn't_ > matter - except for one small little detail. Dirty data, which could > get evicted and overwrite something that matters. Generally there won't > be any dirty data in the L2 cache on normal boot, so this is a situation > which boot loaders probably don't expect. In the use case that concerns me, L2 is retained and I'd well prefer if it didn't get flushed at all. > So all in all, I'm not sure of the wiseness of your change. It's likely > to cause regressions. Could you please tell me if you have such a regression in mind? Of course I could carry the few operations performed by setup_mm_for_reboot() locally, but that looks like useless code duplication. Nicolas
On Tue, Nov 06, 2012 at 09:57:17PM +0000, Santosh Shilimkar wrote: > On Tuesday 06 November 2012 03:12 PM, Nicolas Pitre wrote: > > > > ... instead of flush_cache_all(). The later unconditionally flushes > > the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which > > is costly and unnecessary in some scenarios where setup_mm_for_reboot() > > is used. If L2 has to be flushed as well, it should already be done > > separately on other architectures anyway. > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > > diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c > > index ab88ed4f8e..2c61085a10 100644 > > --- a/arch/arm/mm/idmap.c > > +++ b/arch/arm/mm/idmap.c > > @@ -104,7 +104,7 @@ early_initcall(init_static_idmap); > > void setup_mm_for_reboot(void) > > { > > /* Clean and invalidate L1. */ > > - flush_cache_all(); > > + flush_cache_louis(); > > > > /* Switch to the identity mapping. */ > > cpu_switch_mm(idmap_pgd, &init_mm); > > > Nice. Just one difference is that the I-cache invalidation won't > happen with this change. Not that it is needed here but capturing > that in change-log would be good. Yes, it does happen. The LoUIS API mirrors the flush_cache_all() API in this respect, and it has to. The only change is the data cache level at which it operates. Lorenzo
On Tue, Nov 06, 2012 at 10:04:58PM +0000, Russell King - ARM Linux wrote: > On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote: > > > > ... instead of flush_cache_all(). The later unconditionally flushes > > the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which > > is costly and unnecessary in some scenarios where setup_mm_for_reboot() > > is used. If L2 has to be flushed as well, it should already be done > > separately on other architectures anyway. > > Why does the cost at reboot count? It's a relatively slow operation as > it is anyway, because you have to wait for the system to shut down, call > the boot loader, etc. > > However, the opposite argument is that the state of the L2 _shouldn't_ > matter - except for one small little detail. Dirty data, which could > get evicted and overwrite something that matters. Generally there won't > be any dirty data in the L2 cache on normal boot, so this is a situation > which boot loaders probably don't expect. Wouldn't the L2 flush in this case be included with the code that turns off caching? For reboot, that's currently done in __sort_restart -- the cache flush in setup_mm_for_reboot is just to ensure that the idmap tables are visible to the hardware walker iirc. Will
On Tue, Nov 06, 2012 at 05:53:20PM -0500, Nicolas Pitre wrote: > On Tue, 6 Nov 2012, Russell King - ARM Linux wrote: > > > On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote: > > > > > > ... instead of flush_cache_all(). The later unconditionally flushes > > > the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which > > > is costly and unnecessary in some scenarios where setup_mm_for_reboot() > > > is used. If L2 has to be flushed as well, it should already be done > > > separately on other architectures anyway. > > > > Why does the cost at reboot count? It's a relatively slow operation as > > it is anyway, because you have to wait for the system to shut down, call > > the boot loader, etc. > > Because I have a use case with the big.LITTLE switcher where the full > boot is bypassed but the kernel must be reintered as if the CPU was > powered up. This is of course something that _could_ happen multiple > times in a second, and therefore minimizing its unneeded costs is a good > thing(tm). > > > However, the opposite argument is that the state of the L2 _shouldn't_ > > matter - except for one small little detail. Dirty data, which could > > get evicted and overwrite something that matters. Generally there won't > > be any dirty data in the L2 cache on normal boot, so this is a situation > > which boot loaders probably don't expect. > > In the use case that concerns me, L2 is retained and I'd well prefer if > it didn't get flushed at all. > > > So all in all, I'm not sure of the wiseness of your change. It's likely > > to cause regressions. > > Could you please tell me if you have such a regression in mind? Of > course I could carry the few operations performed by > setup_mm_for_reboot() locally, but that looks like useless code > duplication. I think I included all the relevant information in the original email.
On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote: > Wouldn't the L2 flush in this case be included with the code that turns off > caching? For reboot, that's currently done in __sort_restart -- the cache > flush in setup_mm_for_reboot is just to ensure that the idmap tables are > visible to the hardware walker iirc. Good point - but it does raise another issue. Why do we do this flush and TLB invalidate afterwards in setup_mm_for_reboot() ? It makes sense if we change existing page tables, but we don't anymore, we're just switching them, and cpu_switch_mm() will do whatever's necessary to make the new page tables visible. So I think we can get rid of that flusing in there.
On Wed, Nov 07, 2012 at 09:56:35AM +0000, Russell King - ARM Linux wrote: > On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote: > > Wouldn't the L2 flush in this case be included with the code that turns off > > caching? For reboot, that's currently done in __sort_restart -- the cache > > flush in setup_mm_for_reboot is just to ensure that the idmap tables are > > visible to the hardware walker iirc. > > Good point - but it does raise another issue. Why do we do this flush and > TLB invalidate afterwards in setup_mm_for_reboot() ? It makes sense if > we change existing page tables, but we don't anymore, we're just switching > them, and cpu_switch_mm() will do whatever's necessary to make the new > page tables visible. So I think we can get rid of that flusing in there. Hmm, I'm not sure about that. Looking at cpu_v7_switch_mm (the 2-level version) for example, we set the ASID and then set the new TTBR. There's no flushing of page tables like we get in set_pte and no TLB flushing either. Now, given that the idmap_pgd is populated as an early_initcall, I really doubt we need that flush_cache_all but the TLB invalidate is surely required? Will
On Wednesday 07 November 2012 03:47 AM, Lorenzo Pieralisi wrote: > On Tue, Nov 06, 2012 at 09:57:17PM +0000, Santosh Shilimkar wrote: >> On Tuesday 06 November 2012 03:12 PM, Nicolas Pitre wrote: >>> >>> ... instead of flush_cache_all(). The later unconditionally flushes >>> the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which >>> is costly and unnecessary in some scenarios where setup_mm_for_reboot() >>> is used. If L2 has to be flushed as well, it should already be done >>> separately on other architectures anyway. >>> >>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>> >>> diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c >>> index ab88ed4f8e..2c61085a10 100644 >>> --- a/arch/arm/mm/idmap.c >>> +++ b/arch/arm/mm/idmap.c >>> @@ -104,7 +104,7 @@ early_initcall(init_static_idmap); >>> void setup_mm_for_reboot(void) >>> { >>> /* Clean and invalidate L1. */ >>> - flush_cache_all(); >>> + flush_cache_louis(); >>> >>> /* Switch to the identity mapping. */ >>> cpu_switch_mm(idmap_pgd, &init_mm); >>> >> Nice. Just one difference is that the I-cache invalidation won't >> happen with this change. Not that it is needed here but capturing >> that in change-log would be good. > > Yes, it does happen. The LoUIS API mirrors the flush_cache_all() API in > this respect, and it has to. The only change is the data cache level at > which it operates. > Indeed. I remember our discussion on this part now. Thanks Lorenzo for clarification. Regards Santosh
diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c index ab88ed4f8e..2c61085a10 100644 --- a/arch/arm/mm/idmap.c +++ b/arch/arm/mm/idmap.c @@ -104,7 +104,7 @@ early_initcall(init_static_idmap); void setup_mm_for_reboot(void) { /* Clean and invalidate L1. */ - flush_cache_all(); + flush_cache_louis(); /* Switch to the identity mapping. */ cpu_switch_mm(idmap_pgd, &init_mm);
... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway. Signed-off-by: Nicolas Pitre <nico@linaro.org>