[RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[zijun_hu]

From: zijun_hu <zijun_hu@htc.com>

as shown by pcpu_build_alloc_info(), the number of units within a percpu
group is educed by rounding up the number of CPUs within the group to
@upa boundary, therefore, the number of CPUs isn't equal to the units's
if it isn't aligned to @upa normally. however, pcpu_page_first_chunk()
uses BUG_ON() to assert one number is equal the other roughly, so a panic
is maybe triggered by the BUG_ON() falsely.

in order to fix this issue, the number of CPUs is rounded up then compared
with units's, the BUG_ON() is replaced by warning and returning error code
as well to keep system alive as much as possible.

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 Changes in v2:
  - fix build error

 mm/percpu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index 32e2d8d128c1..ab1186c68ab6 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2095,6 +2095,8 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 	size_t pages_size;
 	struct page **pages;
 	int unit, i, j, rc;
+	int upa;
+	int nr_g0_units;
 
 	snprintf(psize_str, sizeof(psize_str), "%luK", PAGE_SIZE >> 10);
 
@@ -2102,7 +2104,12 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 	if (IS_ERR(ai))
 		return PTR_ERR(ai);
 	BUG_ON(ai->nr_groups != 1);
-	BUG_ON(ai->groups[0].nr_units != num_possible_cpus());
+	upa = ai->alloc_size/ai->unit_size;
+	nr_g0_units = roundup(num_possible_cpus(), upa);
+	if (unlikely(WARN_ON(ai->groups[0].nr_units != nr_g0_units))) {
+		pcpu_free_alloc_info(ai);
+		return -EINVAL;
+	}
 
 	unit_pages = ai->unit_size >> PAGE_SHIFT;
 
@@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 
 	/* allocate pages */
 	j = 0;
-	for (unit = 0; unit < num_possible_cpus(); unit++)
+	for (unit = 0; unit < num_possible_cpus(); unit++) {
+		unsigned int cpu = ai->groups[0].cpu_map[unit];
 		for (i = 0; i < unit_pages; i++) {
-			unsigned int cpu = ai->groups[0].cpu_map[unit];
 			void *ptr;
 
 			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
 			if (!ptr) {
 				pr_warn("failed to allocate %s page for cpu%u\n",
-					psize_str, cpu);
+						psize_str, cpu);
 				goto enomem;
 			}
 			/* kmemleak tracks the percpu allocations separately */
 			kmemleak_free(ptr);
 			pages[j++] = virt_to_page(ptr);
 		}
+	}
 
 	/* allocate vm area, map the pages and copy static data */
 	vm.flags = VM_ALLOC;
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[Andrew Morton]

On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <zijun_hu@zoho.com> wrote:

> as shown by pcpu_build_alloc_info(), the number of units within a percpu
> group is educed by rounding up the number of CPUs within the group to
> @upa boundary, therefore, the number of CPUs isn't equal to the units's
> if it isn't aligned to @upa normally. however, pcpu_page_first_chunk()
> uses BUG_ON() to assert one number is equal the other roughly, so a panic
> is maybe triggered by the BUG_ON() falsely.
> 
> in order to fix this issue, the number of CPUs is rounded up then compared
> with units's, the BUG_ON() is replaced by warning and returning error code
> as well to keep system alive as much as possible.

Under what circumstances is the triggered?  In other words, what are
the end-user visible effects of the fix?

