Round up address to a page, otherwise the last page isn't mapped.
Signed-off-by: Shaohua Li <[email protected]>
---
arch/x86/mm/init_64.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux/arch/x86/mm/init_64.c
===================================================================
--- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
+++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
@@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
unsigned long nr_pages = size >> PAGE_SHIFT;
int ret;
- last_mapped_pfn = init_memory_mapping(start, start + size-1);
+ last_mapped_pfn = init_memory_mapping(start,
+ round_up(start + size-1, PAGE_SIZE));
if (last_mapped_pfn > max_pfn_mapped)
max_pfn_mapped = last_mapped_pfn;
On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
> Round up address to a page, otherwise the last page isn't mapped.
>
> Signed-off-by: Shaohua Li <[email protected]>
> ---
> arch/x86/mm/init_64.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux/arch/x86/mm/init_64.c
> ===================================================================
> --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
> +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
> @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
> unsigned long nr_pages = size >> PAGE_SHIFT;
> int ret;
>
> - last_mapped_pfn = init_memory_mapping(start, start + size-1);
> + last_mapped_pfn = init_memory_mapping(start,
> + round_up(start + size-1, PAGE_SIZE));
> if (last_mapped_pfn > max_pfn_mapped)
> max_pfn_mapped = last_mapped_pfn;
should use
last_mapped_pfn = init_memory_mapping(start, start + size);
YH
On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
> > Round up address to a page, otherwise the last page isn't mapped.
> >
> > Signed-off-by: Shaohua Li <[email protected]>
> > ---
> > arch/x86/mm/init_64.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Index: linux/arch/x86/mm/init_64.c
> > ===================================================================
> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
> > unsigned long nr_pages = size >> PAGE_SHIFT;
> > int ret;
> >
> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
> > + last_mapped_pfn = init_memory_mapping(start,
> > + round_up(start + size-1, PAGE_SIZE));
> > if (last_mapped_pfn > max_pfn_mapped)
> > max_pfn_mapped = last_mapped_pfn;
>
> should use
>
> last_mapped_pfn = init_memory_mapping(start, start + size);
No, this still can't guarantee page aligned, though this works in my
test
On Wed, Oct 8, 2008 at 11:08 PM, Shaohua Li <[email protected]> wrote:
> On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
>> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
>> > Round up address to a page, otherwise the last page isn't mapped.
>> >
>> > Signed-off-by: Shaohua Li <[email protected]>
>> > ---
>> > arch/x86/mm/init_64.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > Index: linux/arch/x86/mm/init_64.c
>> > ===================================================================
>> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
>> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
>> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
>> > unsigned long nr_pages = size >> PAGE_SHIFT;
>> > int ret;
>> >
>> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
>> > + last_mapped_pfn = init_memory_mapping(start,
>> > + round_up(start + size-1, PAGE_SIZE));
>> > if (last_mapped_pfn > max_pfn_mapped)
>> > max_pfn_mapped = last_mapped_pfn;
>>
>> should use
>>
>> last_mapped_pfn = init_memory_mapping(start, start + size);
> No, this still can't guarantee page aligned, though this works in my
> test
who will call arch_add_memory? that should be start and size already
be page aligned.
YH
On Thu, Oct 09, 2008 at 02:22:50PM +0800, Yinghai Lu wrote:
> On Wed, Oct 8, 2008 at 11:08 PM, Shaohua Li <[email protected]> wrote:
> > On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
> >> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
> >> > Round up address to a page, otherwise the last page isn't mapped.
> >> >
> >> > Signed-off-by: Shaohua Li <[email protected]>
> >> > ---
> >> > arch/x86/mm/init_64.c | 3 ++-
> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> > Index: linux/arch/x86/mm/init_64.c
> >> > ===================================================================
> >> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
> >> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
> >> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
> >> > unsigned long nr_pages = size >> PAGE_SHIFT;
> >> > int ret;
> >> >
> >> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
> >> > + last_mapped_pfn = init_memory_mapping(start,
> >> > + round_up(start + size-1, PAGE_SIZE));
> >> > if (last_mapped_pfn > max_pfn_mapped)
> >> > max_pfn_mapped = last_mapped_pfn;
> >>
> >> should use
> >>
> >> last_mapped_pfn = init_memory_mapping(start, start + size);
> > No, this still can't guarantee page aligned, though this works in my
> > test
>
> who will call arch_add_memory? that should be start and size already
> be page aligned.
It's memory hotplug. Doing a round up is always ok and safe even it might be already aligned.
On Wed, Oct 8, 2008 at 11:28 PM, Shaohua Li <[email protected]> wrote:
> On Thu, Oct 09, 2008 at 02:22:50PM +0800, Yinghai Lu wrote:
>> On Wed, Oct 8, 2008 at 11:08 PM, Shaohua Li <[email protected]> wrote:
>> > On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
>> >> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
>> >> > Round up address to a page, otherwise the last page isn't mapped.
>> >> >
>> >> > Signed-off-by: Shaohua Li <[email protected]>
>> >> > ---
>> >> > arch/x86/mm/init_64.c | 3 ++-
>> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >> >
>> >> > Index: linux/arch/x86/mm/init_64.c
>> >> > ===================================================================
>> >> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
>> >> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
>> >> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
>> >> > unsigned long nr_pages = size >> PAGE_SHIFT;
>> >> > int ret;
>> >> >
>> >> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
>> >> > + last_mapped_pfn = init_memory_mapping(start,
>> >> > + round_up(start + size-1, PAGE_SIZE));
>> >> > if (last_mapped_pfn > max_pfn_mapped)
>> >> > max_pfn_mapped = last_mapped_pfn;
>> >>
>> >> should use
>> >>
>> >> last_mapped_pfn = init_memory_mapping(start, start + size);
>> > No, this still can't guarantee page aligned, though this works in my
>> > test
>>
>> who will call arch_add_memory? that should be start and size already
>> be page aligned.
> It's memory hotplug. Doing a round up is always ok and safe even it might be already aligned.
>
it seems rounding up in that case is wrong...
if the caller call that funtion with extra half page, you don't need
map that half page, because you can not use it.
YH
On Thu, Oct 09, 2008 at 02:40:34PM +0800, Yinghai Lu wrote:
> On Wed, Oct 8, 2008 at 11:28 PM, Shaohua Li <[email protected]> wrote:
> > On Thu, Oct 09, 2008 at 02:22:50PM +0800, Yinghai Lu wrote:
> >> On Wed, Oct 8, 2008 at 11:08 PM, Shaohua Li <[email protected]> wrote:
> >> > On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
> >> >> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
> >> >> > Round up address to a page, otherwise the last page isn't mapped.
> >> >> >
> >> >> > Signed-off-by: Shaohua Li <[email protected]>
> >> >> > ---
> >> >> > arch/x86/mm/init_64.c | 3 ++-
> >> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > Index: linux/arch/x86/mm/init_64.c
> >> >> > ===================================================================
> >> >> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
> >> >> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
> >> >> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
> >> >> > unsigned long nr_pages = size >> PAGE_SHIFT;
> >> >> > int ret;
> >> >> >
> >> >> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
> >> >> > + last_mapped_pfn = init_memory_mapping(start,
> >> >> > + round_up(start + size-1, PAGE_SIZE));
> >> >> > if (last_mapped_pfn > max_pfn_mapped)
> >> >> > max_pfn_mapped = last_mapped_pfn;
> >> >>
> >> >> should use
> >> >>
> >> >> last_mapped_pfn = init_memory_mapping(start, start + size);
> >> > No, this still can't guarantee page aligned, though this works in my
> >> > test
> >>
> >> who will call arch_add_memory? that should be start and size already
> >> be page aligned.
> > It's memory hotplug. Doing a round up is always ok and safe even it might be already aligned.
> >
>
> it seems rounding up in that case is wrong...
>
> if the caller call that funtion with extra half page, you don't need
> map that half page, because you can not use it.
shouldn't we mark such page as reserved and so it will not be used? This is the way we handle hole.
> On Thu, Oct 09, 2008 at 02:40:34PM +0800, Yinghai Lu wrote:
> > On Wed, Oct 8, 2008 at 11:28 PM, Shaohua Li <[email protected]> wrote:
> > > On Thu, Oct 09, 2008 at 02:22:50PM +0800, Yinghai Lu wrote:
> > >> On Wed, Oct 8, 2008 at 11:08 PM, Shaohua Li <[email protected]> wrote:
> > >> > On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
> > >> >> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
> > >> >> > Round up address to a page, otherwise the last page isn't mapped.
> > >> >> >
> > >> >> > Signed-off-by: Shaohua Li <[email protected]>
> > >> >> > ---
> > >> >> > arch/x86/mm/init_64.c | 3 ++-
> > >> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >> >> >
> > >> >> > Index: linux/arch/x86/mm/init_64.c
> > >> >> > ===================================================================
> > >> >> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
> > >> >> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
> > >> >> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
> > >> >> > unsigned long nr_pages = size >> PAGE_SHIFT;
> > >> >> > int ret;
> > >> >> >
> > >> >> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
> > >> >> > + last_mapped_pfn = init_memory_mapping(start,
> > >> >> > + round_up(start + size-1, PAGE_SIZE));
> > >> >> > if (last_mapped_pfn > max_pfn_mapped)
> > >> >> > max_pfn_mapped = last_mapped_pfn;
> > >> >>
> > >> >> should use
> > >> >>
> > >> >> last_mapped_pfn = init_memory_mapping(start, start + size);
> > >> > No, this still can't guarantee page aligned, though this works in my
> > >> > test
> > >>
> > >> who will call arch_add_memory? that should be start and size already
> > >> be page aligned.
> > > It's memory hotplug. Doing a round up is always ok and safe even it might be already aligned.
> > >
> >
> > it seems rounding up in that case is wrong...
> >
> > if the caller call that funtion with extra half page, you don't need
> > map that half page, because you can not use it.
> shouldn't we mark such page as reserved and so it will not be used? This is the way we handle hole.
Just curious,
have you ever seen a real machine which needs this patch?
If this patch is needed, there is the firmware which specifies
half page size for new memory's size...
Bye.
--
Yasunori Goto
On Tue, Oct 14, 2008 at 01:16:46PM +0800, Yasunori Goto wrote:
> > On Thu, Oct 09, 2008 at 02:40:34PM +0800, Yinghai Lu wrote:
> > > On Wed, Oct 8, 2008 at 11:28 PM, Shaohua Li <[email protected]> wrote:
> > > > On Thu, Oct 09, 2008 at 02:22:50PM +0800, Yinghai Lu wrote:
> > > >> On Wed, Oct 8, 2008 at 11:08 PM, Shaohua Li <[email protected]> wrote:
> > > >> > On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
> > > >> >> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
> > > >> >> > Round up address to a page, otherwise the last page isn't mapped.
> > > >> >> >
> > > >> >> > Signed-off-by: Shaohua Li <[email protected]>
> > > >> >> > ---
> > > >> >> > arch/x86/mm/init_64.c | 3 ++-
> > > >> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >> >> >
> > > >> >> > Index: linux/arch/x86/mm/init_64.c
> > > >> >> > ===================================================================
> > > >> >> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
> > > >> >> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
> > > >> >> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
> > > >> >> > unsigned long nr_pages = size >> PAGE_SHIFT;
> > > >> >> > int ret;
> > > >> >> >
> > > >> >> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
> > > >> >> > + last_mapped_pfn = init_memory_mapping(start,
> > > >> >> > + round_up(start + size-1, PAGE_SIZE));
> > > >> >> > if (last_mapped_pfn > max_pfn_mapped)
> > > >> >> > max_pfn_mapped = last_mapped_pfn;
> > > >> >>
> > > >> >> should use
> > > >> >>
> > > >> >> last_mapped_pfn = init_memory_mapping(start, start + size);
> > > >> > No, this still can't guarantee page aligned, though this works in my
> > > >> > test
> > > >>
> > > >> who will call arch_add_memory? that should be start and size already
> > > >> be page aligned.
> > > > It's memory hotplug. Doing a round up is always ok and safe even it might be already aligned.
> > > >
> > >
> > > it seems rounding up in that case is wrong...
> > >
> > > if the caller call that funtion with extra half page, you don't need
> > > map that half page, because you can not use it.
> > shouldn't we mark such page as reserved and so it will not be used? This is the way we handle hole.
>
> Just curious,
> have you ever seen a real machine which needs this patch?
No, I just did some experiments on a desktop for memory hotplug and this bug
triggered a crash in my test.
Yinghai's suggestion also fixed the bug. I just want to have safer method. Anyway, either approach is ok to me.
Thanks,
Shaohua
On Tue, Oct 14, 2008 at 12:30 AM, Shaohua Li <[email protected]> wrote:
> On Tue, Oct 14, 2008 at 01:16:46PM +0800, Yasunori Goto wrote:
>> > On Thu, Oct 09, 2008 at 02:40:34PM +0800, Yinghai Lu wrote:
>> > > On Wed, Oct 8, 2008 at 11:28 PM, Shaohua Li <[email protected]> wrote:
>> > > > On Thu, Oct 09, 2008 at 02:22:50PM +0800, Yinghai Lu wrote:
>> > > >> On Wed, Oct 8, 2008 at 11:08 PM, Shaohua Li <[email protected]> wrote:
>> > > >> > On Thu, 2008-10-09 at 14:04 +0800, Yinghai Lu wrote:
>> > > >> >> On Wed, Oct 8, 2008 at 10:31 PM, Shaohua Li <[email protected]> wrote:
>> > > >> >> > Round up address to a page, otherwise the last page isn't mapped.
>> > > >> >> >
>> > > >> >> > Signed-off-by: Shaohua Li <[email protected]>
>> > > >> >> > ---
>> > > >> >> > arch/x86/mm/init_64.c | 3 ++-
>> > > >> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> > > >> >> >
>> > > >> >> > Index: linux/arch/x86/mm/init_64.c
>> > > >> >> > ===================================================================
>> > > >> >> > --- linux.orig/arch/x86/mm/init_64.c 2008-10-09 11:42:33.000000000 +0800
>> > > >> >> > +++ linux/arch/x86/mm/init_64.c 2008-10-09 11:43:22.000000000 +0800
>> > > >> >> > @@ -721,7 +721,8 @@ int arch_add_memory(int nid, u64 start,
>> > > >> >> > unsigned long nr_pages = size >> PAGE_SHIFT;
>> > > >> >> > int ret;
>> > > >> >> >
>> > > >> >> > - last_mapped_pfn = init_memory_mapping(start, start + size-1);
>> > > >> >> > + last_mapped_pfn = init_memory_mapping(start,
>> > > >> >> > + round_up(start + size-1, PAGE_SIZE));
>> > > >> >> > if (last_mapped_pfn > max_pfn_mapped)
>> > > >> >> > max_pfn_mapped = last_mapped_pfn;
>> > > >> >>
>> > > >> >> should use
>> > > >> >>
>> > > >> >> last_mapped_pfn = init_memory_mapping(start, start + size);
>> > > >> > No, this still can't guarantee page aligned, though this works in my
>> > > >> > test
>> > > >>
>> > > >> who will call arch_add_memory? that should be start and size already
>> > > >> be page aligned.
>> > > > It's memory hotplug. Doing a round up is always ok and safe even it might be already aligned.
>> > > >
>> > >
>> > > it seems rounding up in that case is wrong...
>> > >
>> > > if the caller call that funtion with extra half page, you don't need
>> > > map that half page, because you can not use it.
>> > shouldn't we mark such page as reserved and so it will not be used? This is the way we handle hole.
>>
>> Just curious,
>> have you ever seen a real machine which needs this patch?
> No, I just did some experiments on a desktop for memory hotplug and this bug
> triggered a crash in my test.
> Yinghai's suggestion also fixed the bug. I just want to have safer method. Anyway, either approach is ok to me.
good, please resubmit updated one with
last_mapped_pfn = init_memory_mapping(start, start + size);
Thanks
YH