[PATCH] memblock: Make sure reserved.regions is freed really

[PATCH] memblock: Make sure reserved.regions is freed really

[Yinghai Lu]

memblock_free() would double reserved.regions too, so we could free
old range for reserved.regions.

So need to check that regions get doubled. If it is doubled, we need to
free it.

Cc: Tejun Heo <tj@kernel.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 mm/memblock.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/memblock.c
===================================================================
--- linux-2.6.orig/mm/memblock.c
+++ linux-2.6/mm/memblock.c
@@ -148,11 +148,26 @@ phys_addr_t __init_memblock memblock_fin
  */
 int __init_memblock memblock_free_reserved_regions(void)
 {
+	struct memblock_region *r;
+	int ret;
+
 	if (memblock.reserved.regions == memblock_reserved_init_regions)
 		return 0;
 
-	return memblock_free(__pa(memblock.reserved.regions),
-		 sizeof(struct memblock_region) * memblock.reserved.max);
+	/*
+	 * During memblock_free, reserved.regions could be doubled,
+	 * try to check with old one for checking, and need to free the new one.
+	 */
+	do {
+		r = memblock.reserved.regions;
+		ret = memblock_free(__pa(memblock.reserved.regions),
+			sizeof(struct memblock_region) * memblock.reserved.max);
+
+		if (ret)
+			break;
+	} while (memblock.reserved.regions != r);
+
+	return ret;
 }
 
 /*
@@ -163,6 +178,10 @@ int __init_memblock memblock_reserve_res
 	if (memblock.reserved.regions == memblock_reserved_init_regions)
 		return 0;
 
+	/*
+	 * Don't need to worry about if reserved.regions get updated later,
+	 * and it is big enough after memblock_free_reserved_regions().
+	 */
 	return memblock_reserve(__pa(memblock.reserved.regions),
 		 sizeof(struct memblock_region) * memblock.reserved.max);
 }

Re: [PATCH] memblock: Make sure reserved.regions is freed really

[Tejun Heo]

Hello, Yinghai.

On Fri, Jun 15, 2012 at 08:12:11PM -0700, Yinghai Lu wrote:
> memblock_free() would double reserved.regions too, so we could free
> old range for reserved.regions.
> 
> So need to check that regions get doubled. If it is doubled, we need to
> free it.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  mm/memblock.c |   23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/mm/memblock.c
> ===================================================================
> --- linux-2.6.orig/mm/memblock.c
> +++ linux-2.6/mm/memblock.c
> @@ -148,11 +148,26 @@ phys_addr_t __init_memblock memblock_fin
>   */
>  int __init_memblock memblock_free_reserved_regions(void)
>  {
> +	struct memblock_region *r;
> +	int ret;
> +
>  	if (memblock.reserved.regions == memblock_reserved_init_regions)
>  		return 0;
>  
> -	return memblock_free(__pa(memblock.reserved.regions),
> -		 sizeof(struct memblock_region) * memblock.reserved.max);
> +	/*
> +	 * During memblock_free, reserved.regions could be doubled,
> +	 * try to check with old one for checking, and need to free the new one.
> +	 */
> +	do {
> +		r = memblock.reserved.regions;
> +		ret = memblock_free(__pa(memblock.reserved.regions),
> +			sizeof(struct memblock_region) * memblock.reserved.max);
> +
> +		if (ret)
> +			break;
> +	} while (memblock.reserved.regions != r);
> +
> +	return ret;
>  }

Hmmm... nice catch but I think it's a bit complex and ugly.  In this
case, we *know* that the region isn't gonna be split.  Maybe a better
option is to add something like the following?

void memblock_remove_region_by_ptr(struct memblock_type *type,
				   struct memblock_region *r)
{
	WARN_ON(/* make sure @r is inside @type->regions */);
	memblock_remove_region(type, r - type->regions);
}

Thanks.

-- 
tejun

Re: [PATCH] memblock: Make sure reserved.regions is freed really

[Yinghai Lu]

On Mon, Jun 18, 2012 at 3:05 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Yinghai.
>
> On Fri, Jun 15, 2012 at 08:12:11PM -0700, Yinghai Lu wrote:
>> memblock_free() would double reserved.regions too, so we could free
>> old range for reserved.regions.
>>
>> So need to check that regions get doubled. If it is doubled, we need to
>> free it.
>>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  mm/memblock.c |   23 +++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> Index: linux-2.6/mm/memblock.c
>> ===================================================================
>> --- linux-2.6.orig/mm/memblock.c
>> +++ linux-2.6/mm/memblock.c
>> @@ -148,11 +148,26 @@ phys_addr_t __init_memblock memblock_fin
>>   */
>>  int __init_memblock memblock_free_reserved_regions(void)
>>  {
>> +     struct memblock_region *r;
>> +     int ret;
>> +
>>       if (memblock.reserved.regions == memblock_reserved_init_regions)
>>               return 0;
>>
>> -     return memblock_free(__pa(memblock.reserved.regions),
>> -              sizeof(struct memblock_region) * memblock.reserved.max);
>> +     /*
>> +      * During memblock_free, reserved.regions could be doubled,
>> +      * try to check with old one for checking, and need to free the new one.
>> +      */
>> +     do {
>> +             r = memblock.reserved.regions;
>> +             ret = memblock_free(__pa(memblock.reserved.regions),
>> +                     sizeof(struct memblock_region) * memblock.reserved.max);
>> +
>> +             if (ret)
>> +                     break;
>> +     } while (memblock.reserved.regions != r);
>> +
>> +     return ret;
>>  }
>
> Hmmm... nice catch but I think it's a bit complex and ugly.  In this
> case, we *know* that the region isn't gonna be split.  Maybe a better
> option is to add something like the following?