I mean, this is pretty old code (isn't it?) so what are you doing that
triggers this?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[zijun_hu]

On 10/13/2016 05:41 AM, Andrew Morton wrote:
> On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <zijun_hu@zoho.com> wrote:
> 
>> as shown by pcpu_build_alloc_info(), the number of units within a percpu
>> group is educed by rounding up the number of CPUs within the group to
>> @upa boundary, therefore, the number of CPUs isn't equal to the units's
>> if it isn't aligned to @upa normally. however, pcpu_page_first_chunk()
>> uses BUG_ON() to assert one number is equal the other roughly, so a panic
>> is maybe triggered by the BUG_ON() falsely.
>>
>> in order to fix this issue, the number of CPUs is rounded up then compared
>> with units's, the BUG_ON() is replaced by warning and returning error code
>> as well to keep system alive as much as possible.
> 
> Under what circumstances is the triggered?  In other words, what are
> the end-user visible effects of the fix?
> 
the BUG_ON() takes effect when the number isn't aligned @upa, the BUG_ON()
should not be triggered under this normal circumstances.
the aim of this fixing is prevent the BUG_ON() which is triggered under
the case.

see below original code segments for reason.
pcpu_build_alloc_info(){
...

	for_each_possible_cpu(cpu)
		if (group_map[cpu] == group)
			gi->cpu_map[gi->nr_units++] = cpu;
	gi->nr_units = roundup(gi->nr_units, upa);

calculate the number of CPUs belonging to a group into relevant @gi->nr_units
then roundup @gi->nr_units up to @upa for itself

unit += gi->nr_units;
...
}

pcpu_page_first_chunk() {
...
	ai = pcpu_build_alloc_info(reserved_size, 0, PAGE_SIZE, NULL);
	if (IS_ERR(ai))
		return PTR_ERR(ai);
	BUG_ON(ai->nr_groups != 1);
	BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

it seems there is only one group and all CPUs belong to the group
but compare the number of CPUs with the number of units directly.

as shown by comments in above function. ai->groups[0].nr_units
should equal to roundup(num_possible_cpus(), @upa) other than
num_possible_cpus() directly.
...
}

> I mean, this is pretty old code (isn't it?) so what are you doing that
> triggers this?
> 
> 
i am learning memory source and find the inconsistency and think
the BUG_ON() maybe be triggered under this special normal but possible case

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[zijun_hu]

On 10/13/2016 05:41 AM, Andrew Morton wrote:
> On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <zijun_hu@zoho.com> wrote:
> 
>> as shown by pcpu_build_alloc_info(), the number of units within a percpu
>> group is educed by rounding up the number of CPUs within the group to
>> @upa boundary, therefore, the number of CPUs isn't equal to the units's
>> if it isn't aligned to @upa normally. however, pcpu_page_first_chunk()
>> uses BUG_ON() to assert one number is equal the other roughly, so a panic
>> is maybe triggered by the BUG_ON() falsely.
>>
>> in order to fix this issue, the number of CPUs is rounded up then compared
>> with units's, the BUG_ON() is replaced by warning and returning error code
>> as well to keep system alive as much as possible.
> 
> Under what circumstances is the triggered?  In other words, what are
> the end-user visible effects of the fix?
> 

the BUG_ON() takes effect when the number of CPUs isn't aligned @upa,
the BUG_ON() should not be triggered under this normal circumstances.
the aim of this fixing is prevent the BUG_ON() which is triggered under
the case.

see below original code segments for reason.
pcpu_build_alloc_info(){
...

	for_each_possible_cpu(cpu)
		if (group_map[cpu] == group)
			gi->cpu_map[gi->nr_units++] = cpu;
	gi->nr_units = roundup(gi->nr_units, upa);

calculate the number of CPUs belonging to a group into relevant @gi->nr_units
then roundup @gi->nr_units up to @upa for itself

	unit += gi->nr_units;
...
}

pcpu_page_first_chunk() {
...
	ai = pcpu_build_alloc_info(reserved_size, 0, PAGE_SIZE, NULL);
	if (IS_ERR(ai))
		return PTR_ERR(ai);
	BUG_ON(ai->nr_groups != 1);
	BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

it seems there is only one group and all CPUs belong to the group
but compare the number of CPUs with the number of units directly.
...
}

as shown by comments in above function. ai->groups[0].nr_units
should equal to roundup(num_possible_cpus(), @upa) other than
num_possible_cpus() directly.


> I mean, this is pretty old code (isn't it?) so what are you doing that
> triggers this?
> 
> 
i am learning memory management source and find the inconsistency and think
the BUG_ON() maybe be triggered under this special normal but possible case
it maybe a logic error 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[Tejun Heo]

On Tue, Oct 11, 2016 at 10:00:28PM +0800, zijun_hu wrote:
> From: zijun_hu <zijun_hu@htc.com>
> 
> as shown by pcpu_build_alloc_info(), the number of units within a percpu
> group is educed by rounding up the number of CPUs within the group to
> @upa boundary, therefore, the number of CPUs isn't equal to the units's
> if it isn't aligned to @upa normally. however, pcpu_page_first_chunk()
> uses BUG_ON() to assert one number is equal the other roughly, so a panic
> is maybe triggered by the BUG_ON() falsely.
> 
> in order to fix this issue, the number of CPUs is rounded up then compared
> with units's, the BUG_ON() is replaced by warning and returning error code
> as well to keep system alive as much as possible.

I really can't decode what the actual issue is here.  Can you please
give an example of a concrete case?

> @@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
>  
>  	/* allocate pages */
>  	j = 0;
> -	for (unit = 0; unit < num_possible_cpus(); unit++)
> +	for (unit = 0; unit < num_possible_cpus(); unit++) {
> +		unsigned int cpu = ai->groups[0].cpu_map[unit];
>  		for (i = 0; i < unit_pages; i++) {
> -			unsigned int cpu = ai->groups[0].cpu_map[unit];
>  			void *ptr;
>  
>  			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
>  			if (!ptr) {
>  				pr_warn("failed to allocate %s page for cpu%u\n",
> -					psize_str, cpu);
> +						psize_str, cpu);

And stop making gratuitous changes?

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[zijun_hu]

On 2016/10/14 7:29, Tejun Heo wrote:
> On Tue, Oct 11, 2016 at 10:00:28PM +0800, zijun_hu wrote:
>> From: zijun_hu <zijun_hu@htc.com>
>>
>> as shown by pcpu_build_alloc_info(), the number of units within a percpu
>> group is educed by rounding up the number of CPUs within the group to
>> @upa boundary, therefore, the number of CPUs isn't equal to the units's
>> if it isn't aligned to @upa normally. however, pcpu_page_first_chunk()
>> uses BUG_ON() to assert one number is equal the other roughly, so a panic
>> is maybe triggered by the BUG_ON() falsely.
>>
>> in order to fix this issue, the number of CPUs is rounded up then compared
>> with units's, the BUG_ON() is replaced by warning and returning error code
>> as well to keep system alive as much as possible.
> 
> I really can't decode what the actual issue is here.  Can you please
> give an example of a concrete case?
> 
the right relationship between the number of CPUs @nr_cpus within a percpu group
and the number of unites @nr_units within the same group is that
@nr_units == roundup(@nr_cpus, @upa);

the process of consideration is shown as follows:

1i 1/4 ? current code segments:

BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

2i 1/4 ? changes for considering the right relationship between the number of CPUs and units 

BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa));

3) replace BUG_ON() by warning and returning error code since it seems BUG_ON() isn't
   nice as shown by linus recent LKML mail

BUG_ON(ai->nr_groups != 1);
if (ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa))
   return -EINVAL;

so 3) is my finial changes;


