Skip to contentSkip to conversation

tlb flushing on Power

DISCUSSION Brian King 5 participants 12 messages Jan 26, 2012 – Mar 7, 2012 1 message not in the archive

lore
Share & tools
lore

Brian King

author
#1 lore raw
In reply to a message not in the archive

CC'ing linuxppc-dev...

Quoted 33 lines · Seth Jennings

Hey Dave,

So I submitted the zsmalloc patches to lkml at the beginning of the year

https://lkml.org/lkml/2012/1/9/389

I found there are two functions Nitin used in the mapping functions that are not supported in the powerpc arch:
set_pte() and __flush_tlb_one().

set_pte() seems straightforward for the ppc64 case, although I think Power does some sort of pte hashing that might convolute it a little.

My real question is how to achieve the function of __flush_tlb_one() on Power. I looked in Docuemntation/cachetlb.txt and it seems like the portable tlb flush functions are:

void flush_tlb_all(void)
void flush_tlb_mm(struct mm_struct *mm)
void flush_tlb_range(struct vm_area_struct *vma,
unsigned long start, unsigned long end)
void flush_tlb_page(struct vm_area_struct *vma, unsigned long addr)

Of those options, flush_tlb_page() seems the closest to what __flush_tlb_one() does. However, we don't have a vma argument in our context (akaik) to send to it.

I was wondering if you have any ideas. Any help is greatly appreciated!

--
Seth

Signature · 3 lines
Brian King
Linux on Power Virtualization
IBM Linux Technology Center
#2 lore raw
Quoted 14 lines · Brian King

CC'ing linuxppc-dev...

Quoted 10 lines · Seth Jennings

Hey Dave,

So I submitted the zsmalloc patches to lkml at the beginning of the year

https://lkml.org/lkml/2012/1/9/389

I found there are two functions Nitin used in the mapping functions that are not supported in the powerpc arch:
set_pte() and __flush_tlb_one().

.../...

The arch management of page tables can be tricky indeed :-) I need to have a better understanding of what you are doing to see how I can try to adapt it to power.

set_pte() is long gone on all archs really (or if it's still there it's not meant to be used as is), use set_pte_at().

__flush_tlb_one() doesn't mean anything as an arch independent functionality. We have a local_flush_tlb_page() that -might- do what you want but why in hell is that patch not using proper existing interfaces ?

Can you explain to me a bit more the whole business in this patch set about doing kmap_atomic() vs. manually trying to populate the PTEs ? Why not just use two kmap atomic entries ? If interrupts are disabled kmap_atomic() should give you contiguous ones I suppose (unless NMIs are allowed to use kmap_atomic, are they ?)

Cheers,
Ben.

#3 lore raw
Quoted 2 lines · Benjamin Herrenschmidt

Can you explain to me a bit more the whole business in this patch set about doing kmap_atomic() vs. manually trying to populate the PTEs ?

They're compressing pages and the allocator is trying getting very poor packing of compressed pages in to PAGE_SIZE chunks. So, they're moving to 2-page allocations that they need to be mapped contiguously to make it easier on the users of the allocator.

Quoted 2 lines

Why not just use two kmap atomic entries ? If interrupts are disabled kmap_atomic() should give you contiguous ones I suppose

I think you and I are at least on the same page on this one:

https://lkml.org/lkml/2012/1/26/355

Quoted 1 line

(unless NMIs are allowed to use kmap_atomic, are they ?)

Surely they can't be. Even if they could use it, they'd have to return the __kmap_atomic_idx back to the place where it started before they returned, so the interrupted code wouldn't notice.

#4 lore raw
Quoted 9 lines · Dave Hansen
Quoted 2 lines · Benjamin Herrenschmidt

Can you explain to me a bit more the whole business in this patch set about doing kmap_atomic() vs. manually trying to populate the PTEs ?

They're compressing pages and the allocator is trying getting very poor packing of compressed pages in to PAGE_SIZE chunks. So, they're moving to 2-page allocations that they need to be mapped contiguously to make it easier on the users of the allocator.

Quoted 6 lines
Quoted 2 lines

Why not just use two kmap atomic entries ? If interrupts are disabled kmap_atomic() should give you contiguous ones I suppose

I think you and I are at least on the same page on this one:

https://lkml.org/lkml/2012/1/26/355

Right. We probably want to document this somewhere if we start relying on that behaviour or at the very least add a WARN_ON() and fail from the compressed allocator if we end up with non-contiguous pages.

Quoted 5 lines
Quoted 1 line

(unless NMIs are allowed to use kmap_atomic, are they ?)

Surely they can't be. Even if they could use it, they'd have to return the __kmap_atomic_idx back to the place where it started before they returned, so the interrupted code wouldn't notice.

Ah indeed, that's for talking before I had breakfast :-)

Cheers,
Ben.