how do you know __pa(memblock.reserved.regions) would not be merged
with other entries
in reserved.regions?

>
> void memblock_remove_region_by_ptr(struct memblock_type *type,
>                                   struct memblock_region *r)
> {
>        WARN_ON(/* make sure @r is inside @type->regions */);
>        memblock_remove_region(type, r - type->regions);
> }

you want to change

/*
 * Free memblock.reserved.regions
 */
int __init_memblock memblock_free_reserved_regions(void)
{
        if (memblock.reserved.regions == memblock_reserved_init_regions)
                return 0;

        return memblock_free(__pa(memblock.reserved.regions),
                 sizeof(struct memblock_region) * memblock.reserved.max);
}

to use memblock_remove_region_by_ptr() ?

Thanks

Yinghai

Re: [PATCH] memblock: Make sure reserved.regions is freed really

[Tejun Heo]

Hey, Yinghai.

On Mon, Jun 18, 2012 at 03:58:52PM -0700, Yinghai Lu wrote:
> > Hmmm... nice catch but I think it's a bit complex and ugly.  In this
> > case, we *know* that the region isn't gonna be split.  Maybe a better
> > option is to add something like the following?
> 
> how do you know __pa(memblock.reserved.regions) would not be merged
> with other entries
> in reserved.regions?
>
> >
> > void memblock_remove_region_by_ptr(struct memblock_type *type,
> >                                   struct memblock_region *r)
> > {
> >        WARN_ON(/* make sure @r is inside @type->regions */);
> >        memblock_remove_region(type, r - type->regions);
> > }
> 
> you want to change

Heh, you're right.  My suggestion was completely bonkers.  Sorry about
that.  Hmm.... how about separating out removal pre-expansion and
doing it before calling into free?  ie. something like

static int memblock_prepare_for_isolation(struct memblock_type *type)
{
	/* we'll create at most two more regions */
	while (type->cnt + 2 > type->max)
		if (memblock_double_array(type) < 0)
			return -ENOMEM;
	return 0;
}

int __init_memblock memblock_free_reserved_regions(void)
{
	int ret;

	if (memblock.reserved.regions == memblock_reserved_init_regions)
		return 0;

	ret = memblock_prepare_for_isolation(&memblock.reserved);
	if (ret)
		return ret;

	ret = memblock_free(__pa(memblock.reserved.regions),
		sizeof(struct memblock_region) * memblock.reserved.max);
	WARN_ON(ret || memblock.reserved.regions has changed);
	return ret;
}

Thanks.

-- 
tejun

Re: [PATCH] memblock: Make sure reserved.regions is freed really

[Yinghai Lu]

On Mon, Jun 18, 2012 at 4:09 PM, Tejun Heo <tj@kernel.org> wrote:
> Hey, Yinghai.
>
> On Mon, Jun 18, 2012 at 03:58:52PM -0700, Yinghai Lu wrote:
>> > Hmmm... nice catch but I think it's a bit complex and ugly.  In this
>> > case, we *know* that the region isn't gonna be split.  Maybe a better
>> > option is to add something like the following?
>>
>> how do you know __pa(memblock.reserved.regions) would not be merged
>> with other entries
>> in reserved.regions?
>>
>> >
>> > void memblock_remove_region_by_ptr(struct memblock_type *type,
>> >                                   struct memblock_region *r)
>> > {
>> >        WARN_ON(/* make sure @r is inside @type->regions */);
>> >        memblock_remove_region(type, r - type->regions);
>> > }
>>
>> you want to change
>
> Heh, you're right.  My suggestion was completely bonkers.  Sorry about
> that.  Hmm.... how about separating out removal pre-expansion and
> doing it before calling into free?  ie. something like
>
> static int memblock_prepare_for_isolation(struct memblock_type *type)
> {
>        /* we'll create at most two more regions */
>        while (type->cnt + 2 > type->max)
>                if (memblock_double_array(type) < 0)
>                        return -ENOMEM;
>        return 0;
> }
>
> int __init_memblock memblock_free_reserved_regions(void)
> {
>        int ret;
>
>        if (memblock.reserved.regions == memblock_reserved_init_regions)
>                return 0;
>
>        ret = memblock_prepare_for_isolation(&memblock.reserved);
>        if (ret)
>                return ret;
>
>        ret = memblock_free(__pa(memblock.reserved.regions),
>                sizeof(struct memblock_region) * memblock.reserved.max);
>        WARN_ON(ret || memblock.reserved.regions has changed);
>        return ret;
> }

yes. that looks good. but that WARN_ON looks wrong, could just drop that line.

Please send complete version to Andrew or Linus.

Thanks

Yinghai