From: zijun_hu <[email protected]>
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 <[email protected]>
---
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
On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <[email protected]> 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?
On 10/13/2016 05:41 AM, Andrew Morton wrote:
> On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <[email protected]> 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
On 10/13/2016 05:41 AM, Andrew Morton wrote:
> On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <[email protected]> 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
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 <[email protected]>
>>
>> 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:
1） current code segments:
BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != num_possible_cpus());
2） 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.
>
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 <[email protected]>
>>
>> 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:
1） current code segments:
BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != num_possible_cpus());
2） 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.
>
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
On Tue, Oct 11, 2016 at 10:00:28PM +0800, zijun_hu wrote:
> From: zijun_hu <[email protected]>
>
> 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