Re: Kdump issue with percpu_alloc=lpage

Re: Kdump issue with percpu_alloc=lpage

[John Blackwood]

 > Vivek Goyal wrote:
 > > > In kdump, we allocate per cpu area using alloc_percpu() and later
 > > > export the physical address of the area allocated to user space 
through
 > > > sysfs. (/sys/devices/system/cpu/cpuN/crash_notes). kexec-tools 
user space
 > > > utility makes use of this physical address to store in some ELF 
headers
 > > > which in turn are used by the second kernel booted after crash.
 > > >
 > > > We assume that address returned by per_cpu_ptr() is unity mapped and
 > > > use __pa() to convert that address to physical address.
 > > >
 > > > addr = __pa(per_cpu_ptr(crash_notes, cpunum));
 > > >
 > > > Is that not a valid assumption with percpu_alloc=lpage or 
percpu_alloc=4k
 > > > options? If not, what's the right way to get the physical address in
 > > > such situations?
 >
 > The lpage allocator is gone in the latest tree and only "embed" and
 > "page" allocators are there.  The only difference between the two is
 > that the embed one will put the first chunk inside the linearly mapped
 > area which in turn means that __pa() would work on static percpu
 > variables and some of dynamic ones but from the second chunk on and
 > for the page allocator, the percpu addresses will be remapped into
 > vmalloc area and behaves just like any other vmalloc address meaning
 > that the physical page can be determined using vmalloc_to_page().  So,
 > something like the following should work,
 >
 >         v = per_cpu_ptr(crash_notes, cpunum);
 >         if (v < VMALLOC_START || v >= VMALLOC_END)
 >                 p = __pa(v);
 >         else
 >                 p = page_to_phys(vmalloc_to_page(v));
 >
 > For the now removed lpage, it would be a bit difficult and we'll
 > probably need to add a dedicated function to percpu to determine the
 > physical address.  Hmmm... probably the right thing to do is to add
 > such function so that the user can simply call percpu_to_phys()
 > regardless of address?

Hi Tejun,

Just my 2 cent's worth...

I like the idea of having a percpu_to_phys() function that others can
call so that such detail are localized... and this also would tend to
be maintained better over time, since it would be more visible being
located near where the percpu support is implmented.

Thank you for the information.


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Vivek Goyal]

On Tue, Oct 27, 2009 at 02:16:21PM -0500, John Blackwood wrote:
>
> > Vivek Goyal wrote:
> > > > In kdump, we allocate per cpu area using alloc_percpu() and later
> > > > export the physical address of the area allocated to user space  
> through
> > > > sysfs. (/sys/devices/system/cpu/cpuN/crash_notes). kexec-tools  
> user space
> > > > utility makes use of this physical address to store in some ELF  
> headers
> > > > which in turn are used by the second kernel booted after crash.
> > > >
> > > > We assume that address returned by per_cpu_ptr() is unity mapped and
> > > > use __pa() to convert that address to physical address.
> > > >
> > > > addr = __pa(per_cpu_ptr(crash_notes, cpunum));
> > > >
> > > > Is that not a valid assumption with percpu_alloc=lpage or  
> percpu_alloc=4k
> > > > options? If not, what's the right way to get the physical address in
> > > > such situations?
> >
> > The lpage allocator is gone in the latest tree and only "embed" and
> > "page" allocators are there.  The only difference between the two is
> > that the embed one will put the first chunk inside the linearly mapped
> > area which in turn means that __pa() would work on static percpu
> > variables and some of dynamic ones but from the second chunk on and
> > for the page allocator, the percpu addresses will be remapped into
> > vmalloc area and behaves just like any other vmalloc address meaning
> > that the physical page can be determined using vmalloc_to_page().  So,
> > something like the following should work,
> >
> >         v = per_cpu_ptr(crash_notes, cpunum);
> >         if (v < VMALLOC_START || v >= VMALLOC_END)
> >                 p = __pa(v);
> >         else
> >                 p = page_to_phys(vmalloc_to_page(v));
> >
> > For the now removed lpage, it would be a bit difficult and we'll
> > probably need to add a dedicated function to percpu to determine the
> > physical address.  Hmmm... probably the right thing to do is to add
> > such function so that the user can simply call percpu_to_phys()
> > regardless of address?
>
> Hi Tejun,
>
> Just my 2 cent's worth...
>
> I like the idea of having a percpu_to_phys() function that others can
> call so that such detail are localized... and this also would tend to
> be maintained better over time, since it would be more visible being
> located near where the percpu support is implmented.
>
> Thank you for the information.