for the relationship of both numbers : see the reply for andrew
thanks

;
>> @@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
>>  
>>  	/* allocate pages */
>>  	j = 0;
>> -	for (unit = 0; unit < num_possible_cpus(); unit++)
>> +	for (unit = 0; unit < num_possible_cpus(); unit++) {
>> +		unsigned int cpu = ai->groups[0].cpu_map[unit];
>>  		for (i = 0; i < unit_pages; i++) {
>> -			unsigned int cpu = ai->groups[0].cpu_map[unit];
>>  			void *ptr;
>>  
>>  			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
>>  			if (!ptr) {
>>  				pr_warn("failed to allocate %s page for cpu%u\n",
>> -					psize_str, cpu);
>> +						psize_str, cpu);
> 
> And stop making gratuitous changes?
> 
> Thanks.
> 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[zijun_hu]

On 2016/10/14 7:29, Tejun Heo wrote:
> On Tue, Oct 11, 2016 at 10:00:28PM +0800, zijun_hu wrote:
>> From: zijun_hu <zijun_hu@htc.com>
>>
>> as shown by pcpu_build_alloc_info(), the number of units within a percpu
>> group is educed by rounding up the number of CPUs within the group to
>> @upa boundary, therefore, the number of CPUs isn't equal to the units's
>> if it isn't aligned to @upa normally. however, pcpu_page_first_chunk()
>> uses BUG_ON() to assert one number is equal the other roughly, so a panic
>> is maybe triggered by the BUG_ON() falsely.
>>
>> in order to fix this issue, the number of CPUs is rounded up then compared
>> with units's, the BUG_ON() is replaced by warning and returning error code
>> as well to keep system alive as much as possible.
> 
> I really can't decode what the actual issue is here.  Can you please
> give an example of a concrete case?
> 
the right relationship between the number of CPUs @nr_cpus within a percpu group
and the number of unites @nr_units within the same group is that
@nr_units == roundup(@nr_cpus, @upa);

the process of consideration is shown as follows:

1i 1/4 ? current code segments:

BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

2i 1/4 ? changes for considering the right relationship between the number of CPUs and units 

BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa));