#5 lore raw
In reply to Benjamin Herrenschmidt

Hey Ben,

Thanks for responding.

Quoted 16 lines · Benjamin Herrenschmidt
Quoted 14 lines · Brian King

CC'ing linuxppc-dev...

Quoted 10 lines · Seth Jennings

Hey Dave,

So I submitted the zsmalloc patches to lkml at the beginning of the year

https://lkml.org/lkml/2012/1/9/389

I found there are two functions Nitin used in the mapping functions that are not supported in the powerpc arch:
set_pte() and __flush_tlb_one().

Quoted 5 lines

.../...

The arch management of page tables can be tricky indeed :-) I need to have a better understanding of what you are doing to see how I can try to adapt it to power.

You can look at https://lkml.org/lkml/2012/1/9/389 in zsmalloc-main.c, zs_[un]map_object() functions for the currently uses of set_pte() and __flush_tlb_one().

Quoted 2 lines

set_pte() is long gone on all archs really (or if it's still there it's not meant to be used as is), use set_pte_at().

Problem with set_pte_at() for us is that we don't have an mm_struct to pass because the mapping is not for a userspace process but for the kernel itself.

However, I do think this is the portable function we need to be using. Just need to figure out what to pass in for the mm_struct param.

Quoted 4 lines

__flush_tlb_one() doesn't mean anything as an arch independent functionality. We have a local_flush_tlb_page() that -might- do what you want but why in hell is that patch not using proper existing interfaces ?

flush_tlb_page() is the portable function we should be using. However, again, it requires a vma_area_struct. I'm not sure what we should be passing there.

--
Seth

#6 lore raw
Quoted 10 lines

You can look at https://lkml.org/lkml/2012/1/9/389 in zsmalloc-main.c, zs_[un]map_object() functions for the currently uses of set_pte() and __flush_tlb_one().

Quoted 2 lines

set_pte() is long gone on all archs really (or if it's still there it's not meant to be used as is), use set_pte_at().

Problem with set_pte_at() for us is that we don't have an mm_struct to pass because the mapping is not for a userspace process but for the kernel itself.

Then use init_mm

Quoted 11 lines

However, I do think this is the portable function we need to be using. Just need to figure out what to pass in for the mm_struct param.

Quoted 4 lines

__flush_tlb_one() doesn't mean anything as an arch independent functionality. We have a local_flush_tlb_page() that -might- do what you want but why in hell is that patch not using proper existing interfaces ?

flush_tlb_page() is the portable function we should be using. However, again, it requires a vma_area_struct. I'm not sure what we should be passing there.

Do you need this to be CPU local flush or global ? In the later, flush_tlb_kernel_range() is the right API.

If you want per-cpu, we'll have to add a new arch hook.

Cheers,
Ben.

#7 lore raw
Quoted 5 lines · Benjamin Herrenschmidt
Quoted 5 lines

You can look at https://lkml.org/lkml/2012/1/9/389 in zsmalloc-main.c, zs_[un]map_object() functions for the currently uses of set_pte() and __flush_tlb_one().

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

set_pte() is long gone on all archs really (or if it's still there it's not meant to be used as is), use set_pte_at().

Problem with set_pte_at() for us is that we don't have an mm_struct to pass because the mapping is not for a userspace process but for the kernel itself.

Then use init_mm

Thanks, that's what I was looking for.

Quoted 8 lines
Quoted 8 lines

However, I do think this is the portable function we need to be using. Just need to figure out what to pass in for the mm_struct param.

Quoted 4 lines

__flush_tlb_one() doesn't mean anything as an arch independent functionality. We have a local_flush_tlb_page() that -might- do what you want but why in hell is that patch not using proper existing interfaces ?

Quoted 8 lines
Quoted 3 lines

flush_tlb_page() is the portable function we should be using. However, again, it requires a vma_area_struct. I'm not sure what we should be passing there.

Do you need this to be CPU local flush or global ? In the later, flush_tlb_kernel_range() is the right API.

If you want per-cpu, we'll have to add a new arch hook.

We have interrupts disabled, due to the get_cpu_var() on the percpu variable zs_map_area in zs_map_object(), while the allocation is mapped. So a CPU local would be what we need.

--
Seth

#8 lore raw
Quoted 28 lines · Seth Jennings
Quoted 5 lines · Benjamin Herrenschmidt
Quoted 5 lines

You can look at https://lkml.org/lkml/2012/1/9/389 in zsmalloc-main.c, zs_[un]map_object() functions for the currently uses of set_pte() and __flush_tlb_one().

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

set_pte() is long gone on all archs really (or if it's still there it's not meant to be used as is), use set_pte_at().

Problem with set_pte_at() for us is that we don't have an mm_struct to pass because the mapping is not for a userspace process but for the kernel itself.

Then use init_mm

Thanks, that's what I was looking for.