Thanks Tejun. percpu_to_phys() makes sense to me also. John, would you like to
post a patch for this.

Thanks
Vivek

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[John Blackwood]

 > Subject: Re: Kdump issue with percpu_alloc=lpage
 > From: Vivek Goyal <vgoyal@redhat.com>
 > Date: Tue, 27 Oct 2009 15:24:14 -0400
 > To: "Blackwood, John" <john.blackwood@ccur.com>
 > CC: Tejun Heo <tj@kernel.org>, "kexec@lists.infradead.org" 
<kexec@lists.infradead.org>
 >
 > > > The lpage allocator is gone in the latest tree and only "embed" and
 > > > "page" allocators are there.  The only difference between the two is
 > > > that the embed one will put the first chunk inside the linearly 
mapped
 > > > area which in turn means that __pa() would work on static percpu
 > > > variables and some of dynamic ones but from the second chunk on and
 > > > for the page allocator, the percpu addresses will be remapped into
 > > > vmalloc area and behaves just like any other vmalloc address meaning
 > > > that the physical page can be determined using vmalloc_to_page(). 
  So,
 > > > something like the following should work,
 > > >
 > > >         v = per_cpu_ptr(crash_notes, cpunum);
 > > >         if (v < VMALLOC_START || v >= VMALLOC_END)
 > > >                 p = __pa(v);
 > > >         else
 > > >                 p = page_to_phys(vmalloc_to_page(v));
 > > >
 > > > For the now removed lpage, it would be a bit difficult and we'll
 > > > probably need to add a dedicated function to percpu to determine the
 > > > physical address.  Hmmm... probably the right thing to do is to add
 > > > such function so that the user can simply call percpu_to_phys()
 > > > regardless of address?
 > >
 > > Hi Tejun,
 > >
 > > Just my 2 cent's worth...
 > >
 > > I like the idea of having a percpu_to_phys() function that others can
 > > call so that such detail are localized... and this also would tend to
 > > be maintained better over time, since it would be more visible being
 > > located near where the percpu support is implmented.
 > >
 > > Thank you for the information.
 >
 > Thanks Tejun. percpu_to_phys() makes sense to me also. John, would you
 > like to post a patch for this.

Hi Vivek,

I'm probably not really NUMA/vm savy enough to attempt a patch
for approval at LKML.

But maybe someone else more qualified can take up the cause.

Thanks.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Tejun Heo]

Hello,

10/29/2009 06:22 AM, John Blackwood wrote:
>> Thanks Tejun. percpu_to_phys() makes sense to me also. John, would you
>> like to post a patch for this.
> 
> Hi Vivek,
> 
> I'm probably not really NUMA/vm savy enough to attempt a patch
> for approval at LKML.
> 
> But maybe someone else more qualified can take up the cause.

Is this still necessary for the current upstream where lpage allocator
is gone (but vmalloc mapping is still there)?

Thanks.

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Vivek Goyal]

On Wed, Nov 18, 2009 at 06:25:14PM +0900, Tejun Heo wrote:
> Hello,
> 
> 10/29/2009 06:22 AM, John Blackwood wrote:
> >> Thanks Tejun. percpu_to_phys() makes sense to me also. John, would you
> >> like to post a patch for this.
> > 
> > Hi Vivek,
> > 
> > I'm probably not really NUMA/vm savy enough to attempt a patch
> > for approval at LKML.
> > 
> > But maybe someone else more qualified can take up the cause.
> 
> Is this still necessary for the current upstream where lpage allocator
> is gone (but vmalloc mapping is still there)?
> 

