Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1351317 > unrolled thread
| Started by | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| First post | 2016-03-07 05:40 +0100 |
| Last post | 2016-03-09 03:20 +0100 |
| Articles | 20 on this page of 25 — 8 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: Suspicious error for CMA stress test Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-07 05:40 +0100
Re: Suspicious error for CMA stress test "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> - 2016-03-07 09:20 +0100
Re: Suspicious error for CMA stress test Laura Abbott <labbott@redhat.com> - 2016-03-07 19:50 +0100
Re: Suspicious error for CMA stress test "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> - 2016-03-08 03:00 +0100
Re: Suspicious error for CMA stress test "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> - 2016-03-09 02:30 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <js1304@gmail.com> - 2016-03-11 16:10 +0100
Re: Suspicious error for CMA stress test Vlastimil Babka <vbabka@suse.cz> - 2016-03-11 18:10 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-14 07:50 +0100
Re: Suspicious error for CMA stress test Vlastimil Babka <vbabka@suse.cz> - 2016-03-14 08:10 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-14 08:20 +0100
Re: Suspicious error for CMA stress test Vlastimil Babka <vbabka@suse.cz> - 2016-03-14 13:40 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <js1304@gmail.com> - 2016-03-14 15:20 +0100
Re: Suspicious error for CMA stress test Vlastimil Babka <vbabka@suse.cz> - 2016-03-16 13:10 +0100
Re: Suspicious error for CMA stress test Hanjun Guo <guohanjun@huawei.com> - 2016-03-16 10:50 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-17 08:00 +0100
Re: Suspicious error for CMA stress test Hanjun Guo <guohanjun@huawei.com> - 2016-03-17 10:30 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <js1304@gmail.com> - 2016-03-17 16:40 +0100
Re: Suspicious error for CMA stress test Vlastimil Babka <vbabka@suse.cz> - 2016-03-17 16:50 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <js1304@gmail.com> - 2016-03-17 17:00 +0100
Re: Suspicious error for CMA stress test Hanjun Guo <hanjun.guo@linaro.org> - 2016-03-08 05:10 +0100
Re: Suspicious error for CMA stress test Vlastimil Babka <vbabka@suse.cz> - 2016-03-07 14:00 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-08 08:50 +0100
Re: Suspicious error for CMA stress test Xishi Qiu <qiuxishi@huawei.com> - 2016-03-08 11:50 +0100
Re: Suspicious error for CMA stress test Joonsoo Kim <js1304@gmail.com> - 2016-03-08 16:40 +0100
Re: Suspicious error for CMA stress test Xishi Qiu <qiuxishi@huawei.com> - 2016-03-09 03:20 +0100
Page 1 of 2 [1] 2 Next page →
| From | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| Date | 2016-03-07 05:40 +0100 |
| Subject | Re: Suspicious error for CMA stress test |
| Message-ID | <r9V7z-6sn-1@gated-at.bofh.it> |
On Fri, Mar 04, 2016 at 03:35:26PM +0800, Hanjun Guo wrote:
> On 2016/3/4 14:38, Joonsoo Kim wrote:
> > On Fri, Mar 04, 2016 at 02:05:09PM +0800, Hanjun Guo wrote:
> >> On 2016/3/4 12:32, Joonsoo Kim wrote:
> >>> On Fri, Mar 04, 2016 at 11:02:33AM +0900, Joonsoo Kim wrote:
> >>>> On Thu, Mar 03, 2016 at 08:49:01PM +0800, Hanjun Guo wrote:
> >>>>> On 2016/3/3 15:42, Joonsoo Kim wrote:
> >>>>>> 2016-03-03 10:25 GMT+09:00 Laura Abbott <labbott@redhat.com>:
> >>>>>>> (cc -mm and Joonsoo Kim)
> >>>>>>>
> >>>>>>>
> >>>>>>> On 03/02/2016 05:52 AM, Hanjun Guo wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> I came across a suspicious error for CMA stress test:
> >>>>>>>>
> >>>>>>>> Before the test, I got:
> >>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
> >>>>>>>> CmaTotal: 204800 kB
> >>>>>>>> CmaFree: 195044 kB
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> After running the test:
> >>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
> >>>>>>>> CmaTotal: 204800 kB
> >>>>>>>> CmaFree: 6602584 kB
> >>>>>>>>
> >>>>>>>> So the freed CMA memory is more than total..
> >>>>>>>>
> >>>>>>>> Also the the MemFree is more than mem total:
> >>>>>>>>
> >>>>>>>> -bash-4.3# cat /proc/meminfo
> >>>>>>>> MemTotal: 16342016 kB
> >>>>>>>> MemFree: 22367268 kB
> >>>>>>>> MemAvailable: 22370528 kB
> >>>>> [...]
> >>>>>>> I played with this a bit and can see the same problem. The sanity
> >>>>>>> check of CmaFree < CmaTotal generally triggers in
> >>>>>>> __move_zone_freepage_state in unset_migratetype_isolate.
> >>>>>>> This also seems to be present as far back as v4.0 which was the
> >>>>>>> first version to have the updated accounting from Joonsoo.
> >>>>>>> Were there known limitations with the new freepage accounting,
> >>>>>>> Joonsoo?
> >>>>>> I don't know. I also played with this and looks like there is
> >>>>>> accounting problem, however, for my case, number of free page is slightly less
> >>>>>> than total. I will take a look.
> >>>>>>
> >>>>>> Hanjun, could you tell me your malloc_size? I tested with 1 and it doesn't
> >>>>>> look like your case.
> >>>>> I tested with malloc_size with 2M, and it grows much bigger than 1M, also I
> >>>>> did some other test:
> >>>> Thanks! Now, I can re-generate erronous situation you mentioned.
> >>>>
> >>>>> - run with single thread with 100000 times, everything is fine.
> >>>>>
> >>>>> - I hack the cam_alloc() and free as below [1] to see if it's lock issue, with
> >>>>> the same test with 100 multi-thread, then I got:
> >>>> [1] would not be sufficient to close this race.
> >>>>
> >>>> Try following things [A]. And, for more accurate test, I changed code a bit more
> >>>> to prevent kernel page allocation from cma area [B]. This will prevent kernel
> >>>> page allocation from cma area completely so we can focus cma_alloc/release race.
> >>>>
> >>>> Although, this is not correct fix, it could help that we can guess
> >>>> where the problem is.
> >>> More correct fix is something like below.
> >>> Please test it.
> >> Hmm, this is not working:
> > Sad to hear that.
> >
> > Could you tell me your system's MAX_ORDER and pageblock_order?
> >
>
> MAX_ORDER is 11, pageblock_order is 9, thanks for your help!
Hmm... that's same with me.
Below is similar fix that prevents buddy merging when one of buddy's
migrate type, but, not both, is MIGRATE_ISOLATE. In fact, I have
no idea why previous fix (more correct fix) doesn't work for you.
(It works for me.) But, maybe there is a bug on the fix
so I make new one which is more general form. Please test it.
Thanks.
---------->8-------------
From dd41e348572948d70b935fc24f82c096ff0fb417 Mon Sep 17 00:00:00 2001
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Date: Fri, 4 Mar 2016 13:28:17 +0900
Subject: [PATCH] mm/cma: fix race
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
mm/page_alloc.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c6c38ed..d80d071 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -620,8 +620,8 @@ static inline void rmv_page_order(struct page *page)
*
* For recording page's order, we use page_private(page).
*/
-static inline int page_is_buddy(struct page *page, struct page *buddy,
- unsigned int order)
+static inline int page_is_buddy(struct zone *zone, struct page *page,
+ struct page *buddy, unsigned int order)
{
if (!pfn_valid_within(page_to_pfn(buddy)))
return 0;
@@ -644,6 +644,20 @@ static inline int page_is_buddy(struct page *page, struct page *buddy,
if (page_zone_id(page) != page_zone_id(buddy))
return 0;
+ if (IS_ENABLED(CONFIG_CMA) &&
+ unlikely(has_isolate_pageblock(zone)) &&
+ unlikely(order >= pageblock_order)) {
+ int page_mt, buddy_mt;
+
+ page_mt = get_pageblock_migratetype(page);
+ buddy_mt = get_pageblock_migratetype(buddy);
+
+ if (page_mt != buddy_mt &&
+ (is_migrate_isolate(page_mt) ||
+ is_migrate_isolate(buddy_mt)))
+ return 0;
+ }
+
VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy);
return 1;
@@ -691,17 +705,8 @@ static inline void __free_one_page(struct page *page,
VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
VM_BUG_ON(migratetype == -1);
- if (is_migrate_isolate(migratetype)) {
- /*
- * We restrict max order of merging to prevent merge
- * between freepages on isolate pageblock and normal
- * pageblock. Without this, pageblock isolation
- * could cause incorrect freepage accounting.
- */
- max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
- } else {
+ if (!is_migrate_isolate(migratetype))
__mod_zone_freepage_state(zone, 1 << order, migratetype);
- }
page_idx = pfn & ((1 << max_order) - 1);
@@ -711,7 +716,7 @@ static inline void __free_one_page(struct page *page,
while (order < max_order - 1) {
buddy_idx = __find_buddy_index(page_idx, order);
buddy = page + (buddy_idx - page_idx);
- if (!page_is_buddy(page, buddy, order))
+ if (!page_is_buddy(zone, page, buddy, order))
break;
/*
* Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
@@ -745,7 +750,7 @@ static inline void __free_one_page(struct page *page,
higher_page = page + (combined_idx - page_idx);
buddy_idx = __find_buddy_index(combined_idx, order + 1);
higher_buddy = higher_page + (buddy_idx - combined_idx);
- if (page_is_buddy(higher_page, higher_buddy, order + 1)) {
+ if (page_is_buddy(zone, higher_page, higher_buddy, order + 1)) {
list_add_tail(&page->lru,
&zone->free_area[order].free_list[migratetype]);
goto out;
--
1.9.1
[toc] | [next] | [standalone]
| From | "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> |
|---|---|
| Date | 2016-03-07 09:20 +0100 |
| Message-ID | <r9Yyu-jj-5@gated-at.bofh.it> |
| In reply to | #1351317 |
On 2016/3/7 12:34, Joonsoo Kim wrote:
> On Fri, Mar 04, 2016 at 03:35:26PM +0800, Hanjun Guo wrote:
>> On 2016/3/4 14:38, Joonsoo Kim wrote:
>>> On Fri, Mar 04, 2016 at 02:05:09PM +0800, Hanjun Guo wrote:
>>>> On 2016/3/4 12:32, Joonsoo Kim wrote:
>>>>> On Fri, Mar 04, 2016 at 11:02:33AM +0900, Joonsoo Kim wrote:
>>>>>> On Thu, Mar 03, 2016 at 08:49:01PM +0800, Hanjun Guo wrote:
>>>>>>> On 2016/3/3 15:42, Joonsoo Kim wrote:
>>>>>>>> 2016-03-03 10:25 GMT+09:00 Laura Abbott <labbott@redhat.com>:
>>>>>>>>> (cc -mm and Joonsoo Kim)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 03/02/2016 05:52 AM, Hanjun Guo wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> I came across a suspicious error for CMA stress test:
>>>>>>>>>>
>>>>>>>>>> Before the test, I got:
>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>> CmaFree: 195044 kB
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> After running the test:
>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>> CmaFree: 6602584 kB
>>>>>>>>>>
>>>>>>>>>> So the freed CMA memory is more than total..
>>>>>>>>>>
>>>>>>>>>> Also the the MemFree is more than mem total:
>>>>>>>>>>
>>>>>>>>>> -bash-4.3# cat /proc/meminfo
>>>>>>>>>> MemTotal: 16342016 kB
>>>>>>>>>> MemFree: 22367268 kB
>>>>>>>>>> MemAvailable: 22370528 kB
>>>>>>> [...]
>>>>>>>>> I played with this a bit and can see the same problem. The sanity
>>>>>>>>> check of CmaFree < CmaTotal generally triggers in
>>>>>>>>> __move_zone_freepage_state in unset_migratetype_isolate.
>>>>>>>>> This also seems to be present as far back as v4.0 which was the
>>>>>>>>> first version to have the updated accounting from Joonsoo.
>>>>>>>>> Were there known limitations with the new freepage accounting,
>>>>>>>>> Joonsoo?
>>>>>>>> I don't know. I also played with this and looks like there is
>>>>>>>> accounting problem, however, for my case, number of free page is slightly less
>>>>>>>> than total. I will take a look.
>>>>>>>>
>>>>>>>> Hanjun, could you tell me your malloc_size? I tested with 1 and it doesn't
>>>>>>>> look like your case.
>>>>>>> I tested with malloc_size with 2M, and it grows much bigger than 1M, also I
>>>>>>> did some other test:
>>>>>> Thanks! Now, I can re-generate erronous situation you mentioned.
>>>>>>
>>>>>>> - run with single thread with 100000 times, everything is fine.
>>>>>>>
>>>>>>> - I hack the cam_alloc() and free as below [1] to see if it's lock issue, with
>>>>>>> the same test with 100 multi-thread, then I got:
>>>>>> [1] would not be sufficient to close this race.
>>>>>>
>>>>>> Try following things [A]. And, for more accurate test, I changed code a bit more
>>>>>> to prevent kernel page allocation from cma area [B]. This will prevent kernel
>>>>>> page allocation from cma area completely so we can focus cma_alloc/release race.
>>>>>>
>>>>>> Although, this is not correct fix, it could help that we can guess
>>>>>> where the problem is.
>>>>> More correct fix is something like below.
>>>>> Please test it.
>>>> Hmm, this is not working:
>>> Sad to hear that.
>>>
>>> Could you tell me your system's MAX_ORDER and pageblock_order?
>>>
>>
>> MAX_ORDER is 11, pageblock_order is 9, thanks for your help!
>
> Hmm... that's same with me.
>
> Below is similar fix that prevents buddy merging when one of buddy's
> migrate type, but, not both, is MIGRATE_ISOLATE. In fact, I have
> no idea why previous fix (more correct fix) doesn't work for you.
> (It works for me.) But, maybe there is a bug on the fix
> so I make new one which is more general form. Please test it.
Hi,
Hanjun Guo has gone to Tailand on business, so I help him to run this patch. The result
shows that the count of "CmaFree:" is OK now. But sometimes printed some information as below:
alloc_contig_range: [28500, 28600) PFNs busy
alloc_contig_range: [28300, 28380) PFNs busy
>
> Thanks.
>
> ---------->8-------------
>>From dd41e348572948d70b935fc24f82c096ff0fb417 Mon Sep 17 00:00:00 2001
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Date: Fri, 4 Mar 2016 13:28:17 +0900
> Subject: [PATCH] mm/cma: fix race
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
> mm/page_alloc.c | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c6c38ed..d80d071 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -620,8 +620,8 @@ static inline void rmv_page_order(struct page *page)
> *
> * For recording page's order, we use page_private(page).
> */
> -static inline int page_is_buddy(struct page *page, struct page *buddy,
> - unsigned int order)
> +static inline int page_is_buddy(struct zone *zone, struct page *page,
> + struct page *buddy, unsigned int order)
> {
> if (!pfn_valid_within(page_to_pfn(buddy)))
> return 0;
> @@ -644,6 +644,20 @@ static inline int page_is_buddy(struct page *page, struct page *buddy,
> if (page_zone_id(page) != page_zone_id(buddy))
> return 0;
>
> + if (IS_ENABLED(CONFIG_CMA) &&
> + unlikely(has_isolate_pageblock(zone)) &&
> + unlikely(order >= pageblock_order)) {
> + int page_mt, buddy_mt;
> +
> + page_mt = get_pageblock_migratetype(page);
> + buddy_mt = get_pageblock_migratetype(buddy);
> +
> + if (page_mt != buddy_mt &&
> + (is_migrate_isolate(page_mt) ||
> + is_migrate_isolate(buddy_mt)))
> + return 0;
> + }
> +
> VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy);
>
> return 1;
> @@ -691,17 +705,8 @@ static inline void __free_one_page(struct page *page,
> VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
>
> VM_BUG_ON(migratetype == -1);
> - if (is_migrate_isolate(migratetype)) {
> - /*
> - * We restrict max order of merging to prevent merge
> - * between freepages on isolate pageblock and normal
> - * pageblock. Without this, pageblock isolation
> - * could cause incorrect freepage accounting.
> - */
> - max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
> - } else {
> + if (!is_migrate_isolate(migratetype))
> __mod_zone_freepage_state(zone, 1 << order, migratetype);
> - }
>
> page_idx = pfn & ((1 << max_order) - 1);
>
> @@ -711,7 +716,7 @@ static inline void __free_one_page(struct page *page,
> while (order < max_order - 1) {
> buddy_idx = __find_buddy_index(page_idx, order);
> buddy = page + (buddy_idx - page_idx);
> - if (!page_is_buddy(page, buddy, order))
> + if (!page_is_buddy(zone, page, buddy, order))
> break;
> /*
> * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
> @@ -745,7 +750,7 @@ static inline void __free_one_page(struct page *page,
> higher_page = page + (combined_idx - page_idx);
> buddy_idx = __find_buddy_index(combined_idx, order + 1);
> higher_buddy = higher_page + (buddy_idx - combined_idx);
> - if (page_is_buddy(higher_page, higher_buddy, order + 1)) {
> + if (page_is_buddy(zone, higher_page, higher_buddy, order + 1)) {
> list_add_tail(&page->lru,
> &zone->free_area[order].free_list[migratetype]);
> goto out;
>
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2016-03-07 19:50 +0100 |
| Message-ID | <ra8oa-6zK-31@gated-at.bofh.it> |
| In reply to | #1351366 |
On 03/07/2016 12:16 AM, Leizhen (ThunderTown) wrote:
>
>
> On 2016/3/7 12:34, Joonsoo Kim wrote:
>> On Fri, Mar 04, 2016 at 03:35:26PM +0800, Hanjun Guo wrote:
>>> On 2016/3/4 14:38, Joonsoo Kim wrote:
>>>> On Fri, Mar 04, 2016 at 02:05:09PM +0800, Hanjun Guo wrote:
>>>>> On 2016/3/4 12:32, Joonsoo Kim wrote:
>>>>>> On Fri, Mar 04, 2016 at 11:02:33AM +0900, Joonsoo Kim wrote:
>>>>>>> On Thu, Mar 03, 2016 at 08:49:01PM +0800, Hanjun Guo wrote:
>>>>>>>> On 2016/3/3 15:42, Joonsoo Kim wrote:
>>>>>>>>> 2016-03-03 10:25 GMT+09:00 Laura Abbott <labbott@redhat.com>:
>>>>>>>>>> (cc -mm and Joonsoo Kim)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 03/02/2016 05:52 AM, Hanjun Guo wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> I came across a suspicious error for CMA stress test:
>>>>>>>>>>>
>>>>>>>>>>> Before the test, I got:
>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>>> CmaFree: 195044 kB
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> After running the test:
>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>>> CmaFree: 6602584 kB
>>>>>>>>>>>
>>>>>>>>>>> So the freed CMA memory is more than total..
>>>>>>>>>>>
>>>>>>>>>>> Also the the MemFree is more than mem total:
>>>>>>>>>>>
>>>>>>>>>>> -bash-4.3# cat /proc/meminfo
>>>>>>>>>>> MemTotal: 16342016 kB
>>>>>>>>>>> MemFree: 22367268 kB
>>>>>>>>>>> MemAvailable: 22370528 kB
>>>>>>>> [...]
>>>>>>>>>> I played with this a bit and can see the same problem. The sanity
>>>>>>>>>> check of CmaFree < CmaTotal generally triggers in
>>>>>>>>>> __move_zone_freepage_state in unset_migratetype_isolate.
>>>>>>>>>> This also seems to be present as far back as v4.0 which was the
>>>>>>>>>> first version to have the updated accounting from Joonsoo.
>>>>>>>>>> Were there known limitations with the new freepage accounting,
>>>>>>>>>> Joonsoo?
>>>>>>>>> I don't know. I also played with this and looks like there is
>>>>>>>>> accounting problem, however, for my case, number of free page is slightly less
>>>>>>>>> than total. I will take a look.
>>>>>>>>>
>>>>>>>>> Hanjun, could you tell me your malloc_size? I tested with 1 and it doesn't
>>>>>>>>> look like your case.
>>>>>>>> I tested with malloc_size with 2M, and it grows much bigger than 1M, also I
>>>>>>>> did some other test:
>>>>>>> Thanks! Now, I can re-generate erronous situation you mentioned.
>>>>>>>
>>>>>>>> - run with single thread with 100000 times, everything is fine.
>>>>>>>>
>>>>>>>> - I hack the cam_alloc() and free as below [1] to see if it's lock issue, with
>>>>>>>> the same test with 100 multi-thread, then I got:
>>>>>>> [1] would not be sufficient to close this race.
>>>>>>>
>>>>>>> Try following things [A]. And, for more accurate test, I changed code a bit more
>>>>>>> to prevent kernel page allocation from cma area [B]. This will prevent kernel
>>>>>>> page allocation from cma area completely so we can focus cma_alloc/release race.
>>>>>>>
>>>>>>> Although, this is not correct fix, it could help that we can guess
>>>>>>> where the problem is.
>>>>>> More correct fix is something like below.
>>>>>> Please test it.
>>>>> Hmm, this is not working:
>>>> Sad to hear that.
>>>>
>>>> Could you tell me your system's MAX_ORDER and pageblock_order?
>>>>
>>>
>>> MAX_ORDER is 11, pageblock_order is 9, thanks for your help!
>>
>> Hmm... that's same with me.
>>
>> Below is similar fix that prevents buddy merging when one of buddy's
>> migrate type, but, not both, is MIGRATE_ISOLATE. In fact, I have
>> no idea why previous fix (more correct fix) doesn't work for you.
>> (It works for me.) But, maybe there is a bug on the fix
>> so I make new one which is more general form. Please test it.
>
> Hi,
> Hanjun Guo has gone to Tailand on business, so I help him to run this patch. The result
> shows that the count of "CmaFree:" is OK now. But sometimes printed some information as below:
>
> alloc_contig_range: [28500, 28600) PFNs busy
> alloc_contig_range: [28300, 28380) PFNs busy
>
Those messages aren't necessarily a problem. Those messages indicate that
those pages weren't able to be isolated. Given the test here is a
concurrency test, I suspect some concurrent allocation or free prevented
isolation which is to be expected some times. I'd only be concerned if
seeing those messages cause allocation failure or some other notable impact.
Thanks,
Laura
>>
>> Thanks.
>>
>> ---------->8-------------
>> >From dd41e348572948d70b935fc24f82c096ff0fb417 Mon Sep 17 00:00:00 2001
>> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> Date: Fri, 4 Mar 2016 13:28:17 +0900
>> Subject: [PATCH] mm/cma: fix race
>>
>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> ---
>> mm/page_alloc.c | 33 +++++++++++++++++++--------------
>> 1 file changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index c6c38ed..d80d071 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -620,8 +620,8 @@ static inline void rmv_page_order(struct page *page)
>> *
>> * For recording page's order, we use page_private(page).
>> */
>> -static inline int page_is_buddy(struct page *page, struct page *buddy,
>> - unsigned int order)
>> +static inline int page_is_buddy(struct zone *zone, struct page *page,
>> + struct page *buddy, unsigned int order)
>> {
>> if (!pfn_valid_within(page_to_pfn(buddy)))
>> return 0;
>> @@ -644,6 +644,20 @@ static inline int page_is_buddy(struct page *page, struct page *buddy,
>> if (page_zone_id(page) != page_zone_id(buddy))
>> return 0;
>>
>> + if (IS_ENABLED(CONFIG_CMA) &&
>> + unlikely(has_isolate_pageblock(zone)) &&
>> + unlikely(order >= pageblock_order)) {
>> + int page_mt, buddy_mt;
>> +
>> + page_mt = get_pageblock_migratetype(page);
>> + buddy_mt = get_pageblock_migratetype(buddy);
>> +
>> + if (page_mt != buddy_mt &&
>> + (is_migrate_isolate(page_mt) ||
>> + is_migrate_isolate(buddy_mt)))
>> + return 0;
>> + }
>> +
>> VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy);
>>
>> return 1;
>> @@ -691,17 +705,8 @@ static inline void __free_one_page(struct page *page,
>> VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
>>
>> VM_BUG_ON(migratetype == -1);
>> - if (is_migrate_isolate(migratetype)) {
>> - /*
>> - * We restrict max order of merging to prevent merge
>> - * between freepages on isolate pageblock and normal
>> - * pageblock. Without this, pageblock isolation
>> - * could cause incorrect freepage accounting.
>> - */
>> - max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
>> - } else {
>> + if (!is_migrate_isolate(migratetype))
>> __mod_zone_freepage_state(zone, 1 << order, migratetype);
>> - }
>>
>> page_idx = pfn & ((1 << max_order) - 1);
>>
>> @@ -711,7 +716,7 @@ static inline void __free_one_page(struct page *page,
>> while (order < max_order - 1) {
>> buddy_idx = __find_buddy_index(page_idx, order);
>> buddy = page + (buddy_idx - page_idx);
>> - if (!page_is_buddy(page, buddy, order))
>> + if (!page_is_buddy(zone, page, buddy, order))
>> break;
>> /*
>> * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
>> @@ -745,7 +750,7 @@ static inline void __free_one_page(struct page *page,
>> higher_page = page + (combined_idx - page_idx);
>> buddy_idx = __find_buddy_index(combined_idx, order + 1);
>> higher_buddy = higher_page + (buddy_idx - combined_idx);
>> - if (page_is_buddy(higher_page, higher_buddy, order + 1)) {
>> + if (page_is_buddy(zone, higher_page, higher_buddy, order + 1)) {
>> list_add_tail(&page->lru,
>> &zone->free_area[order].free_list[migratetype]);
>> goto out;
>>
>
[toc] | [prev] | [next] | [standalone]
| From | "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> |
|---|---|
| Date | 2016-03-08 03:00 +0100 |
| Message-ID | <raf6i-2wB-11@gated-at.bofh.it> |
| In reply to | #1351880 |
On 2016/3/8 2:42, Laura Abbott wrote:
> On 03/07/2016 12:16 AM, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2016/3/7 12:34, Joonsoo Kim wrote:
>>> On Fri, Mar 04, 2016 at 03:35:26PM +0800, Hanjun Guo wrote:
>>>> On 2016/3/4 14:38, Joonsoo Kim wrote:
>>>>> On Fri, Mar 04, 2016 at 02:05:09PM +0800, Hanjun Guo wrote:
>>>>>> On 2016/3/4 12:32, Joonsoo Kim wrote:
>>>>>>> On Fri, Mar 04, 2016 at 11:02:33AM +0900, Joonsoo Kim wrote:
>>>>>>>> On Thu, Mar 03, 2016 at 08:49:01PM +0800, Hanjun Guo wrote:
>>>>>>>>> On 2016/3/3 15:42, Joonsoo Kim wrote:
>>>>>>>>>> 2016-03-03 10:25 GMT+09:00 Laura Abbott <labbott@redhat.com>:
>>>>>>>>>>> (cc -mm and Joonsoo Kim)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 03/02/2016 05:52 AM, Hanjun Guo wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> I came across a suspicious error for CMA stress test:
>>>>>>>>>>>>
>>>>>>>>>>>> Before the test, I got:
>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>>>> CmaFree: 195044 kB
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> After running the test:
>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>>>> CmaFree: 6602584 kB
>>>>>>>>>>>>
>>>>>>>>>>>> So the freed CMA memory is more than total..
>>>>>>>>>>>>
>>>>>>>>>>>> Also the the MemFree is more than mem total:
>>>>>>>>>>>>
>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo
>>>>>>>>>>>> MemTotal: 16342016 kB
>>>>>>>>>>>> MemFree: 22367268 kB
>>>>>>>>>>>> MemAvailable: 22370528 kB
>>>>>>>>> [...]
>>>>>>>>>>> I played with this a bit and can see the same problem. The sanity
>>>>>>>>>>> check of CmaFree < CmaTotal generally triggers in
>>>>>>>>>>> __move_zone_freepage_state in unset_migratetype_isolate.
>>>>>>>>>>> This also seems to be present as far back as v4.0 which was the
>>>>>>>>>>> first version to have the updated accounting from Joonsoo.
>>>>>>>>>>> Were there known limitations with the new freepage accounting,
>>>>>>>>>>> Joonsoo?
>>>>>>>>>> I don't know. I also played with this and looks like there is
>>>>>>>>>> accounting problem, however, for my case, number of free page is slightly less
>>>>>>>>>> than total. I will take a look.
>>>>>>>>>>
>>>>>>>>>> Hanjun, could you tell me your malloc_size? I tested with 1 and it doesn't
>>>>>>>>>> look like your case.
>>>>>>>>> I tested with malloc_size with 2M, and it grows much bigger than 1M, also I
>>>>>>>>> did some other test:
>>>>>>>> Thanks! Now, I can re-generate erronous situation you mentioned.
>>>>>>>>
>>>>>>>>> - run with single thread with 100000 times, everything is fine.
>>>>>>>>>
>>>>>>>>> - I hack the cam_alloc() and free as below [1] to see if it's lock issue, with
>>>>>>>>> the same test with 100 multi-thread, then I got:
>>>>>>>> [1] would not be sufficient to close this race.
>>>>>>>>
>>>>>>>> Try following things [A]. And, for more accurate test, I changed code a bit more
>>>>>>>> to prevent kernel page allocation from cma area [B]. This will prevent kernel
>>>>>>>> page allocation from cma area completely so we can focus cma_alloc/release race.
>>>>>>>>
>>>>>>>> Although, this is not correct fix, it could help that we can guess
>>>>>>>> where the problem is.
>>>>>>> More correct fix is something like below.
>>>>>>> Please test it.
>>>>>> Hmm, this is not working:
>>>>> Sad to hear that.
>>>>>
>>>>> Could you tell me your system's MAX_ORDER and pageblock_order?
>>>>>
>>>>
>>>> MAX_ORDER is 11, pageblock_order is 9, thanks for your help!
>>>
>>> Hmm... that's same with me.
>>>
>>> Below is similar fix that prevents buddy merging when one of buddy's
>>> migrate type, but, not both, is MIGRATE_ISOLATE. In fact, I have
>>> no idea why previous fix (more correct fix) doesn't work for you.
>>> (It works for me.) But, maybe there is a bug on the fix
>>> so I make new one which is more general form. Please test it.
>>
>> Hi,
>> Hanjun Guo has gone to Tailand on business, so I help him to run this patch. The result
>> shows that the count of "CmaFree:" is OK now. But sometimes printed some information as below:
>>
>> alloc_contig_range: [28500, 28600) PFNs busy
>> alloc_contig_range: [28300, 28380) PFNs busy
>>
>
> Those messages aren't necessarily a problem. Those messages indicate that
OK.
> those pages weren't able to be isolated. Given the test here is a
> concurrency test, I suspect some concurrent allocation or free prevented
> isolation which is to be expected some times. I'd only be concerned if
> seeing those messages cause allocation failure or some other notable impact.
I chose memory block size: 512K, 1M, 2M ran serveral times, there was no memory allocation failure.
>
> Thanks,
> Laura
>
>>>
>>> Thanks.
>>>
>>> ---------->8-------------
>>> >From dd41e348572948d70b935fc24f82c096ff0fb417 Mon Sep 17 00:00:00 2001
>>> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>> Date: Fri, 4 Mar 2016 13:28:17 +0900
>>> Subject: [PATCH] mm/cma: fix race
>>>
>>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>> ---
>>> mm/page_alloc.c | 33 +++++++++++++++++++--------------
>>> 1 file changed, 19 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index c6c38ed..d80d071 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -620,8 +620,8 @@ static inline void rmv_page_order(struct page *page)
>>> *
>>> * For recording page's order, we use page_private(page).
>>> */
>>> -static inline int page_is_buddy(struct page *page, struct page *buddy,
>>> - unsigned int order)
>>> +static inline int page_is_buddy(struct zone *zone, struct page *page,
>>> + struct page *buddy, unsigned int order)
>>> {
>>> if (!pfn_valid_within(page_to_pfn(buddy)))
>>> return 0;
>>> @@ -644,6 +644,20 @@ static inline int page_is_buddy(struct page *page, struct page *buddy,
>>> if (page_zone_id(page) != page_zone_id(buddy))
>>> return 0;
>>>
>>> + if (IS_ENABLED(CONFIG_CMA) &&
>>> + unlikely(has_isolate_pageblock(zone)) &&
>>> + unlikely(order >= pageblock_order)) {
>>> + int page_mt, buddy_mt;
>>> +
>>> + page_mt = get_pageblock_migratetype(page);
>>> + buddy_mt = get_pageblock_migratetype(buddy);
>>> +
>>> + if (page_mt != buddy_mt &&
>>> + (is_migrate_isolate(page_mt) ||
>>> + is_migrate_isolate(buddy_mt)))
>>> + return 0;
>>> + }
>>> +
>>> VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy);
>>>
>>> return 1;
>>> @@ -691,17 +705,8 @@ static inline void __free_one_page(struct page *page,
>>> VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
>>>
>>> VM_BUG_ON(migratetype == -1);
>>> - if (is_migrate_isolate(migratetype)) {
>>> - /*
>>> - * We restrict max order of merging to prevent merge
>>> - * between freepages on isolate pageblock and normal
>>> - * pageblock. Without this, pageblock isolation
>>> - * could cause incorrect freepage accounting.
>>> - */
>>> - max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
>>> - } else {
>>> + if (!is_migrate_isolate(migratetype))
>>> __mod_zone_freepage_state(zone, 1 << order, migratetype);
>>> - }
>>>
>>> page_idx = pfn & ((1 << max_order) - 1);
>>>
>>> @@ -711,7 +716,7 @@ static inline void __free_one_page(struct page *page,
>>> while (order < max_order - 1) {
>>> buddy_idx = __find_buddy_index(page_idx, order);
>>> buddy = page + (buddy_idx - page_idx);
>>> - if (!page_is_buddy(page, buddy, order))
>>> + if (!page_is_buddy(zone, page, buddy, order))
>>> break;
>>> /*
>>> * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
>>> @@ -745,7 +750,7 @@ static inline void __free_one_page(struct page *page,
>>> higher_page = page + (combined_idx - page_idx);
>>> buddy_idx = __find_buddy_index(combined_idx, order + 1);
>>> higher_buddy = higher_page + (buddy_idx - combined_idx);
>>> - if (page_is_buddy(higher_page, higher_buddy, order + 1)) {
>>> + if (page_is_buddy(zone, higher_page, higher_buddy, order + 1)) {
>>> list_add_tail(&page->lru,
>>> &zone->free_area[order].free_list[migratetype]);
>>> goto out;
>>>
>>
>
>
> .
>
[toc] | [prev] | [next] | [standalone]
| From | "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> |
|---|---|
| Date | 2016-03-09 02:30 +0100 |
| Message-ID | <raB6N-IC-3@gated-at.bofh.it> |
| In reply to | #1352551 |
On 2016/3/8 9:54, Leizhen (ThunderTown) wrote:
>
>
> On 2016/3/8 2:42, Laura Abbott wrote:
>> On 03/07/2016 12:16 AM, Leizhen (ThunderTown) wrote:
>>>
>>>
>>> On 2016/3/7 12:34, Joonsoo Kim wrote:
>>>> On Fri, Mar 04, 2016 at 03:35:26PM +0800, Hanjun Guo wrote:
>>>>> On 2016/3/4 14:38, Joonsoo Kim wrote:
>>>>>> On Fri, Mar 04, 2016 at 02:05:09PM +0800, Hanjun Guo wrote:
>>>>>>> On 2016/3/4 12:32, Joonsoo Kim wrote:
>>>>>>>> On Fri, Mar 04, 2016 at 11:02:33AM +0900, Joonsoo Kim wrote:
>>>>>>>>> On Thu, Mar 03, 2016 at 08:49:01PM +0800, Hanjun Guo wrote:
>>>>>>>>>> On 2016/3/3 15:42, Joonsoo Kim wrote:
>>>>>>>>>>> 2016-03-03 10:25 GMT+09:00 Laura Abbott <labbott@redhat.com>:
>>>>>>>>>>>> (cc -mm and Joonsoo Kim)
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 03/02/2016 05:52 AM, Hanjun Guo wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I came across a suspicious error for CMA stress test:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Before the test, I got:
>>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>>>>> CmaFree: 195044 kB
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> After running the test:
>>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma
>>>>>>>>>>>>> CmaTotal: 204800 kB
>>>>>>>>>>>>> CmaFree: 6602584 kB
>>>>>>>>>>>>>
>>>>>>>>>>>>> So the freed CMA memory is more than total..
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also the the MemFree is more than mem total:
>>>>>>>>>>>>>
>>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo
>>>>>>>>>>>>> MemTotal: 16342016 kB
>>>>>>>>>>>>> MemFree: 22367268 kB
>>>>>>>>>>>>> MemAvailable: 22370528 kB
>>>>>>>>>> [...]
>>>>>>>>>>>> I played with this a bit and can see the same problem. The sanity
>>>>>>>>>>>> check of CmaFree < CmaTotal generally triggers in
>>>>>>>>>>>> __move_zone_freepage_state in unset_migratetype_isolate.
>>>>>>>>>>>> This also seems to be present as far back as v4.0 which was the
>>>>>>>>>>>> first version to have the updated accounting from Joonsoo.
>>>>>>>>>>>> Were there known limitations with the new freepage accounting,
>>>>>>>>>>>> Joonsoo?
>>>>>>>>>>> I don't know. I also played with this and looks like there is
>>>>>>>>>>> accounting problem, however, for my case, number of free page is slightly less
>>>>>>>>>>> than total. I will take a look.
>>>>>>>>>>>
>>>>>>>>>>> Hanjun, could you tell me your malloc_size? I tested with 1 and it doesn't
>>>>>>>>>>> look like your case.
>>>>>>>>>> I tested with malloc_size with 2M, and it grows much bigger than 1M, also I
>>>>>>>>>> did some other test:
>>>>>>>>> Thanks! Now, I can re-generate erronous situation you mentioned.
>>>>>>>>>
>>>>>>>>>> - run with single thread with 100000 times, everything is fine.
>>>>>>>>>>
>>>>>>>>>> - I hack the cam_alloc() and free as below [1] to see if it's lock issue, with
>>>>>>>>>> the same test with 100 multi-thread, then I got:
>>>>>>>>> [1] would not be sufficient to close this race.
>>>>>>>>>
>>>>>>>>> Try following things [A]. And, for more accurate test, I changed code a bit more
>>>>>>>>> to prevent kernel page allocation from cma area [B]. This will prevent kernel
>>>>>>>>> page allocation from cma area completely so we can focus cma_alloc/release race.
>>>>>>>>>
>>>>>>>>> Although, this is not correct fix, it could help that we can guess
>>>>>>>>> where the problem is.
>>>>>>>> More correct fix is something like below.
>>>>>>>> Please test it.
>>>>>>> Hmm, this is not working:
>>>>>> Sad to hear that.
>>>>>>
>>>>>> Could you tell me your system's MAX_ORDER and pageblock_order?
>>>>>>
>>>>>
>>>>> MAX_ORDER is 11, pageblock_order is 9, thanks for your help!
>>>>
>>>> Hmm... that's same with me.
>>>>
>>>> Below is similar fix that prevents buddy merging when one of buddy's
>>>> migrate type, but, not both, is MIGRATE_ISOLATE. In fact, I have
>>>> no idea why previous fix (more correct fix) doesn't work for you.
>>>> (It works for me.) But, maybe there is a bug on the fix
>>>> so I make new one which is more general form. Please test it.
>>>
>>> Hi,
>>> Hanjun Guo has gone to Tailand on business, so I help him to run this patch. The result
>>> shows that the count of "CmaFree:" is OK now. But sometimes printed some information as below:
>>>
>>> alloc_contig_range: [28500, 28600) PFNs busy
>>> alloc_contig_range: [28300, 28380) PFNs busy
>>>
>>
>> Those messages aren't necessarily a problem. Those messages indicate that
> OK.
>
>> those pages weren't able to be isolated. Given the test here is a
>> concurrency test, I suspect some concurrent allocation or free prevented
>> isolation which is to be expected some times. I'd only be concerned if
>> seeing those messages cause allocation failure or some other notable impact.
> I chose memory block size: 512K, 1M, 2M ran serveral times, there was no memory allocation failure.
Hi, Joonsoo:
This new patch worked well. Do you plan to upstream it in the near furture?
>
>>
>> Thanks,
>> Laura
>>
>>>>
>>>> Thanks.
>>>>
>>>> ---------->8-------------
>>>> >From dd41e348572948d70b935fc24f82c096ff0fb417 Mon Sep 17 00:00:00 2001
>>>> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>>> Date: Fri, 4 Mar 2016 13:28:17 +0900
>>>> Subject: [PATCH] mm/cma: fix race
>>>>
>>>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>>> ---
>>>> mm/page_alloc.c | 33 +++++++++++++++++++--------------
>>>> 1 file changed, 19 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index c6c38ed..d80d071 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -620,8 +620,8 @@ static inline void rmv_page_order(struct page *page)
>>>> *
>>>> * For recording page's order, we use page_private(page).
>>>> */
>>>> -static inline int page_is_buddy(struct page *page, struct page *buddy,
>>>> - unsigned int order)
>>>> +static inline int page_is_buddy(struct zone *zone, struct page *page,
>>>> + struct page *buddy, unsigned int order)
>>>> {
>>>> if (!pfn_valid_within(page_to_pfn(buddy)))
>>>> return 0;
>>>> @@ -644,6 +644,20 @@ static inline int page_is_buddy(struct page *page, struct page *buddy,
>>>> if (page_zone_id(page) != page_zone_id(buddy))
>>>> return 0;
>>>>
>>>> + if (IS_ENABLED(CONFIG_CMA) &&
>>>> + unlikely(has_isolate_pageblock(zone)) &&
>>>> + unlikely(order >= pageblock_order)) {
>>>> + int page_mt, buddy_mt;
>>>> +
>>>> + page_mt = get_pageblock_migratetype(page);
>>>> + buddy_mt = get_pageblock_migratetype(buddy);
>>>> +
>>>> + if (page_mt != buddy_mt &&
>>>> + (is_migrate_isolate(page_mt) ||
>>>> + is_migrate_isolate(buddy_mt)))
>>>> + return 0;
>>>> + }
>>>> +
>>>> VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy);
>>>>
>>>> return 1;
>>>> @@ -691,17 +705,8 @@ static inline void __free_one_page(struct page *page,
>>>> VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
>>>>
>>>> VM_BUG_ON(migratetype == -1);
>>>> - if (is_migrate_isolate(migratetype)) {
>>>> - /*
>>>> - * We restrict max order of merging to prevent merge
>>>> - * between freepages on isolate pageblock and normal
>>>> - * pageblock. Without this, pageblock isolation
>>>> - * could cause incorrect freepage accounting.
>>>> - */
>>>> - max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
>>>> - } else {
>>>> + if (!is_migrate_isolate(migratetype))
>>>> __mod_zone_freepage_state(zone, 1 << order, migratetype);
>>>> - }
>>>>
>>>> page_idx = pfn & ((1 << max_order) - 1);
>>>>
>>>> @@ -711,7 +716,7 @@ static inline void __free_one_page(struct page *page,
>>>> while (order < max_order - 1) {
>>>> buddy_idx = __find_buddy_index(page_idx, order);
>>>> buddy = page + (buddy_idx - page_idx);
>>>> - if (!page_is_buddy(page, buddy, order))
>>>> + if (!page_is_buddy(zone, page, buddy, order))
>>>> break;
>>>> /*
>>>> * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
>>>> @@ -745,7 +750,7 @@ static inline void __free_one_page(struct page *page,
>>>> higher_page = page + (combined_idx - page_idx);
>>>> buddy_idx = __find_buddy_index(combined_idx, order + 1);
>>>> higher_buddy = higher_page + (buddy_idx - combined_idx);
>>>> - if (page_is_buddy(higher_page, higher_buddy, order + 1)) {
>>>> + if (page_is_buddy(zone, higher_page, higher_buddy, order + 1)) {
>>>> list_add_tail(&page->lru,
>>>> &zone->free_area[order].free_list[migratetype]);
>>>> goto out;
>>>>
>>>
>>
>>
>> .
>>
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <js1304@gmail.com> |
|---|---|
| Date | 2016-03-11 16:10 +0100 |
| Message-ID | <rbwRs-7YD-17@gated-at.bofh.it> |
| In reply to | #1353706 |
2016-03-09 10:23 GMT+09:00 Leizhen (ThunderTown) <thunder.leizhen@huawei.com>: > > > On 2016/3/8 9:54, Leizhen (ThunderTown) wrote: >> >> >> On 2016/3/8 2:42, Laura Abbott wrote: >>> On 03/07/2016 12:16 AM, Leizhen (ThunderTown) wrote: >>>> >>>> >>>> On 2016/3/7 12:34, Joonsoo Kim wrote: >>>>> On Fri, Mar 04, 2016 at 03:35:26PM +0800, Hanjun Guo wrote: >>>>>> On 2016/3/4 14:38, Joonsoo Kim wrote: >>>>>>> On Fri, Mar 04, 2016 at 02:05:09PM +0800, Hanjun Guo wrote: >>>>>>>> On 2016/3/4 12:32, Joonsoo Kim wrote: >>>>>>>>> On Fri, Mar 04, 2016 at 11:02:33AM +0900, Joonsoo Kim wrote: >>>>>>>>>> On Thu, Mar 03, 2016 at 08:49:01PM +0800, Hanjun Guo wrote: >>>>>>>>>>> On 2016/3/3 15:42, Joonsoo Kim wrote: >>>>>>>>>>>> 2016-03-03 10:25 GMT+09:00 Laura Abbott <labbott@redhat.com>: >>>>>>>>>>>>> (cc -mm and Joonsoo Kim) >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 03/02/2016 05:52 AM, Hanjun Guo wrote: >>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I came across a suspicious error for CMA stress test: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Before the test, I got: >>>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma >>>>>>>>>>>>>> CmaTotal: 204800 kB >>>>>>>>>>>>>> CmaFree: 195044 kB >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> After running the test: >>>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma >>>>>>>>>>>>>> CmaTotal: 204800 kB >>>>>>>>>>>>>> CmaFree: 6602584 kB >>>>>>>>>>>>>> >>>>>>>>>>>>>> So the freed CMA memory is more than total.. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Also the the MemFree is more than mem total: >>>>>>>>>>>>>> >>>>>>>>>>>>>> -bash-4.3# cat /proc/meminfo >>>>>>>>>>>>>> MemTotal: 16342016 kB >>>>>>>>>>>>>> MemFree: 22367268 kB >>>>>>>>>>>>>> MemAvailable: 22370528 kB >>>>>>>>>>> [...] >>>>>>>>>>>>> I played with this a bit and can see the same problem. The sanity >>>>>>>>>>>>> check of CmaFree < CmaTotal generally triggers in >>>>>>>>>>>>> __move_zone_freepage_state in unset_migratetype_isolate. >>>>>>>>>>>>> This also seems to be present as far back as v4.0 which was the >>>>>>>>>>>>> first version to have the updated accounting from Joonsoo. >>>>>>>>>>>>> Were there known limitations with the new freepage accounting, >>>>>>>>>>>>> Joonsoo? >>>>>>>>>>>> I don't know. I also played with this and looks like there is >>>>>>>>>>>> accounting problem, however, for my case, number of free page is slightly less >>>>>>>>>>>> than total. I will take a look. >>>>>>>>>>>> >>>>>>>>>>>> Hanjun, could you tell me your malloc_size? I tested with 1 and it doesn't >>>>>>>>>>>> look like your case. >>>>>>>>>>> I tested with malloc_size with 2M, and it grows much bigger than 1M, also I >>>>>>>>>>> did some other test: >>>>>>>>>> Thanks! Now, I can re-generate erronous situation you mentioned. >>>>>>>>>> >>>>>>>>>>> - run with single thread with 100000 times, everything is fine. >>>>>>>>>>> >>>>>>>>>>> - I hack the cam_alloc() and free as below [1] to see if it's lock issue, with >>>>>>>>>>> the same test with 100 multi-thread, then I got: >>>>>>>>>> [1] would not be sufficient to close this race. >>>>>>>>>> >>>>>>>>>> Try following things [A]. And, for more accurate test, I changed code a bit more >>>>>>>>>> to prevent kernel page allocation from cma area [B]. This will prevent kernel >>>>>>>>>> page allocation from cma area completely so we can focus cma_alloc/release race. >>>>>>>>>> >>>>>>>>>> Although, this is not correct fix, it could help that we can guess >>>>>>>>>> where the problem is. >>>>>>>>> More correct fix is something like below. >>>>>>>>> Please test it. >>>>>>>> Hmm, this is not working: >>>>>>> Sad to hear that. >>>>>>> >>>>>>> Could you tell me your system's MAX_ORDER and pageblock_order? >>>>>>> >>>>>> >>>>>> MAX_ORDER is 11, pageblock_order is 9, thanks for your help! >>>>> >>>>> Hmm... that's same with me. >>>>> >>>>> Below is similar fix that prevents buddy merging when one of buddy's >>>>> migrate type, but, not both, is MIGRATE_ISOLATE. In fact, I have >>>>> no idea why previous fix (more correct fix) doesn't work for you. >>>>> (It works for me.) But, maybe there is a bug on the fix >>>>> so I make new one which is more general form. Please test it. >>>> >>>> Hi, >>>> Hanjun Guo has gone to Tailand on business, so I help him to run this patch. The result >>>> shows that the count of "CmaFree:" is OK now. But sometimes printed some information as below: >>>> >>>> alloc_contig_range: [28500, 28600) PFNs busy >>>> alloc_contig_range: [28300, 28380) PFNs busy >>>> >>> >>> Those messages aren't necessarily a problem. Those messages indicate that >> OK. >> >>> those pages weren't able to be isolated. Given the test here is a >>> concurrency test, I suspect some concurrent allocation or free prevented >>> isolation which is to be expected some times. I'd only be concerned if >>> seeing those messages cause allocation failure or some other notable impact. >> I chose memory block size: 512K, 1M, 2M ran serveral times, there was no memory allocation failure. > > Hi, Joonsoo: > This new patch worked well. Do you plan to upstream it in the near furture? Of course! But, I should think more because it touches allocator's fastpatch and I'd like to detour. If I fail to think a better solution, I will send it as is, soon. Thanks.
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-03-11 18:10 +0100 |
| Message-ID | <rbyJz-11y-1@gated-at.bofh.it> |
| In reply to | #1355976 |
On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
> 2016-03-09 10:23 GMT+09:00 Leizhen (ThunderTown) <thunder.leizhen@huawei.com>:
>>
>> Hi, Joonsoo:
>> This new patch worked well. Do you plan to upstream it in the near furture?
>
> Of course!
> But, I should think more because it touches allocator's fastpatch and
> I'd like to detour.
> If I fail to think a better solution, I will send it as is, soon.
How about something like this? Just and idea, probably buggy (off-by-one etc.).
Should keep away cost from <pageblock_order iterations at the expense of the
relatively fewer >pageblock_order iterations.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ff1e3cbc8956..b8005a07b2a1 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -685,21 +685,13 @@ static inline void __free_one_page(struct page *page,
unsigned long combined_idx;
unsigned long uninitialized_var(buddy_idx);
struct page *buddy;
- unsigned int max_order = MAX_ORDER;
+ unsigned int max_order = pageblock_order + 1;
VM_BUG_ON(!zone_is_initialized(zone));
VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
VM_BUG_ON(migratetype == -1);
- if (is_migrate_isolate(migratetype)) {
- /*
- * We restrict max order of merging to prevent merge
- * between freepages on isolate pageblock and normal
- * pageblock. Without this, pageblock isolation
- * could cause incorrect freepage accounting.
- */
- max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
- } else {
+ if (likely(!is_migrate_isolate(migratetype))) {
__mod_zone_freepage_state(zone, 1 << order, migratetype);
}
@@ -708,11 +700,12 @@ static inline void __free_one_page(struct page *page,
VM_BUG_ON_PAGE(page_idx & ((1 << order) - 1), page);
VM_BUG_ON_PAGE(bad_range(zone, page), page);
+continue_merging:
while (order < max_order - 1) {
buddy_idx = __find_buddy_index(page_idx, order);
buddy = page + (buddy_idx - page_idx);
if (!page_is_buddy(page, buddy, order))
- break;
+ goto done_merging;
/*
* Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
* merge with it and move up one order.
@@ -729,6 +722,26 @@ static inline void __free_one_page(struct page *page,
page_idx = combined_idx;
order++;
}
+ if (max_order < MAX_ORDER) {
+ if (IS_ENABLED(CONFIG_CMA) &&
+ unlikely(has_isolate_pageblock(zone))) {
+
+ int buddy_mt;
+
+ buddy_idx = __find_buddy_index(page_idx, order);
+ buddy = page + (buddy_idx - page_idx);
+ buddy_mt = get_pageblock_migratetype(buddy);
+
+ if (migratetype != buddy_mt &&
+ (is_migrate_isolate(migratetype) ||
+ is_migrate_isolate(buddy_mt)))
+ goto done_merging;
+ }
+ max_order++;
+ goto continue_merging;
+ }
+
+done_merging:
set_page_order(page, order);
/*
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| Date | 2016-03-14 07:50 +0100 |
| Message-ID | <rcuue-8R-19@gated-at.bofh.it> |
| In reply to | #1356063 |
On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
> > 2016-03-09 10:23 GMT+09:00 Leizhen (ThunderTown) <thunder.leizhen@huawei.com>:
> >>
> >> Hi, Joonsoo:
> >> This new patch worked well. Do you plan to upstream it in the near furture?
> >
> > Of course!
> > But, I should think more because it touches allocator's fastpatch and
> > I'd like to detour.
> > If I fail to think a better solution, I will send it as is, soon.
>
> How about something like this? Just and idea, probably buggy (off-by-one etc.).
> Should keep away cost from <pageblock_order iterations at the expense of the
> relatively fewer >pageblock_order iterations.
Hmm... I tested this and found that it's code size is a little bit
larger than mine. I'm not sure why this happens exactly but I guess it would be
related to compiler optimization. In this case, I'm in favor of my
implementation because it looks like well abstraction. It adds one
unlikely branch to the merge loop but compiler would optimize it to
check it once.
Thanks.
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ff1e3cbc8956..b8005a07b2a1 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -685,21 +685,13 @@ static inline void __free_one_page(struct page *page,
> unsigned long combined_idx;
> unsigned long uninitialized_var(buddy_idx);
> struct page *buddy;
> - unsigned int max_order = MAX_ORDER;
> + unsigned int max_order = pageblock_order + 1;
>
> VM_BUG_ON(!zone_is_initialized(zone));
> VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
>
> VM_BUG_ON(migratetype == -1);
> - if (is_migrate_isolate(migratetype)) {
> - /*
> - * We restrict max order of merging to prevent merge
> - * between freepages on isolate pageblock and normal
> - * pageblock. Without this, pageblock isolation
> - * could cause incorrect freepage accounting.
> - */
> - max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
> - } else {
> + if (likely(!is_migrate_isolate(migratetype))) {
> __mod_zone_freepage_state(zone, 1 << order, migratetype);
> }
>
> @@ -708,11 +700,12 @@ static inline void __free_one_page(struct page *page,
> VM_BUG_ON_PAGE(page_idx & ((1 << order) - 1), page);
> VM_BUG_ON_PAGE(bad_range(zone, page), page);
>
> +continue_merging:
> while (order < max_order - 1) {
> buddy_idx = __find_buddy_index(page_idx, order);
> buddy = page + (buddy_idx - page_idx);
> if (!page_is_buddy(page, buddy, order))
> - break;
> + goto done_merging;
> /*
> * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
> * merge with it and move up one order.
> @@ -729,6 +722,26 @@ static inline void __free_one_page(struct page *page,
> page_idx = combined_idx;
> order++;
> }
> + if (max_order < MAX_ORDER) {
> + if (IS_ENABLED(CONFIG_CMA) &&
> + unlikely(has_isolate_pageblock(zone))) {
> +
> + int buddy_mt;
> +
> + buddy_idx = __find_buddy_index(page_idx, order);
> + buddy = page + (buddy_idx - page_idx);
> + buddy_mt = get_pageblock_migratetype(buddy);
> +
> + if (migratetype != buddy_mt &&
> + (is_migrate_isolate(migratetype) ||
> + is_migrate_isolate(buddy_mt)))
> + goto done_merging;
> + }
> + max_order++;
> + goto continue_merging;
> + }
> +
> +done_merging:
> set_page_order(page, order);
>
> /*
>
> --
> 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>
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-03-14 08:10 +0100 |
| Message-ID | <rcuNz-ve-3@gated-at.bofh.it> |
| In reply to | #1356969 |
On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
>>
>> How about something like this? Just and idea, probably buggy (off-by-one etc.).
>> Should keep away cost from <pageblock_order iterations at the expense of the
>> relatively fewer >pageblock_order iterations.
>
> Hmm... I tested this and found that it's code size is a little bit
> larger than mine. I'm not sure why this happens exactly but I guess it would be
> related to compiler optimization. In this case, I'm in favor of my
> implementation because it looks like well abstraction. It adds one
> unlikely branch to the merge loop but compiler would optimize it to
> check it once.
I would be surprised if compiler optimized that to check it once, as
order increases with each loop iteration. But maybe it's smart enough to
do something like I did by hand? Guess I'll check the disassembly.
>
> Thanks.
>
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index ff1e3cbc8956..b8005a07b2a1 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -685,21 +685,13 @@ static inline void __free_one_page(struct page *page,
>> unsigned long combined_idx;
>> unsigned long uninitialized_var(buddy_idx);
>> struct page *buddy;
>> - unsigned int max_order = MAX_ORDER;
>> + unsigned int max_order = pageblock_order + 1;
>>
>> VM_BUG_ON(!zone_is_initialized(zone));
>> VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
>>
>> VM_BUG_ON(migratetype == -1);
>> - if (is_migrate_isolate(migratetype)) {
>> - /*
>> - * We restrict max order of merging to prevent merge
>> - * between freepages on isolate pageblock and normal
>> - * pageblock. Without this, pageblock isolation
>> - * could cause incorrect freepage accounting.
>> - */
>> - max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
>> - } else {
>> + if (likely(!is_migrate_isolate(migratetype))) {
>> __mod_zone_freepage_state(zone, 1 << order, migratetype);
>> }
>>
>> @@ -708,11 +700,12 @@ static inline void __free_one_page(struct page *page,
>> VM_BUG_ON_PAGE(page_idx & ((1 << order) - 1), page);
>> VM_BUG_ON_PAGE(bad_range(zone, page), page);
>>
>> +continue_merging:
>> while (order < max_order - 1) {
>> buddy_idx = __find_buddy_index(page_idx, order);
>> buddy = page + (buddy_idx - page_idx);
>> if (!page_is_buddy(page, buddy, order))
>> - break;
>> + goto done_merging;
>> /*
>> * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
>> * merge with it and move up one order.
>> @@ -729,6 +722,26 @@ static inline void __free_one_page(struct page *page,
>> page_idx = combined_idx;
>> order++;
>> }
>> + if (max_order < MAX_ORDER) {
>> + if (IS_ENABLED(CONFIG_CMA) &&
>> + unlikely(has_isolate_pageblock(zone))) {
>> +
>> + int buddy_mt;
>> +
>> + buddy_idx = __find_buddy_index(page_idx, order);
>> + buddy = page + (buddy_idx - page_idx);
>> + buddy_mt = get_pageblock_migratetype(buddy);
>> +
>> + if (migratetype != buddy_mt &&
>> + (is_migrate_isolate(migratetype) ||
>> + is_migrate_isolate(buddy_mt)))
>> + goto done_merging;
>> + }
>> + max_order++;
>> + goto continue_merging;
>> + }
>> +
>> +done_merging:
>> set_page_order(page, order);
>>
>> /*
>>
>> --
>> 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>
>
> --
> 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>
>
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| Date | 2016-03-14 08:20 +0100 |
| Message-ID | <rcuXf-yJ-1@gated-at.bofh.it> |
| In reply to | #1356981 |
On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote:
> On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
> >On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
> >>On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
> >>
> >>How about something like this? Just and idea, probably buggy (off-by-one etc.).
> >>Should keep away cost from <pageblock_order iterations at the expense of the
> >>relatively fewer >pageblock_order iterations.
> >
> >Hmm... I tested this and found that it's code size is a little bit
> >larger than mine. I'm not sure why this happens exactly but I guess it would be
> >related to compiler optimization. In this case, I'm in favor of my
> >implementation because it looks like well abstraction. It adds one
> >unlikely branch to the merge loop but compiler would optimize it to
> >check it once.
>
> I would be surprised if compiler optimized that to check it once, as
> order increases with each loop iteration. But maybe it's smart
> enough to do something like I did by hand? Guess I'll check the
> disassembly.
Okay. I used following slightly optimized version and I need to
add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)'
to yours. Please consider it, too.
Thanks.
------------------------>8------------------------
From 36b8ffdaa0e7a8d33fd47a62a35a9e507e3e62e9 Mon Sep 17 00:00:00 2001
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Date: Mon, 14 Mar 2016 15:20:07 +0900
Subject: [PATCH] mm: fix cma
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
mm/page_alloc.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0bb933a..f7baa4f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -627,8 +627,8 @@ static inline void rmv_page_order(struct page *page)
*
* For recording page's order, we use page_private(page).
*/
-static inline int page_is_buddy(struct page *page, struct page *buddy,
- unsigned int order)
+static inline int page_is_buddy(struct zone *zone, struct page *page,
+ struct page *buddy, unsigned int order, int mt)
{
if (!pfn_valid_within(page_to_pfn(buddy)))
return 0;
@@ -651,6 +651,15 @@ static inline int page_is_buddy(struct page *page, struct page *buddy,
if (page_zone_id(page) != page_zone_id(buddy))
return 0;
+ if (unlikely(has_isolate_pageblock(zone) &&
+ order >= pageblock_order)) {
+ int buddy_mt = get_pageblock_migratetype(buddy);
+
+ if (mt != buddy_mt && (is_migrate_isolate(mt) ||
+ is_migrate_isolate(buddy_mt)))
+ return 0;
+ }
+
VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy);
return 1;
@@ -698,17 +707,8 @@ static inline void __free_one_page(struct page *page,
VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
VM_BUG_ON(migratetype == -1);
- if (is_migrate_isolate(migratetype)) {
- /*
- * We restrict max order of merging to prevent merge
- * between freepages on isolate pageblock and normal
- * pageblock. Without this, pageblock isolation
- * could cause incorrect freepage accounting.
- */
- max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
- } else {
+ if (!is_migrate_isolate(migratetype))
__mod_zone_freepage_state(zone, 1 << order, migratetype);
- }
page_idx = pfn & ((1 << max_order) - 1);
@@ -718,7 +718,7 @@ static inline void __free_one_page(struct page *page,
while (order < max_order - 1) {
buddy_idx = __find_buddy_index(page_idx, order);
buddy = page + (buddy_idx - page_idx);
- if (!page_is_buddy(page, buddy, order))
+ if (!page_is_buddy(zone, page, buddy, order, migratetype))
break;
/*
* Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page,
@@ -752,7 +752,8 @@ static inline void __free_one_page(struct page *page,
higher_page = page + (combined_idx - page_idx);
buddy_idx = __find_buddy_index(combined_idx, order + 1);
higher_buddy = higher_page + (buddy_idx - combined_idx);
- if (page_is_buddy(higher_page, higher_buddy, order + 1)) {
+ if (page_is_buddy(zone, higher_page, higher_buddy,
+ order + 1, migratetype)) {
list_add_tail(&page->lru,
&zone->free_area[order].free_list[migratetype]);
goto out;
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-03-14 13:40 +0100 |
| Message-ID | <rczWW-3JY-19@gated-at.bofh.it> |
| In reply to | #1356982 |
On 03/14/2016 08:18 AM, Joonsoo Kim wrote: > On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote: >> On 03/14/2016 07:49 AM, Joonsoo Kim wrote: >>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote: >>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote: >>>> >>>> How about something like this? Just and idea, probably buggy (off-by-one etc.). >>>> Should keep away cost from <pageblock_order iterations at the expense of the >>>> relatively fewer >pageblock_order iterations. >>> >>> Hmm... I tested this and found that it's code size is a little bit >>> larger than mine. I'm not sure why this happens exactly but I guess it would be >>> related to compiler optimization. In this case, I'm in favor of my >>> implementation because it looks like well abstraction. It adds one >>> unlikely branch to the merge loop but compiler would optimize it to >>> check it once. >> >> I would be surprised if compiler optimized that to check it once, as >> order increases with each loop iteration. But maybe it's smart >> enough to do something like I did by hand? Guess I'll check the >> disassembly. > > Okay. I used following slightly optimized version and I need to > add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)' > to yours. Please consider it, too. Hmm, so this is bloat-o-meter on x86_64, gcc 5.3.1. CONFIG_CMA=y next-20160310 vs my patch (with added min_t as you pointed out): add/remove: 0/0 grow/shrink: 1/1 up/down: 69/-5 (64) function old new delta free_one_page 833 902 +69 free_pcppages_bulk 1333 1328 -5 next-20160310 vs your patch: add/remove: 0/0 grow/shrink: 2/0 up/down: 577/0 (577) function old new delta free_one_page 833 1187 +354 free_pcppages_bulk 1333 1556 +223 my patch vs your patch: add/remove: 0/0 grow/shrink: 2/0 up/down: 513/0 (513) function old new delta free_one_page 902 1187 +285 free_pcppages_bulk 1328 1556 +228 The increase of your version is surprising, wonder what the compiler did. Otherwise I would like simpler/maintainable version, but this is crazy. Can you post your results? I wonder if your compiler e.g. decided to stop inlining page_is_buddy() or something.
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <js1304@gmail.com> |
|---|---|
| Date | 2016-03-14 15:20 +0100 |
| Message-ID | <rcBvI-4Pn-9@gated-at.bofh.it> |
| In reply to | #1357214 |
2016-03-14 21:30 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>: > On 03/14/2016 08:18 AM, Joonsoo Kim wrote: >> >> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote: >>> >>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote: >>>> >>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote: >>>>> >>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote: >>>>> >>>>> How about something like this? Just and idea, probably buggy >>>>> (off-by-one etc.). >>>>> Should keep away cost from <pageblock_order iterations at the expense >>>>> of the >>>>> relatively fewer >pageblock_order iterations. >>>> >>>> >>>> Hmm... I tested this and found that it's code size is a little bit >>>> larger than mine. I'm not sure why this happens exactly but I guess it >>>> would be >>>> related to compiler optimization. In this case, I'm in favor of my >>>> implementation because it looks like well abstraction. It adds one >>>> unlikely branch to the merge loop but compiler would optimize it to >>>> check it once. >>> >>> >>> I would be surprised if compiler optimized that to check it once, as >>> order increases with each loop iteration. But maybe it's smart >>> enough to do something like I did by hand? Guess I'll check the >>> disassembly. >> >> >> Okay. I used following slightly optimized version and I need to >> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)' >> to yours. Please consider it, too. > > > Hmm, so this is bloat-o-meter on x86_64, gcc 5.3.1. CONFIG_CMA=y > > next-20160310 vs my patch (with added min_t as you pointed out): > add/remove: 0/0 grow/shrink: 1/1 up/down: 69/-5 (64) > function old new delta > free_one_page 833 902 +69 > free_pcppages_bulk 1333 1328 -5 > > next-20160310 vs your patch: > add/remove: 0/0 grow/shrink: 2/0 up/down: 577/0 (577) > function old new delta > free_one_page 833 1187 +354 > free_pcppages_bulk 1333 1556 +223 > > my patch vs your patch: > add/remove: 0/0 grow/shrink: 2/0 up/down: 513/0 (513) > function old new delta > free_one_page 902 1187 +285 > free_pcppages_bulk 1328 1556 +228 > > The increase of your version is surprising, wonder what the compiler did. > Otherwise I would like simpler/maintainable version, but this is crazy. > Can you post your results? I wonder if your compiler e.g. decided to stop > inlining page_is_buddy() or something. Now I see why this happen. I enabled CONFIG_DEBUG_PAGEALLOC and it makes difference. I tested on x86_64, gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4. With CONFIG_CMA + CONFIG_DEBUG_PAGEALLOC ./scripts/bloat-o-meter page_alloc_base.o page_alloc_vlastimil_orig.o add/remove: 0/0 grow/shrink: 2/0 up/down: 510/0 (510) function old new delta free_one_page 1050 1334 +284 free_pcppages_bulk 1396 1622 +226 ./scripts/bloat-o-meter page_alloc_base.o page_alloc_mine.o add/remove: 0/0 grow/shrink: 2/0 up/down: 351/0 (351) function old new delta free_one_page 1050 1230 +180 free_pcppages_bulk 1396 1567 +171 With CONFIG_CMA + !CONFIG_DEBUG_PAGEALLOC (pa_b is base, pa_v is yours and pa_m is mine) ./scripts/bloat-o-meter pa_b.o pa_v.o add/remove: 0/0 grow/shrink: 1/1 up/down: 88/-23 (65) function old new delta free_one_page 761 849 +88 free_pcppages_bulk 1117 1094 -23 ./scripts/bloat-o-meter pa_b.o pa_m.o add/remove: 0/0 grow/shrink: 2/0 up/down: 329/0 (329) function old new delta free_one_page 761 1031 +270 free_pcppages_bulk 1117 1176 +59 Still, it has difference but less than before. Maybe, we are still using different configuration. Could you check if CONFIG_DEBUG_VM is enabled or not? In my case, it's not enabled. And, do you think this bloat isn't acceptable? Thanks.
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-03-16 13:10 +0100 |
| Message-ID | <rdir1-kr-23@gated-at.bofh.it> |
| In reply to | #1357260 |
On 03/14/2016 03:10 PM, Joonsoo Kim wrote: > 2016-03-14 21:30 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>: > > Now I see why this happen. I enabled CONFIG_DEBUG_PAGEALLOC > and it makes difference. > > I tested on x86_64, gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4. > > With CONFIG_CMA + CONFIG_DEBUG_PAGEALLOC > ./scripts/bloat-o-meter page_alloc_base.o page_alloc_vlastimil_orig.o > add/remove: 0/0 grow/shrink: 2/0 up/down: 510/0 (510) > function old new delta > free_one_page 1050 1334 +284 > free_pcppages_bulk 1396 1622 +226 > > ./scripts/bloat-o-meter page_alloc_base.o page_alloc_mine.o > add/remove: 0/0 grow/shrink: 2/0 up/down: 351/0 (351) > function old new delta > free_one_page 1050 1230 +180 > free_pcppages_bulk 1396 1567 +171 > > > With CONFIG_CMA + !CONFIG_DEBUG_PAGEALLOC > (pa_b is base, pa_v is yours and pa_m is mine) > > ./scripts/bloat-o-meter pa_b.o pa_v.o > add/remove: 0/0 grow/shrink: 1/1 up/down: 88/-23 (65) > function old new delta > free_one_page 761 849 +88 > free_pcppages_bulk 1117 1094 -23 > > ./scripts/bloat-o-meter pa_b.o pa_m.o > add/remove: 0/0 grow/shrink: 2/0 up/down: 329/0 (329) > function old new delta > free_one_page 761 1031 +270 > free_pcppages_bulk 1117 1176 +59 > > Still, it has difference but less than before. > Maybe, we are still using different configuration. Could you > check if CONFIG_DEBUG_VM is enabled or not? In my case, it's not It's disabled here. > enabled. And, do you think this bloat isn't acceptable? Well, it is quite significant. But given that Hanjun sees the errors still, it's not the biggest issue now :/ > Thanks. >
[toc] | [prev] | [next] | [standalone]
| From | Hanjun Guo <guohanjun@huawei.com> |
|---|---|
| Date | 2016-03-16 10:50 +0100 |
| Message-ID | <rdgfv-7bg-1@gated-at.bofh.it> |
| In reply to | #1356982 |
On 2016/3/14 15:18, Joonsoo Kim wrote: > On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote: >> On 03/14/2016 07:49 AM, Joonsoo Kim wrote: >>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote: >>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote: >>>> >>>> How about something like this? Just and idea, probably buggy (off-by-one etc.). >>>> Should keep away cost from <pageblock_order iterations at the expense of the >>>> relatively fewer >pageblock_order iterations. >>> Hmm... I tested this and found that it's code size is a little bit >>> larger than mine. I'm not sure why this happens exactly but I guess it would be >>> related to compiler optimization. In this case, I'm in favor of my >>> implementation because it looks like well abstraction. It adds one >>> unlikely branch to the merge loop but compiler would optimize it to >>> check it once. >> I would be surprised if compiler optimized that to check it once, as >> order increases with each loop iteration. But maybe it's smart >> enough to do something like I did by hand? Guess I'll check the >> disassembly. > Okay. I used following slightly optimized version and I need to > add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)' > to yours. Please consider it, too. Hmm, this one is not work, I still can see the bug is there after applying this patch, did I miss something? Thanks Hanjun
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| Date | 2016-03-17 08:00 +0100 |
| Message-ID | <rdA4y-3K4-3@gated-at.bofh.it> |
| In reply to | #1358772 |
On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote: > On 2016/3/14 15:18, Joonsoo Kim wrote: > > On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote: > >> On 03/14/2016 07:49 AM, Joonsoo Kim wrote: > >>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote: > >>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote: > >>>> > >>>> How about something like this? Just and idea, probably buggy (off-by-one etc.). > >>>> Should keep away cost from <pageblock_order iterations at the expense of the > >>>> relatively fewer >pageblock_order iterations. > >>> Hmm... I tested this and found that it's code size is a little bit > >>> larger than mine. I'm not sure why this happens exactly but I guess it would be > >>> related to compiler optimization. In this case, I'm in favor of my > >>> implementation because it looks like well abstraction. It adds one > >>> unlikely branch to the merge loop but compiler would optimize it to > >>> check it once. > >> I would be surprised if compiler optimized that to check it once, as > >> order increases with each loop iteration. But maybe it's smart > >> enough to do something like I did by hand? Guess I'll check the > >> disassembly. > > Okay. I used following slightly optimized version and I need to > > add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)' > > to yours. Please consider it, too. > > Hmm, this one is not work, I still can see the bug is there after applying > this patch, did I miss something? I may find that there is a bug which was introduced by me some time ago. Could you test following change in __free_one_page() on top of Vlastimil's patch? -page_idx = pfn & ((1 << max_order) - 1); +page_idx = pfn & ((1 << MAX_ORDER) - 1); Thanks.
[toc] | [prev] | [next] | [standalone]
| From | Hanjun Guo <guohanjun@huawei.com> |
|---|---|
| Date | 2016-03-17 10:30 +0100 |
| Message-ID | <rdCpJ-5uU-21@gated-at.bofh.it> |
| In reply to | #1359600 |
On 2016/3/17 14:54, Joonsoo Kim wrote:
> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote:
>> On 2016/3/14 15:18, Joonsoo Kim wrote:
>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote:
>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
>>>>>>
>>>>>> How about something like this? Just and idea, probably buggy (off-by-one etc.).
>>>>>> Should keep away cost from <pageblock_order iterations at the expense of the
>>>>>> relatively fewer >pageblock_order iterations.
>>>>> Hmm... I tested this and found that it's code size is a little bit
>>>>> larger than mine. I'm not sure why this happens exactly but I guess it would be
>>>>> related to compiler optimization. In this case, I'm in favor of my
>>>>> implementation because it looks like well abstraction. It adds one
>>>>> unlikely branch to the merge loop but compiler would optimize it to
>>>>> check it once.
>>>> I would be surprised if compiler optimized that to check it once, as
>>>> order increases with each loop iteration. But maybe it's smart
>>>> enough to do something like I did by hand? Guess I'll check the
>>>> disassembly.
>>> Okay. I used following slightly optimized version and I need to
>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)'
>>> to yours. Please consider it, too.
>> Hmm, this one is not work, I still can see the bug is there after applying
>> this patch, did I miss something?
> I may find that there is a bug which was introduced by me some time
> ago. Could you test following change in __free_one_page() on top of
> Vlastimil's patch?
>
> -page_idx = pfn & ((1 << max_order) - 1);
> +page_idx = pfn & ((1 << MAX_ORDER) - 1);
I tested Vlastimil's patch + your change with stress for more than half hour, the bug
I reported is gone :)
I have some questions, Joonsoo, you provided a patch as following:
diff --git a/mm/cma.c b/mm/cma.c
index 3a7a67b..952a8a3 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -448,7 +448,10 @@ bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
+ mutex_lock(&cma_mutex);
free_contig_range(pfn, count);
+ mutex_unlock(&cma_mutex);
+
cma_clear_bitmap(cma, pfn, count);
trace_cma_release(pfn, pages, count);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 7f32950..68ed5ae 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1559,7 +1559,8 @@ void free_hot_cold_page(struct page *page, bool cold)
* excessively into the page allocator
*/
if (migratetype >= MIGRATE_PCPTYPES) {
- if (unlikely(is_migrate_isolate(migratetype))) {
+ if (is_migrate_cma(migratetype) ||
+ unlikely(is_migrate_isolate(migratetype))) {
free_one_page(zone, page, pfn, 0, migratetype);
goto out;
}
This patch also works to fix the bug, why not just use this one? is there
any side effects for this patch? maybe there is performance issue as the
mutex lock is used, any other issues?
Thanks
Hanjun
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <js1304@gmail.com> |
|---|---|
| Date | 2016-03-17 16:40 +0100 |
| Message-ID | <rdIbM-Jr-11@gated-at.bofh.it> |
| In reply to | #1359655 |
2016-03-17 18:24 GMT+09:00 Hanjun Guo <guohanjun@huawei.com>:
> On 2016/3/17 14:54, Joonsoo Kim wrote:
>> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote:
>>> On 2016/3/14 15:18, Joonsoo Kim wrote:
>>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote:
>>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
>>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
>>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
>>>>>>>
>>>>>>> How about something like this? Just and idea, probably buggy (off-by-one etc.).
>>>>>>> Should keep away cost from <pageblock_order iterations at the expense of the
>>>>>>> relatively fewer >pageblock_order iterations.
>>>>>> Hmm... I tested this and found that it's code size is a little bit
>>>>>> larger than mine. I'm not sure why this happens exactly but I guess it would be
>>>>>> related to compiler optimization. In this case, I'm in favor of my
>>>>>> implementation because it looks like well abstraction. It adds one
>>>>>> unlikely branch to the merge loop but compiler would optimize it to
>>>>>> check it once.
>>>>> I would be surprised if compiler optimized that to check it once, as
>>>>> order increases with each loop iteration. But maybe it's smart
>>>>> enough to do something like I did by hand? Guess I'll check the
>>>>> disassembly.
>>>> Okay. I used following slightly optimized version and I need to
>>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)'
>>>> to yours. Please consider it, too.
>>> Hmm, this one is not work, I still can see the bug is there after applying
>>> this patch, did I miss something?
>> I may find that there is a bug which was introduced by me some time
>> ago. Could you test following change in __free_one_page() on top of
>> Vlastimil's patch?
>>
>> -page_idx = pfn & ((1 << max_order) - 1);
>> +page_idx = pfn & ((1 << MAX_ORDER) - 1);
>
> I tested Vlastimil's patch + your change with stress for more than half hour, the bug
> I reported is gone :)
Good to hear!
> I have some questions, Joonsoo, you provided a patch as following:
>
> diff --git a/mm/cma.c b/mm/cma.c
> index 3a7a67b..952a8a3 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -448,7 +448,10 @@ bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
>
> VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
>
> + mutex_lock(&cma_mutex);
> free_contig_range(pfn, count);
> + mutex_unlock(&cma_mutex);
> +
> cma_clear_bitmap(cma, pfn, count);
> trace_cma_release(pfn, pages, count);
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 7f32950..68ed5ae 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1559,7 +1559,8 @@ void free_hot_cold_page(struct page *page, bool cold)
> * excessively into the page allocator
> */
> if (migratetype >= MIGRATE_PCPTYPES) {
> - if (unlikely(is_migrate_isolate(migratetype))) {
> + if (is_migrate_cma(migratetype) ||
> + unlikely(is_migrate_isolate(migratetype))) {
> free_one_page(zone, page, pfn, 0, migratetype);
> goto out;
> }
>
> This patch also works to fix the bug, why not just use this one? is there
> any side effects for this patch? maybe there is performance issue as the
> mutex lock is used, any other issues?
The changes in free_hot_cold_page() would cause unacceptable performance
problem in a big machine, because, with above change, it takes zone->lock
whenever freeing one page on CMA region.
Thanks.
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-03-17 16:50 +0100 |
| Message-ID | <rdIls-MU-23@gated-at.bofh.it> |
| In reply to | #1359655 |
On 03/17/2016 10:24 AM, Hanjun Guo wrote: > On 2016/3/17 14:54, Joonsoo Kim wrote: >> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote: >>> On 2016/3/14 15:18, Joonsoo Kim wrote: >>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote: >>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote: >>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote: >>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote: >>>>>>> >>>>>>> How about something like this? Just and idea, probably buggy (off-by-one etc.). >>>>>>> Should keep away cost from <pageblock_order iterations at the expense of the >>>>>>> relatively fewer >pageblock_order iterations. >>>>>> Hmm... I tested this and found that it's code size is a little bit >>>>>> larger than mine. I'm not sure why this happens exactly but I guess it would be >>>>>> related to compiler optimization. In this case, I'm in favor of my >>>>>> implementation because it looks like well abstraction. It adds one >>>>>> unlikely branch to the merge loop but compiler would optimize it to >>>>>> check it once. >>>>> I would be surprised if compiler optimized that to check it once, as >>>>> order increases with each loop iteration. But maybe it's smart >>>>> enough to do something like I did by hand? Guess I'll check the >>>>> disassembly. >>>> Okay. I used following slightly optimized version and I need to >>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)' >>>> to yours. Please consider it, too. >>> Hmm, this one is not work, I still can see the bug is there after applying >>> this patch, did I miss something? >> I may find that there is a bug which was introduced by me some time >> ago. Could you test following change in __free_one_page() on top of >> Vlastimil's patch? >> >> -page_idx = pfn & ((1 << max_order) - 1); >> +page_idx = pfn & ((1 << MAX_ORDER) - 1); > > I tested Vlastimil's patch + your change with stress for more than half hour, the bug > I reported is gone :) Oh, ok, will try to send proper patch, once I figure out what to write in the changelog :)
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <js1304@gmail.com> |
|---|---|
| Date | 2016-03-17 17:00 +0100 |
| Message-ID | <rdIv7-Qo-3@gated-at.bofh.it> |
| In reply to | #1359970 |
2016-03-18 0:43 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>: > On 03/17/2016 10:24 AM, Hanjun Guo wrote: >> >> On 2016/3/17 14:54, Joonsoo Kim wrote: >>> >>> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote: >>>> >>>> On 2016/3/14 15:18, Joonsoo Kim wrote: >>>>> >>>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote: >>>>>> >>>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote: >>>>>>> >>>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote: >>>>>>>> >>>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote: >>>>>>>> >>>>>>>> How about something like this? Just and idea, probably buggy >>>>>>>> (off-by-one etc.). >>>>>>>> Should keep away cost from <pageblock_order iterations at the >>>>>>>> expense of the >>>>>>>> relatively fewer >pageblock_order iterations. >>>>>>> >>>>>>> Hmm... I tested this and found that it's code size is a little bit >>>>>>> larger than mine. I'm not sure why this happens exactly but I guess >>>>>>> it would be >>>>>>> related to compiler optimization. In this case, I'm in favor of my >>>>>>> implementation because it looks like well abstraction. It adds one >>>>>>> unlikely branch to the merge loop but compiler would optimize it to >>>>>>> check it once. >>>>>> >>>>>> I would be surprised if compiler optimized that to check it once, as >>>>>> order increases with each loop iteration. But maybe it's smart >>>>>> enough to do something like I did by hand? Guess I'll check the >>>>>> disassembly. >>>>> >>>>> Okay. I used following slightly optimized version and I need to >>>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)' >>>>> to yours. Please consider it, too. >>>> >>>> Hmm, this one is not work, I still can see the bug is there after >>>> applying >>>> this patch, did I miss something? >>> >>> I may find that there is a bug which was introduced by me some time >>> ago. Could you test following change in __free_one_page() on top of >>> Vlastimil's patch? >>> >>> -page_idx = pfn & ((1 << max_order) - 1); >>> +page_idx = pfn & ((1 << MAX_ORDER) - 1); >> >> >> I tested Vlastimil's patch + your change with stress for more than half >> hour, the bug >> I reported is gone :) > > > Oh, ok, will try to send proper patch, once I figure out what to write in > the changelog :) Thanks in advance! Thanks.
[toc] | [prev] | [next] | [standalone]
| From | Hanjun Guo <hanjun.guo@linaro.org> |
|---|---|
| Date | 2016-03-08 05:10 +0100 |
| Message-ID | <rah86-493-9@gated-at.bofh.it> |
| In reply to | #1351366 |
On 03/07/2016 04:16 PM, Leizhen (ThunderTown) wrote: > > > On 2016/3/7 12:34, Joonsoo Kim wrote: >> On Fri, Mar 04, 2016 at 03:35:26PM +0800, Hanjun Guo wrote: >>> On 2016/3/4 14:38, Joonsoo Kim wrote: >>>> On Fri, Mar 04, 2016 at 02:05:09PM +0800, Hanjun Guo wrote: >>>>> On 2016/3/4 12:32, Joonsoo Kim wrote: >>>>>> On Fri, Mar 04, 2016 at 11:02:33AM +0900, Joonsoo Kim wrote: >>>>>>> On Thu, Mar 03, 2016 at 08:49:01PM +0800, Hanjun Guo wrote: >>>>>>>> On 2016/3/3 15:42, Joonsoo Kim wrote: >>>>>>>>> 2016-03-03 10:25 GMT+09:00 Laura Abbott <labbott@redhat.com>: >>>>>>>>>> (cc -mm and Joonsoo Kim) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 03/02/2016 05:52 AM, Hanjun Guo wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> I came across a suspicious error for CMA stress test: >>>>>>>>>>> >>>>>>>>>>> Before the test, I got: >>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma >>>>>>>>>>> CmaTotal: 204800 kB >>>>>>>>>>> CmaFree: 195044 kB >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> After running the test: >>>>>>>>>>> -bash-4.3# cat /proc/meminfo | grep Cma >>>>>>>>>>> CmaTotal: 204800 kB >>>>>>>>>>> CmaFree: 6602584 kB >>>>>>>>>>> >>>>>>>>>>> So the freed CMA memory is more than total.. >>>>>>>>>>> >>>>>>>>>>> Also the the MemFree is more than mem total: >>>>>>>>>>> >>>>>>>>>>> -bash-4.3# cat /proc/meminfo >>>>>>>>>>> MemTotal: 16342016 kB >>>>>>>>>>> MemFree: 22367268 kB >>>>>>>>>>> MemAvailable: 22370528 kB >>>>>>>> [...] >>>>>>>>>> I played with this a bit and can see the same problem. The sanity >>>>>>>>>> check of CmaFree < CmaTotal generally triggers in >>>>>>>>>> __move_zone_freepage_state in unset_migratetype_isolate. >>>>>>>>>> This also seems to be present as far back as v4.0 which was the >>>>>>>>>> first version to have the updated accounting from Joonsoo. >>>>>>>>>> Were there known limitations with the new freepage accounting, >>>>>>>>>> Joonsoo? >>>>>>>>> I don't know. I also played with this and looks like there is >>>>>>>>> accounting problem, however, for my case, number of free page is slightly less >>>>>>>>> than total. I will take a look. >>>>>>>>> >>>>>>>>> Hanjun, could you tell me your malloc_size? I tested with 1 and it doesn't >>>>>>>>> look like your case. >>>>>>>> I tested with malloc_size with 2M, and it grows much bigger than 1M, also I >>>>>>>> did some other test: >>>>>>> Thanks! Now, I can re-generate erronous situation you mentioned. >>>>>>> >>>>>>>> - run with single thread with 100000 times, everything is fine. >>>>>>>> >>>>>>>> - I hack the cam_alloc() and free as below [1] to see if it's lock issue, with >>>>>>>> the same test with 100 multi-thread, then I got: >>>>>>> [1] would not be sufficient to close this race. >>>>>>> >>>>>>> Try following things [A]. And, for more accurate test, I changed code a bit more >>>>>>> to prevent kernel page allocation from cma area [B]. This will prevent kernel >>>>>>> page allocation from cma area completely so we can focus cma_alloc/release race. >>>>>>> >>>>>>> Although, this is not correct fix, it could help that we can guess >>>>>>> where the problem is. >>>>>> More correct fix is something like below. >>>>>> Please test it. >>>>> Hmm, this is not working: >>>> Sad to hear that. >>>> >>>> Could you tell me your system's MAX_ORDER and pageblock_order? >>>> >>> >>> MAX_ORDER is 11, pageblock_order is 9, thanks for your help! >> >> Hmm... that's same with me. >> >> Below is similar fix that prevents buddy merging when one of buddy's >> migrate type, but, not both, is MIGRATE_ISOLATE. In fact, I have >> no idea why previous fix (more correct fix) doesn't work for you. >> (It works for me.) But, maybe there is a bug on the fix >> so I make new one which is more general form. Please test it. > > Hi, > Hanjun Guo has gone to Tailand on business, so I help him to run this patch. The result > shows that the count of "CmaFree:" is OK now. Thanks Leizhen :) > But sometimes printed some information as below: > > alloc_contig_range: [28500, 28600) PFNs busy > alloc_contig_range: [28300, 28380) PFNs busy I think it's not a problem for the stress test, as it's the lock not released yet. Thanks Hanjun
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web