3) replace BUG_ON() by warning and returning error code since it seems BUG_ON() isn't
   nice as shown by linus recent LKML mail

BUG_ON(ai->nr_groups != 1);
if (ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa))
   return -EINVAL;

so 3) is my finial changes;
for the relationship of both numbers : see the reply for andrew

>> @@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
>>  
>>  	/* allocate pages */
>>  	j = 0;
>> -	for (unit = 0; unit < num_possible_cpus(); unit++)
>> +	for (unit = 0; unit < num_possible_cpus(); unit++) {
>> +		unsigned int cpu = ai->groups[0].cpu_map[unit];
>>  		for (i = 0; i < unit_pages; i++) {
>> -			unsigned int cpu = ai->groups[0].cpu_map[unit];
>>  			void *ptr;
>>  
>>  			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
>>  			if (!ptr) {
>>  				pr_warn("failed to allocate %s page for cpu%u\n",
>> -					psize_str, cpu);
>> +						psize_str, cpu);
> 
> And stop making gratuitous changes?
>

this changes is just for looking nicer instinctively
@cpu can be determined in the first outer loop.

> Thanks.
> 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[Tejun Heo]

Hello,

On Fri, Oct 14, 2016 at 08:06:10AM +0800, zijun_hu wrote:
> > I really can't decode what the actual issue is here.  Can you please
> > give an example of a concrete case?
> > 
> the right relationship between the number of CPUs @nr_cpus within a percpu group
> and the number of unites @nr_units within the same group is that
> @nr_units == roundup(@nr_cpus, @upa);

My question was whether there can be actual hardware configurations
where this code can fail and if so what they look like and how they
would fail.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC v2 PATCH] mm/percpu.c: fix panic triggered by BUG_ON() falsely

[zijun_hu]

On 2016/10/14 8:24, Tejun Heo wrote:
> Hello,
> 
> On Fri, Oct 14, 2016 at 08:06:10AM +0800, zijun_hu wrote:
>>> I really can't decode what the actual issue is here.  Can you please
>>> give an example of a concrete case?
>>>
>> the right relationship between the number of CPUs @nr_cpus within a percpu group
>> and the number of unites @nr_units within the same group is that
>> @nr_units == roundup(@nr_cpus, @upa);
> 
> My question was whether there can be actual hardware configurations
> where this code can fail and if so what they look like and how they
> would fail.
> 
> Thanks.
> 
this answer is difficult to answer since there are so many hardware configurations
moreover, besides hardware configuration, reserved size can contribute to this issue
as we known, this interface is developed for various ARCHs to setups percpu areas, 
so we should not assume more detailed aspects about ARCH. neither hardware config
nor reserved size.

i am learning memory management code and find the inconsistency between here and
there. the log is similar with a panic triggered by BUG_ON() if the numbers of
CPUs isn't aligned to @upa

are you agree the relationship of between CPU and units? 
what bad effects do this changes results in?

are you sure all hardware configurations and reserved size always make number of CPUs
are equal to units? if turei 1/4 ? is it redundant for the consideration in there place.





--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>