Hi Tejun,

I did load a kdump kernel on 32-rc7 and it worked fine. But I guess in
this case memory might have come from linearly mapped region.

If the default per cpu allocator can get memory from vmalloc region also, then
I think we will need this function which can map virtual address to
physical address.

Are there multiple allocators now? If yes, what are the command line
options and I can try to use some other allocator and see if I can force
the condition where memory comes from vmalloc region and I observe the
crash.

Once I can reproduce it, I can also send you the fix you suggested.

Thanks
Vivek

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Tejun Heo]

Hello,

11/19/2009 11:33 PM, Vivek Goyal wrote:
> I did load a kdump kernel on 32-rc7 and it worked fine. But I guess in
> this case memory might have come from linearly mapped region.
> 
> If the default per cpu allocator can get memory from vmalloc region
> also, then I think we will need this function which can map virtual
> address to physical address.

I see.

> Are there multiple allocators now? If yes, what are the command line
> options and I can try to use some other allocator and see if I can force
> the condition where memory comes from vmalloc region and I observe the
> crash.
> 
> Once I can reproduce it, I can also send you the fix you suggested.

Now there are two allocators - embed (default) and page.  You can
choose using percpu_alloc= parameter.  Embed allocator will put the
first chunk in linear mapping area while page will put the first chunk
in vmalloc area too but regardless of the allocator from the second
chunk it will always be in the vmalloc area.  So, either using
percpu_alloc=page or allocating some amount of percpu memory using
__alloc_percpu() - a thousand 4k blocks will always be enough - should
do it.

Thanks.

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Vivek Goyal]

On Thu, Nov 19, 2009 at 11:45:25PM +0900, Tejun Heo wrote:
> Hello,
> 
> 11/19/2009 11:33 PM, Vivek Goyal wrote:
> > I did load a kdump kernel on 32-rc7 and it worked fine. But I guess in
> > this case memory might have come from linearly mapped region.
> > 
> > If the default per cpu allocator can get memory from vmalloc region
> > also, then I think we will need this function which can map virtual
> > address to physical address.
> 
> I see.
> 
> > Are there multiple allocators now? If yes, what are the command line
> > options and I can try to use some other allocator and see if I can force
> > the condition where memory comes from vmalloc region and I observe the
> > crash.
> > 
> > Once I can reproduce it, I can also send you the fix you suggested.
> 
> Now there are two allocators - embed (default) and page.  You can
> choose using percpu_alloc= parameter.  Embed allocator will put the
> first chunk in linear mapping area while page will put the first chunk
> in vmalloc area too but regardless of the allocator from the second
> chunk it will always be in the vmalloc area.  So, either using
> percpu_alloc=page or allocating some amount of percpu memory using
> __alloc_percpu() - a thousand 4k blocks will always be enough - should
> do it.

Looks like with percpu_alloc=page, I am getting bogus physical addresses in
/sys/devices/system/cpu<N>/crash_notes.

With percpu_alloc=page

cpu0 60fffff49800
cpu1 60fffff60800
cpu2 60fffff77800

with percpu_alloc=embed

cpu0 28219800
cpu1 28259800
cpu2 28299800

I got 4G of RAM in my box. "embed" seems to be fine.

Thanks
Vivek

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[John Blackwood]