Quoted 12 lines
Quoted 11 lines

However, I do think this is the portable function we need to be using. Just need to figure out what to pass in for the mm_struct param.

Quoted 4 lines

__flush_tlb_one() doesn't mean anything as an arch independent functionality. We have a local_flush_tlb_page() that -might- do what you want but why in hell is that patch not using proper existing interfaces ?

flush_tlb_page() is the portable function we should be using. However, again, it requires a vma_area_struct. I'm not sure what we should be passing there.

Quoted 8 lines
Quoted 4 lines

Do you need this to be CPU local flush or global ? In the later, flush_tlb_kernel_range() is the right API.

If you want per-cpu, we'll have to add a new arch hook.

We have interrupts disabled, due to the get_cpu_var() on the percpu variable zs_map_area in zs_map_object(), while the allocation is mapped. So a CPU local would be what we need.

Hey Ben,

Just wanted to bump you again about this. You mentioned that if I wanted to do a cpu-local flush of a single tlb entry, that there would have to be a new hook. Is that right?

I've been looking through the powerpc arch and I thought that I might have found the power analog to __flush_tlb_one() for x86 in local_flush_tlb_page() with a NULL vma argument. This doesn't seem to work though, as indicated by a crash when I tried it. After looking in tlbflush.h again, it seems that local_flush_tlb_page() is a no-op when CONFIG_PPC_STD_MMU_64 is set, as are almost ALL of the tlb flushing functions... which makes no sense to me.

Any help you (or anyone) can give me would be greatly appreciated.

--
Seth

#9 lore raw
Quoted 12 lines · Seth Jennings

Just wanted to bump you again about this. You mentioned that if I wanted to do a cpu-local flush of a single tlb entry, that there would have to be a new hook. Is that right?

I've been looking through the powerpc arch and I thought that I might have found the power analog to __flush_tlb_one() for x86 in local_flush_tlb_page() with a NULL vma argument. This doesn't seem to work though, as indicated by a crash when I tried it. After looking in tlbflush.h again, it seems that local_flush_tlb_page() is a no-op when CONFIG_PPC_STD_MMU_64 is set, as are almost ALL of the tlb flushing functions... which makes no sense to me.

Quoted 1 line

Any help you (or anyone) can give me would be greatly appreciated.

On ppc64 with hash-table MMU, we handle the flushes very differently. PTEs that are modified are added to a list at the time of the modification and either flushed immediately if no lazy tlb batching is in progress or flushed when leaving the lazy tlb op.

This is to avoid a problem where we might otherwise, under some circumstances, create a new TLB which can be hashed in to the hash table before the previous one has been flushed out. That would lead to a dup in the hash table which is architecturally illegal.

This happens via the call to hpte_need_flush() in pte_update().

Unfortunately, it will always consider all kernel mappings as global, so the per-cpu "optimization" won't be usable in this case, at least not until we add some kind of additional argument to that function.

Cheers,
Ben.

#10 lore raw

Hey Ben,

Thanks for the help! I was wondering if you could take a look at something for me.

I've been working on this staging driver (zsmalloc memory allocator) that does virtual mapping of two pages.

I have a github repo with the driver and the unsubmitted changes. I'm trying to make to get the pte/tlb stuff working in a portable way:

git://github.com/spartacus06/linux.git (portable branch)

The experimental commits are the top 5 and the branch is based on Greg's staging-next + frontswap-v11 patches.

Could you take a look at the zs_map_object() and zs_unmap_object() in drivers/staging/zsmalloc/zsmalloc-main.c and see if they should work for PPC64?

I'm using set_pte_at() to map. Then I'm using and ptep_get_and_clear() and local_flush_tlb_kernel_page() to unmap (I #defined local_flush_tlb_kernel_page() to local_flush_tlb_page() for PPC64 which is a no-op).

It will work most of the time, but then I'll get a crash and the mapped memory won't be what I expect. I know the cause lies in the virtual mapping because if I reduce the max_zspage_order to 1, preventing the "else" branch in zs_[un]map_object() from being called, everything is stable.

Any feedback you (or others) can provide would be appreciated!

Thanks,
Seth

Quoted 36 lines · Benjamin Herrenschmidt
Quoted 12 lines · Seth Jennings

Just wanted to bump you again about this. You mentioned that if I wanted to do a cpu-local flush of a single tlb entry, that there would have to be a new hook. Is that right?

I've been looking through the powerpc arch and I thought that I might have found the power analog to __flush_tlb_one() for x86 in local_flush_tlb_page() with a NULL vma argument. This doesn't seem to work though, as indicated by a crash when I tried it. After looking in tlbflush.h again, it seems that local_flush_tlb_page() is a no-op when CONFIG_PPC_STD_MMU_64 is set, as are almost ALL of the tlb flushing functions... which makes no sense to me.

Quoted 1 line

Any help you (or anyone) can give me would be greatly appreciated.

On ppc64 with hash-table MMU, we handle the flushes very differently. PTEs that are modified are added to a list at the time of the modification and either flushed immediately if no lazy tlb batching is in progress or flushed when leaving the lazy tlb op.

This is to avoid a problem where we might otherwise, under some circumstances, create a new TLB which can be hashed in to the hash table before the previous one has been flushed out. That would lead to a dup in the hash table which is architecturally illegal.

This happens via the call to hpte_need_flush() in pte_update().

Unfortunately, it will always consider all kernel mappings as global, so the per-cpu "optimization" won't be usable in this case, at least not until we add some kind of additional argument to that function.

Cheers,
Ben.

#11 lore raw

Seth,

Quoted 9 lines

Thanks for the help! I was wondering if you could take a look at something for me.

I've been working on this staging driver (zsmalloc memory allocator) that does virtual mapping of two pages.

I have a github repo with the driver and the unsubmitted changes. I'm trying to make to get the pte/tlb stuff working in a portable way:

Quoted 8 lines

git://github.com/spartacus06/linux.git (portable branch)

The experimental commits are the top 5 and the branch is based on Greg's staging-next + frontswap-v11 patches.

Could you take a look at the zs_map_object() and zs_unmap_object() in drivers/staging/zsmalloc/zsmalloc-main.c and see if they should work for PPC64?

I suggest you post the code directly to the list in reviewable chucks. People are much more likely to review it if they don't have to download some random git tree, checkout some branch, find the changes, etc etc.

If it's work in progress, mark it as an RFC patch and note what issues you think still exist.

Mikey

#12 lore raw
Quoted 16 lines · Michael Neuling

Seth,

Quoted 14 lines

Thanks for the help! I was wondering if you could take a look at something for me.

I've been working on this staging driver (zsmalloc memory allocator) that does virtual mapping of two pages.

I have a github repo with the driver and the unsubmitted changes. I'm trying to make to get the pte/tlb stuff working in a portable way:

git://github.com/spartacus06/linux.git (portable branch)

The experimental commits are the top 5 and the branch is based on Greg's staging-next + frontswap-v11 patches.

Quoted 7 lines
Quoted 3 lines

Could you take a look at the zs_map_object() and zs_unmap_object() in drivers/staging/zsmalloc/zsmalloc-main.c and see if they should work for PPC64?

I suggest you post the code directly to the list in reviewable chucks. People are much more likely to review it if they don't have to download some random git tree, checkout some branch, find the changes, etc etc.

It's hard to summarize out of context, but I'll try.

So zsmalloc is designed to store compressed memory pages. In order to do that efficiently in memory restricted environments where memory allocations from the buddy allocator greater than order 0 are very likely to fail, we have to be able to store compressed memory pages across non-contiguous page boundaries.

zsmalloc uses virtual memory mapping to do this. There is a per-cpu struct vm_struct *vm. This vm is initialized like this:
===
pte_t *vm_ptes[2];
vm = alloc_vm_area(2 * PAGE_SIZE, vm_ptes);
===

When the allocation needs to be accessed, we map the memory like this (with preemption disabled):

set_pte_at(&init_mm, 0, vm_ptes[0], mk_pte(page1, PAGE_KERNEL)); set_pte_at(&init_mm, 0, vm_ptes[1], mk_pte(page2, PAGE_KERNEL));

Preemption remains disabled while the user manipulates the allocation.

When the user is done, we unmap by doing:
====
ptep_get_and_clear(&init_mm, vm->addr, vm_ptes[0]); ptep_get_and_clear(&init_mm, vm->addr + PAGE_SIZE, vm_ptes[1]);

local_flush_tlb_kernel_page(vm->addr); local_flush_tlb_kernel_page(vm->addr + PAGE_SIZE);
====

In my patch, I've defined local_flush_tlb_kernel_page as:

#define local_flush_tlb_kernel_page(kaddr) local_flush_tlb_page(NULL, kaddr)

in arch/powerpc/include/asm/tlbflush.h

For PPC64 (the platform I'm testing on) local_flush_tlb_page() is a no-op because of the hashing tlb in that architecture; however, it isn't a no-op on other archs and the whole point of this change is portability.

My understanding is that the tlb flush on PPC64 happens as a result of the pte_update() eventually called from ptep_get_and_clear().

The problem is that this doesn't always seem to work. I get corruption in the mapped memory from time to time.

I've isolated the issue to this mapping code.

Does anyone see anything glaringly wrong with this approach?

Should ptep_get_and_clear() also flush the tlb entry (asynchronously I think)?

For comparison, this code is stable on x86 where local_flush_tlb_kernel_page() is defined to __flush_tlb_one().

Quoted 4 lines

If it's work in progress, mark it as an RFC patch and note what issues you think still exist.

Mikey