Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1402136 > unrolled thread
| Started by | Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> |
|---|---|
| First post | 2016-05-17 09:00 +0200 |
| Last post | 2016-05-18 10:50 +0200 |
| Articles | 4 — 3 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: [PATCH 28/28] mm, page_alloc: Defer debugging checks of pages allocated from the PCP Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2016-05-17 09:00 +0200
Re: [PATCH 28/28] mm, page_alloc: Defer debugging checks of pages allocated from the PCP Vlastimil Babka <vbabka@suse.cz> - 2016-05-18 10:00 +0200
Re: [PATCH 28/28] mm, page_alloc: Defer debugging checks of pages allocated from the PCP Vlastimil Babka <vbabka@suse.cz> - 2016-05-18 10:00 +0200
Re: [PATCH 28/28] mm, page_alloc: Defer debugging checks of pages allocated from the PCP Mel Gorman <mgorman@techsingularity.net> - 2016-05-18 10:50 +0200
| From | Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> |
|---|---|
| Date | 2016-05-17 09:00 +0200 |
| Subject | Re: [PATCH 28/28] mm, page_alloc: Defer debugging checks of pages allocated from the PCP |
| Message-ID | <rzH90-16V-13@gated-at.bofh.it> |
> @@ -2579,20 +2612,22 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
> struct list_head *list;
>
> local_irq_save(flags);
> - pcp = &this_cpu_ptr(zone->pageset)->pcp;
> - list = &pcp->lists[migratetype];
> - if (list_empty(list)) {
> - pcp->count += rmqueue_bulk(zone, 0,
> - pcp->batch, list,
> - migratetype, cold);
> - if (unlikely(list_empty(list)))
> - goto failed;
> - }
> + do {
> + pcp = &this_cpu_ptr(zone->pageset)->pcp;
> + list = &pcp->lists[migratetype];
> + if (list_empty(list)) {
> + pcp->count += rmqueue_bulk(zone, 0,
> + pcp->batch, list,
> + migratetype, cold);
> + if (unlikely(list_empty(list)))
> + goto failed;
> + }
>
> - if (cold)
> - page = list_last_entry(list, struct page, lru);
> - else
> - page = list_first_entry(list, struct page, lru);
> + if (cold)
> + page = list_last_entry(list, struct page, lru);
> + else
> + page = list_first_entry(list, struct page, lru);
> + } while (page && check_new_pcp(page));
This causes infinite loop when check_new_pcp() returns 1, because the bad
page is still in the list (I assume that a bad page never disappears).
The original kernel is free from this problem because we do retry after
list_del(). So moving the following 3 lines into this do-while block solves
the problem?
__dec_zone_state(zone, NR_ALLOC_BATCH);
list_del(&page->lru);
pcp->count--;
There seems no infinit loop issue in order > 0 block below, because bad pages
are deleted from free list in __rmqueue_smallest().
Thanks,
Naoya Horiguchi
>
> __dec_zone_state(zone, NR_ALLOC_BATCH);
> list_del(&page->lru);
> @@ -2605,14 +2640,16 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
> WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> spin_lock_irqsave(&zone->lock, flags);
>
> - page = NULL;
> - if (alloc_flags & ALLOC_HARDER) {
> - page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
> - if (page)
> - trace_mm_page_alloc_zone_locked(page, order, migratetype);
> - }
> - if (!page)
> - page = __rmqueue(zone, order, migratetype);
> + do {
> + page = NULL;
> + if (alloc_flags & ALLOC_HARDER) {
> + page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
> + if (page)
> + trace_mm_page_alloc_zone_locked(page, order, migratetype);
> + }
> + if (!page)
> + page = __rmqueue(zone, order, migratetype);
> + } while (page && check_new_pages(page, order));
> spin_unlock(&zone->lock);
> if (!page)
> goto failed;
> @@ -2979,8 +3016,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
> page = buffered_rmqueue(ac->preferred_zoneref->zone, zone, order,
> gfp_mask, alloc_flags, ac->migratetype);
> if (page) {
> - if (prep_new_page(page, order, gfp_mask, alloc_flags))
> - goto try_this_zone;
> + prep_new_page(page, order, gfp_mask, alloc_flags);
>
> /*
> * If this is a high-order atomic allocation then check
> --
> 2.6.4
>
> --
> 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] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-05-18 10:00 +0200 |
| Message-ID | <rA4yB-7yy-7@gated-at.bofh.it> |
| In reply to | #1402136 |
On 05/18/2016 09:51 AM, Vlastimil Babka wrote:
> ----8<----
> From f52f5e2a7dd65f2814183d8fd254ace43120b828 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Wed, 18 May 2016 09:41:01 +0200
> Subject: [PATCH] mm, page_alloc: prevent infinite loop in buffered_rmqueue()
>
> In DEBUG_VM kernel, we can hit infinite loop for order == 0 in
> buffered_rmqueue() when check_new_pcp() returns 1, because the bad page is
> never removed from the pcp list. Fix this by removing the page before retrying.
> Also we don't need to check if page is non-NULL, because we simply grab it from
> the list which was just tested for being non-empty.
>
> Fixes: http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-page_alloc-defer-debugging-checks-of-freed-pages-until-a-pcp-drain.patch
Wrong.
Fixes: http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-page_alloc-defer-debugging-checks-of-pages-allocated-from-the-pcp.patch
> Reported-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> mm/page_alloc.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 8c81e2e7b172..d5b93e5dd697 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2641,11 +2641,12 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
> page = list_last_entry(list, struct page, lru);
> else
> page = list_first_entry(list, struct page, lru);
> - } while (page && check_new_pcp(page));
>
> - __dec_zone_state(zone, NR_ALLOC_BATCH);
> - list_del(&page->lru);
> - pcp->count--;
> + __dec_zone_state(zone, NR_ALLOC_BATCH);
> + list_del(&page->lru);
> + pcp->count--;
> +
> + } while (check_new_pcp(page));
> } else {
> /*
> * We most definitely don't want callers attempting to
>
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-05-18 10:00 +0200 |
| Message-ID | <rA4yB-7yy-9@gated-at.bofh.it> |
| In reply to | #1402136 |
On 05/17/2016 08:41 AM, Naoya Horiguchi wrote:
>> @@ -2579,20 +2612,22 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
>> struct list_head *list;
>>
>> local_irq_save(flags);
>> - pcp = &this_cpu_ptr(zone->pageset)->pcp;
>> - list = &pcp->lists[migratetype];
>> - if (list_empty(list)) {
>> - pcp->count += rmqueue_bulk(zone, 0,
>> - pcp->batch, list,
>> - migratetype, cold);
>> - if (unlikely(list_empty(list)))
>> - goto failed;
>> - }
>> + do {
>> + pcp = &this_cpu_ptr(zone->pageset)->pcp;
>> + list = &pcp->lists[migratetype];
>> + if (list_empty(list)) {
>> + pcp->count += rmqueue_bulk(zone, 0,
>> + pcp->batch, list,
>> + migratetype, cold);
>> + if (unlikely(list_empty(list)))
>> + goto failed;
>> + }
>>
>> - if (cold)
>> - page = list_last_entry(list, struct page, lru);
>> - else
>> - page = list_first_entry(list, struct page, lru);
>> + if (cold)
>> + page = list_last_entry(list, struct page, lru);
>> + else
>> + page = list_first_entry(list, struct page, lru);
>> + } while (page && check_new_pcp(page));
>
> This causes infinite loop when check_new_pcp() returns 1, because the bad
> page is still in the list (I assume that a bad page never disappears).
> The original kernel is free from this problem because we do retry after
> list_del(). So moving the following 3 lines into this do-while block solves
> the problem?
>
> __dec_zone_state(zone, NR_ALLOC_BATCH);
> list_del(&page->lru);
> pcp->count--;
>
> There seems no infinit loop issue in order > 0 block below, because bad pages
> are deleted from free list in __rmqueue_smallest().
Ooops, thanks for catching this, wish it was sooner...
----8<----
From f52f5e2a7dd65f2814183d8fd254ace43120b828 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Wed, 18 May 2016 09:41:01 +0200
Subject: [PATCH] mm, page_alloc: prevent infinite loop in buffered_rmqueue()
In DEBUG_VM kernel, we can hit infinite loop for order == 0 in
buffered_rmqueue() when check_new_pcp() returns 1, because the bad page is
never removed from the pcp list. Fix this by removing the page before retrying.
Also we don't need to check if page is non-NULL, because we simply grab it from
the list which was just tested for being non-empty.
Fixes: http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-page_alloc-defer-debugging-checks-of-freed-pages-until-a-pcp-drain.patch
Reported-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/page_alloc.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 8c81e2e7b172..d5b93e5dd697 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2641,11 +2641,12 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
page = list_last_entry(list, struct page, lru);
else
page = list_first_entry(list, struct page, lru);
- } while (page && check_new_pcp(page));
- __dec_zone_state(zone, NR_ALLOC_BATCH);
- list_del(&page->lru);
- pcp->count--;
+ __dec_zone_state(zone, NR_ALLOC_BATCH);
+ list_del(&page->lru);
+ pcp->count--;
+
+ } while (check_new_pcp(page));
} else {
/*
* We most definitely don't want callers attempting to
--
2.8.2
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2016-05-18 10:50 +0200 |
| Message-ID | <rA5l0-87e-7@gated-at.bofh.it> |
| In reply to | #1402780 |
On Wed, May 18, 2016 at 09:51:58AM +0200, Vlastimil Babka wrote:
> On 05/17/2016 08:41 AM, Naoya Horiguchi wrote:
> >> @@ -2579,20 +2612,22 @@ struct page *buffered_rmqueue(struct zone *preferred_zone,
> >> struct list_head *list;
> >>
> >> local_irq_save(flags);
> >> - pcp = &this_cpu_ptr(zone->pageset)->pcp;
> >> - list = &pcp->lists[migratetype];
> >> - if (list_empty(list)) {
> >> - pcp->count += rmqueue_bulk(zone, 0,
> >> - pcp->batch, list,
> >> - migratetype, cold);
> >> - if (unlikely(list_empty(list)))
> >> - goto failed;
> >> - }
> >> + do {
> >> + pcp = &this_cpu_ptr(zone->pageset)->pcp;
> >> + list = &pcp->lists[migratetype];
> >> + if (list_empty(list)) {
> >> + pcp->count += rmqueue_bulk(zone, 0,
> >> + pcp->batch, list,
> >> + migratetype, cold);
> >> + if (unlikely(list_empty(list)))
> >> + goto failed;
> >> + }
> >>
> >> - if (cold)
> >> - page = list_last_entry(list, struct page, lru);
> >> - else
> >> - page = list_first_entry(list, struct page, lru);
> >> + if (cold)
> >> + page = list_last_entry(list, struct page, lru);
> >> + else
> >> + page = list_first_entry(list, struct page, lru);
> >> + } while (page && check_new_pcp(page));
> >
> > This causes infinite loop when check_new_pcp() returns 1, because the bad
> > page is still in the list (I assume that a bad page never disappears).
> > The original kernel is free from this problem because we do retry after
> > list_del(). So moving the following 3 lines into this do-while block solves
> > the problem?
> >
> > __dec_zone_state(zone, NR_ALLOC_BATCH);
> > list_del(&page->lru);
> > pcp->count--;
> >
> > There seems no infinit loop issue in order > 0 block below, because bad pages
> > are deleted from free list in __rmqueue_smallest().
>
> Ooops, thanks for catching this, wish it was sooner...
>
Still not too late fortunately! Thanks Naoya for identifying this and
Vlastimil for fixing it.
> ----8<----
> From f52f5e2a7dd65f2814183d8fd254ace43120b828 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Wed, 18 May 2016 09:41:01 +0200
> Subject: [PATCH] mm, page_alloc: prevent infinite loop in buffered_rmqueue()
>
> In DEBUG_VM kernel, we can hit infinite loop for order == 0 in
> buffered_rmqueue() when check_new_pcp() returns 1, because the bad page is
> never removed from the pcp list. Fix this by removing the page before retrying.
> Also we don't need to check if page is non-NULL, because we simply grab it from
> the list which was just tested for being non-empty.
>
> Fixes: http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-page_alloc-defer-debugging-checks-of-freed-pages-until-a-pcp-drain.patch
> Reported-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Mel Gorman <mgorman@techsingularity.net>
--
Mel Gorman
SUSE Labs
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web