[PATCH 1/2] powerpc/mm: Flush cache on memory hot(un)plug

[PATCH 1/2] powerpc/mm: Flush cache on memory hot(un)plug

[Balbir Singh]

This patch adds support for flushing potentially dirty
cache lines when memory is hot-plugged/hot-un-plugged.
The support is currently limited to 64 bit systems.

The bug was exposed when mappings for a device were
actually hot-unplugged and plugged in back later.
A similar issue was observed during the development
of memtrace, but memtrace does it's own flushing of
region via a custom routine.

These patches do a flush both on hotplug/unplug to
clear any stale data in the cache w.r.t mappings,
there is a small race window where a clean cache
line may be created again just prior to tearing
down the mapping.

The patches were tested by disabling the flush
routines in memtrace and doing I/O on the trace
file. The system immediately checkstops (quite
reliablly if prior to the hot-unplug of the memtrace
region, we memset the regions we are about to
hot unplug). After these patches no custom flushing
is needed in the memtrace code.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/mem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 85245ef97e72..0a8959b15b39 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -143,6 +143,7 @@ int __meminit arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *
 			start, start + size, rc);
 		return -EFAULT;
 	}
+	flush_inval_dcache_range(start, start + size);
 
 	return __add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
 }
@@ -169,6 +170,7 @@ int __meminit arch_remove_memory(u64 start, u64 size, struct vmem_altmap *altmap
 
 	/* Remove htab bolted mappings for this section of memory */
 	start = (unsigned long)__va(start);
+	flush_inval_dcache_range(start, start + size);
 	ret = remove_section_mapping(start, start + size);
 
 	/* Ensure all vmalloc mappings are flushed in case they also
-- 
2.13.6

[PATCH 2/2] powerpc/mm/memtrace: Let the arch hotunplug code flush cache

[Balbir Singh]

Don't do this via custom code, instead now that we have support
in the arch hotplug/hotunplug code, rely on those routines
to do the right thing.

Fixes: 9d5171a8f248 ("powerpc/powernv: Enable removal of memory for in memory tracing")
because the older code uses ppc64_caches.l1d.size instead of
ppc64_caches.l1d.line_size

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/platforms/powernv/memtrace.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index de470caf0784..fc222a0c2ac4 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -82,19 +82,6 @@ static const struct file_operations memtrace_fops = {
 	.open	= simple_open,
 };
 
-static void flush_memory_region(u64 base, u64 size)
-{
-	unsigned long line_size = ppc64_caches.l1d.size;
-	u64 end = base + size;
-	u64 addr;
-
-	base = round_down(base, line_size);
-	end = round_up(end, line_size);
-
-	for (addr = base; addr < end; addr += line_size)
-		asm volatile("dcbf 0,%0" : "=r" (addr) :: "memory");
-}
-
 static int check_memblock_online(struct memory_block *mem, void *arg)
 {
 	if (mem->state != MEM_ONLINE)
@@ -132,10 +119,6 @@ static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
 	walk_memory_range(start_pfn, end_pfn, (void *)MEM_OFFLINE,
 			  change_memblock_state);
 
-	/* RCU grace period? */
-	flush_memory_region((u64)__va(start_pfn << PAGE_SHIFT),
-			    nr_pages << PAGE_SHIFT);
-
 	lock_device_hotplug();
 	remove_memory(nid, start_pfn << PAGE_SHIFT, nr_pages << PAGE_SHIFT);
 	unlock_device_hotplug();
-- 
2.13.6

Re: [PATCH 2/2] powerpc/mm/memtrace: Let the arch hotunplug code flush cache

[rashmica]

On 06/04/18 15:24, Balbir Singh wrote:
> Don't do this via custom code, instead now that we have support
> in the arch hotplug/hotunplug code, rely on those routines
> to do the right thing.
>
> Fixes: 9d5171a8f248 ("powerpc/powernv: Enable removal of memory for in memory tracing")
> because the older code uses ppc64_caches.l1d.size instead of
> ppc64_caches.l1d.line_size
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>

Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>

Re: [PATCH 2/2] powerpc/mm/memtrace: Let the arch hotunplug code flush cache

[Michael Ellerman]

Balbir Singh <bsingharora@gmail.com> writes:

> Don't do this via custom code, instead now that we have support
> in the arch hotplug/hotunplug code, rely on those routines
> to do the right thing.
>
> Fixes: 9d5171a8f248 ("powerpc/powernv: Enable removal of memory for in memory tracing")

That's not really right.

This patch doesn't fix it, the previous patch did.

If I just backport this patch then it's still broken.

So I'll tag patch 1 with the above Fixes: tag and add stable, and then
this just becomes a cleanup.

OK?

cheers

> diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
> index de470caf0784..fc222a0c2ac4 100644
> --- a/arch/powerpc/platforms/powernv/memtrace.c
> +++ b/arch/powerpc/platforms/powernv/memtrace.c
> @@ -82,19 +82,6 @@ static const struct file_operations memtrace_fops = {
>  	.open	= simple_open,
>  };
>  
> -static void flush_memory_region(u64 base, u64 size)
> -{
> -	unsigned long line_size = ppc64_caches.l1d.size;
> -	u64 end = base + size;
> -	u64 addr;
> -
> -	base = round_down(base, line_size);
> -	end = round_up(end, line_size);
> -
> -	for (addr = base; addr < end; addr += line_size)
> -		asm volatile("dcbf 0,%0" : "=r" (addr) :: "memory");
> -}
> -
>  static int check_memblock_online(struct memory_block *mem, void *arg)
>  {
>  	if (mem->state != MEM_ONLINE)
> @@ -132,10 +119,6 @@ static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
>  	walk_memory_range(start_pfn, end_pfn, (void *)MEM_OFFLINE,
>  			  change_memblock_state);
>  
> -	/* RCU grace period? */
> -	flush_memory_region((u64)__va(start_pfn << PAGE_SHIFT),
> -			    nr_pages << PAGE_SHIFT);
> -
>  	lock_device_hotplug();
>  	remove_memory(nid, start_pfn << PAGE_SHIFT, nr_pages << PAGE_SHIFT);
>  	unlock_device_hotplug();
> -- 
> 2.13.6

Re: [PATCH 2/2] powerpc/mm/memtrace: Let the arch hotunplug code flush cache

[Balbir Singh]

On Wed, Apr 11, 2018 at 9:05 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
>
>> Don't do this via custom code, instead now that we have support
>> in the arch hotplug/hotunplug code, rely on those routines
>> to do the right thing.
>>
>> Fixes: 9d5171a8f248 ("powerpc/powernv: Enable removal of memory for in memory tracing")
>
> That's not really right.
>
> This patch doesn't fix it, the previous patch did.
>
> If I just backport this patch then it's still broken.
>
> So I'll tag patch 1 with the above Fixes: tag and add stable, and then
> this just becomes a cleanup.
>

Fair point, the previous one does indeed fix things. I can do a minimal backport
fixing .size to .line_size if needed.

Cheers,
Balbir

Re: [PATCH 1/2] powerpc/mm: Flush cache on memory hot(un)plug

[Reza Arbab]

On Fri, Apr 06, 2018 at 03:24:23PM +1000, Balbir Singh wrote:
>This patch adds support for flushing potentially dirty
>cache lines when memory is hot-plugged/hot-un-plugged.

Acked-by: Reza Arbab <arbab@linux.ibm.com>

-- 
Reza Arbab

Re: [PATCH 1/2] powerpc/mm: Flush cache on memory hot(un)plug

[rashmica]

On 06/04/18 15:24, Balbir Singh wrote:
> This patch adds support for flushing potentially dirty
> cache lines when memory is hot-plugged/hot-un-plugged.
> The support is currently limited to 64 bit systems.
>
> The bug was exposed when mappings for a device were
> actually hot-unplugged and plugged in back later.
> A similar issue was observed during the development
> of memtrace, but memtrace does it's own flushing of
> region via a custom routine.
>
> These patches do a flush both on hotplug/unplug to
> clear any stale data in the cache w.r.t mappings,
> there is a small race window where a clean cache
> line may be created again just prior to tearing
> down the mapping.
>
> The patches were tested by disabling the flush
> routines in memtrace and doing I/O on the trace
> file. The system immediately checkstops (quite
> reliablly if prior to the hot-unplug of the memtrace
> region, we memset the regions we are about to
> hot unplug). After these patches no custom flushing
> is needed in the memtrace code.
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>

Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>

Re: [1/2] powerpc/mm: Flush cache on memory hot(un)plug

[Michael Ellerman]

On Fri, 2018-04-06 at 05:24:23 UTC, Balbir Singh wrote:
> This patch adds support for flushing potentially dirty
> cache lines when memory is hot-plugged/hot-un-plugged.
> The support is currently limited to 64 bit systems.
> 
> The bug was exposed when mappings for a device were
> actually hot-unplugged and plugged in back later.
> A similar issue was observed during the development
> of memtrace, but memtrace does it's own flushing of
> region via a custom routine.
> 
> These patches do a flush both on hotplug/unplug to
> clear any stale data in the cache w.r.t mappings,
> there is a small race window where a clean cache
> line may be created again just prior to tearing
> down the mapping.
> 
> The patches were tested by disabling the flush
> routines in memtrace and doing I/O on the trace
> file. The system immediately checkstops (quite
> reliablly if prior to the hot-unplug of the memtrace
> region, we memset the regions we are about to
> hot unplug). After these patches no custom flushing
> is needed in the memtrace code.
> 
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> Acked-by: Reza Arbab <arbab@linux.ibm.com>
> Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>

Series applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/fb5924fddf9ee31db04da7ad4e8c34

cheers