Vivek Goyal wrote:
> On Thu, Nov 19, 2009 at 11:45:25PM +0900, Tejun Heo wrote:
>> Hello,
>>
>> 11/19/2009 11:33 PM, Vivek Goyal wrote:
>>> I did load a kdump kernel on 32-rc7 and it worked fine. But I guess in
>>> this case memory might have come from linearly mapped region.
>>>
>>> If the default per cpu allocator can get memory from vmalloc region
>>> also, then I think we will need this function which can map virtual
>>> address to physical address.
>> I see.
>>
>>> Are there multiple allocators now? If yes, what are the command line
>>> options and I can try to use some other allocator and see if I can force
>>> the condition where memory comes from vmalloc region and I observe the
>>> crash.
>>>
>>> Once I can reproduce it, I can also send you the fix you suggested.
>> Now there are two allocators - embed (default) and page.  You can
>> choose using percpu_alloc= parameter.  Embed allocator will put the
>> first chunk in linear mapping area while page will put the first chunk
>> in vmalloc area too but regardless of the allocator from the second
>> chunk it will always be in the vmalloc area.  So, either using
>> percpu_alloc=page or allocating some amount of percpu memory using
>> __alloc_percpu() - a thousand 4k blocks will always be enough - should
>> do it.
> 
> Looks like with percpu_alloc=page, I am getting bogus physical addresses in
> /sys/devices/system/cpu<N>/crash_notes.
> 
> With percpu_alloc=page
> 
> cpu0 60fffff49800
> cpu1 60fffff60800
> cpu2 60fffff77800
> 
> with percpu_alloc=embed
> 
> cpu0 28219800
> cpu1 28259800
> cpu2 28299800
> 
> I got 4G of RAM in my box. "embed" seems to be fine.

That sounds right to me.  In previous kernels, 'embed' was o.k. also.

Thanks for looking into this Vivek.
I've been too busy right now to try this out.



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Vivek Goyal]

On Thu, Nov 19, 2009 at 11:45:25PM +0900, Tejun Heo wrote:
> Hello,
> 
> 11/19/2009 11:33 PM, Vivek Goyal wrote:
> > I did load a kdump kernel on 32-rc7 and it worked fine. But I guess in
> > this case memory might have come from linearly mapped region.
> > 
> > If the default per cpu allocator can get memory from vmalloc region
> > also, then I think we will need this function which can map virtual
> > address to physical address.
> 
> I see.
> 
> > Are there multiple allocators now? If yes, what are the command line
> > options and I can try to use some other allocator and see if I can force
> > the condition where memory comes from vmalloc region and I observe the
> > crash.
> > 
> > Once I can reproduce it, I can also send you the fix you suggested.
> 
> Now there are two allocators - embed (default) and page.  You can
> choose using percpu_alloc= parameter.  Embed allocator will put the
> first chunk in linear mapping area while page will put the first chunk
> in vmalloc area too but regardless of the allocator from the second
> chunk it will always be in the vmalloc area.  So, either using
> percpu_alloc=page or allocating some amount of percpu memory using
> __alloc_percpu() - a thousand 4k blocks will always be enough - should
> do it.
> 
> Thanks.

Hi Tejun,

I implemented your suggested function. This patch seems to be fixing the
issue. Does it look good to you?

Please let me know if you want me to post it to lkml or you will pull it
in your tree and push it to Linus.

Thanks
Vivek


o kdump functionality reserves a per cpu area at boot time and exports the
  physical address of that area to user space through sys interface. This
  area stores some dump related information like cpu register states etc
  at the time of crash.

o We were assuming that per cpu area always come from linearly mapped meory
  region and using __pa() to determine physical address.
  With percpu_alloc=page, per cpu area can come from vmalloc region also and
  __pa() breaks.

o This patch implements new function to convert per cpu address to physical
  address.

Before the patch, crash_notes addresses looked as follows.

cpu0 60fffff49800
cpu1 60fffff60800
cpu2 60fffff77800

These are bogus phsyical addresses.

After the patch, address are following.

cpu0 13eb44000
cpu1 13eb43000
cpu2 13eb42000
cpu3 13eb41000

These look fine. I got 4G of memory and /proc/iomem tell me following.

100000000-13fffffff : System RAM

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 drivers/base/cpu.c     |    2 +-
 include/linux/percpu.h |    2 ++
 mm/percpu.c            |    9 +++++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

Index: linux9/mm/percpu.c
===================================================================
--- linux9.orig/mm/percpu.c	2009-11-12 19:46:07.000000000 -0500
+++ linux9/mm/percpu.c	2009-11-19 10:55:35.000000000 -0500
@@ -2069,3 +2069,12 @@ void __init setup_per_cpu_areas(void)
 		__per_cpu_offset[cpu] = delta + pcpu_unit_offsets[cpu];
 }
 #endif /* CONFIG_HAVE_SETUP_PER_CPU_AREA */
