Re: tlb flushing on Power

Re: tlb flushing on Power

[Brian King]

CC'ing linuxppc-dev...


On 01/26/2012 08:18 AM, Seth Jennings wrote:
> 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


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: tlb flushing on Power

[Benjamin Herrenschmidt]

On Thu, 2012-01-26 at 08:41 -0600, Brian King wrote:
> CC'ing linuxppc-dev...
> 
> 
> On 01/26/2012 08:18 AM, Seth Jennings wrote:
> > 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.

Re: tlb flushing on Power

[Dave Hansen]

On 01/26/2012 01:39 PM, Benjamin Herrenschmidt wrote:
> 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.

> 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

> (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.

Re: tlb flushing on Power

[Benjamin Herrenschmidt]

On Thu, 2012-01-26 at 14:30 -0800, Dave Hansen wrote:
> On 01/26/2012 01:39 PM, Benjamin Herrenschmidt wrote:
> > 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.
> 
> > 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.

> > (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.

Re: tlb flushing on Power

[Seth Jennings]

Hey Ben,

Thanks for responding.

On 01/26/2012 03:39 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2012-01-26 at 08:41 -0600, Brian King wrote:
>> CC'ing linuxppc-dev...
>>
>>
>> On 01/26/2012 08:18 AM, Seth Jennings wrote:
>>> 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.

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().

> 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.

> __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

Re: tlb flushing on Power

[Benjamin Herrenschmidt]

> 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().
> 
> > 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

> 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.
> 
> > __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.

Re: tlb flushing on Power

[Seth Jennings]

On 02/08/2012 03:04 PM, Benjamin Herrenschmidt wrote:
> 
>> 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().
>>
>>> 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.

>> 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.
>>
>>> __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.

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

Re: tlb flushing on Power

[Seth Jennings]

On 02/10/2012 01:14 PM, Seth Jennings wrote:
> On 02/08/2012 03:04 PM, Benjamin Herrenschmidt wrote:
>>
>>> 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().
>>>
>>>> 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.
> 
>>> 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.
>>>
>>>> __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.
> 
> 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

Re: tlb flushing on Power

[Benjamin Herrenschmidt]

On Thu, 2012-02-16 at 11:11 -0600, Seth Jennings wrote:

> 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.

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.

Re: tlb flushing on Power

[Seth Jennings]

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

On 02/16/2012 02:31 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2012-02-16 at 11:11 -0600, Seth Jennings wrote:
> 
>> 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.
> 
> 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.
> 
> 

Re: tlb flushing on Power

[Michael Neuling]

Seth,

> 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 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

Re: tlb flushing on Power

[Seth Jennings]

On 03/06/2012 11:28 PM, Michael Neuling wrote:
> Seth,
> 
>> 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 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().

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