+
+unsigned long long per_cpu_ptr_to_phys(void *addr)
+{
+	if ((unsigned long)addr < VMALLOC_START ||
+			(unsigned long)addr >= VMALLOC_END)
+		return __pa(addr);
+	else
+		return page_to_phys(vmalloc_to_page(addr));
+}
Index: linux9/include/linux/percpu.h
===================================================================
--- linux9.orig/include/linux/percpu.h	2009-11-12 19:46:07.000000000 -0500
+++ linux9/include/linux/percpu.h	2009-11-19 10:55:47.000000000 -0500
@@ -32,6 +32,8 @@
 	&__get_cpu_var(var); }))
 #define put_cpu_var(var) preempt_enable()
 
+extern unsigned long long per_cpu_ptr_to_phys(void *addr);
+
 #ifdef CONFIG_SMP
 
 #ifndef CONFIG_HAVE_LEGACY_PER_CPU_AREA
Index: linux9/drivers/base/cpu.c
===================================================================
--- linux9.orig/drivers/base/cpu.c	2009-11-12 19:46:07.000000000 -0500
+++ linux9/drivers/base/cpu.c	2009-11-19 10:54:08.000000000 -0500
@@ -97,7 +97,7 @@ static ssize_t show_crash_notes(struct s
 	 * boot up and this data does not change there after. Hence this
 	 * operation should be safe. No locking required.
 	 */
-	addr = __pa(per_cpu_ptr(crash_notes, cpunum));
+	addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpunum));
 	rc = sprintf(buf, "%Lx\n", addr);
 	return rc;
 }

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Tejun Heo]

Hello,

11/20/2009 01:23 AM, Vivek Goyal wrote:
> I implemented your suggested function. This patch seems to be fixing the
> issue. Does it look good to you?
> 
> Please let me know if you want me to post it to lkml or you will pull it
> in your tree and push it to Linus.

Please post it to lkml and cc me.  I'll push it to Linus afterwards.

> Index: linux9/mm/percpu.c
> ===================================================================
> --- linux9.orig/mm/percpu.c	2009-11-12 19:46:07.000000000 -0500
> +++ linux9/mm/percpu.c	2009-11-19 10:55:35.000000000 -0500
> @@ -2069,3 +2069,12 @@ void __init setup_per_cpu_areas(void)
>  		__per_cpu_offset[cpu] = delta + pcpu_unit_offsets[cpu];
>  }
>  #endif /* CONFIG_HAVE_SETUP_PER_CPU_AREA */
> +
> +unsigned long long per_cpu_ptr_to_phys(void *addr)

Maybe phys_addr_t?

Thanks.

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: Kdump issue with percpu_alloc=lpage

[Vivek Goyal]

On Fri, Nov 20, 2009 at 01:31:22AM +0900, Tejun Heo wrote:
> Hello,
> 
> 11/20/2009 01:23 AM, Vivek Goyal wrote:
> > I implemented your suggested function. This patch seems to be fixing the
> > issue. Does it look good to you?
> > 
> > Please let me know if you want me to post it to lkml or you will pull it
> > in your tree and push it to Linus.
> 
> Please post it to lkml and cc me.  I'll push it to Linus afterwards.
> 
> > Index: linux9/mm/percpu.c
> > ===================================================================
> > --- linux9.orig/mm/percpu.c	2009-11-12 19:46:07.000000000 -0500
> > +++ linux9/mm/percpu.c	2009-11-19 10:55:35.000000000 -0500
> > @@ -2069,3 +2069,12 @@ void __init setup_per_cpu_areas(void)
> >  		__per_cpu_offset[cpu] = delta + pcpu_unit_offsets[cpu];
> >  }
> >  #endif /* CONFIG_HAVE_SETUP_PER_CPU_AREA */
> > +
> > +unsigned long long per_cpu_ptr_to_phys(void *addr)
> 
> Maybe phys_addr_t?
> 

Sounds good. Will change it and post to lkml.

Thanks
Vivek

> Thanks.
> 
> -- 
> tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec