* Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page fails (fwd)
@ 2017-05-09 15:43 Julia Lawall
2017-05-10 5:55 ` zhong jiang
0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2017-05-09 15:43 UTC (permalink / raw)
To: zhongjiang; +Cc: akpm, vbabka, kirill.shutemov, hannes, mgorman, linux-mm
Hello,
I don't know if there is a bug here, but it could e worth checking on. If
the loop on line 1481 is executed, page will not be NULL at the out label
on line 1560. Instead it will have a dummy value. Perhaps the value of
result keeps the if at the out label from being taken.
julia
---------- Forwarded message ----------
Date: Tue, 9 May 2017 23:27:43 +0800
From: kbuild test robot <fengguang.wu@intel.com>
To: kbuild@01.org
Cc: Julia Lawall <julia.lawall@lip6.fr>
Subject: Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page
fails
Hi zhong,
[auto build test WARNING on mmotm/master]
[also build test WARNING on v4.11 next-20170509]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/zhongjiang/mm-fix-the-memory-leak-after-collapsing-the-huge-page-fails/20170509-193011
base: git://git.cmpxchg.org/linux-mmotm.git master
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
>> mm/khugepaged.c:1560:5-9: ERROR: invalid reference to the index variable of the iterator on line 1481
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout a5318ea654d5b764d6e06c6cfbfc21e44ce56e2b
vim +1560 mm/khugepaged.c
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1475 struct zone *zone = page_zone(new_page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1476
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1477 /*
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1478 * Replacing old pages with new one has succeed, now we need to
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1479 * copy the content and free old pages.
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1480 */
f3f0e1d2 Kirill A. Shutemov 2016-07-26 @1481 list_for_each_entry_safe(page, tmp, &pagelist, lru) {
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1482 copy_highpage(new_page + (page->index % HPAGE_PMD_NR),
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1483 page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1484 list_del(&page->lru);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1485 unlock_page(page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1486 page_ref_unfreeze(page, 1);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1487 page->mapping = NULL;
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1488 ClearPageActive(page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1489 ClearPageUnevictable(page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1490 put_page(page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1491 }
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1492
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1493 local_irq_save(flags);
11fb9989 Mel Gorman 2016-07-28 1494 __inc_node_page_state(new_page, NR_SHMEM_THPS);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1495 if (nr_none) {
11fb9989 Mel Gorman 2016-07-28 1496 __mod_node_page_state(zone->zone_pgdat, NR_FILE_PAGES, nr_none);
11fb9989 Mel Gorman 2016-07-28 1497 __mod_node_page_state(zone->zone_pgdat, NR_SHMEM, nr_none);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1498 }
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1499 local_irq_restore(flags);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1500
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1501 /*
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1502 * Remove pte page tables, so we can re-faulti
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1503 * the page as huge.
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1504 */
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1505 retract_page_tables(mapping, start);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1506
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1507 /* Everything is ready, let's unfreeze the new_page */
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1508 set_page_dirty(new_page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1509 SetPageUptodate(new_page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1510 page_ref_unfreeze(new_page, HPAGE_PMD_NR);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1511 mem_cgroup_commit_charge(new_page, memcg, false, true);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1512 lru_cache_add_anon(new_page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1513 unlock_page(new_page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1514
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1515 *hpage = NULL;
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1516 } else {
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1517 /* Something went wrong: rollback changes to the radix-tree */
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1518 shmem_uncharge(mapping->host, nr_none);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1519 spin_lock_irq(&mapping->tree_lock);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1520 radix_tree_for_each_slot(slot, &mapping->page_tree, &iter,
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1521 start) {
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1522 if (iter.index >= end)
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1523 break;
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1524 page = list_first_entry_or_null(&pagelist,
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1525 struct page, lru);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1526 if (!page || iter.index < page->index) {
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1527 if (!nr_none)
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1528 break;
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1529 nr_none--;
59749e6c Johannes Weiner 2016-12-12 1530 /* Put holes back where they were */
59749e6c Johannes Weiner 2016-12-12 1531 radix_tree_delete(&mapping->page_tree,
59749e6c Johannes Weiner 2016-12-12 1532 iter.index);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1533 continue;
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1534 }
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1535
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1536 VM_BUG_ON_PAGE(page->index != iter.index, page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1537
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1538 /* Unfreeze the page. */
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1539 list_del(&page->lru);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1540 page_ref_unfreeze(page, 2);
6d75f366 Johannes Weiner 2016-12-12 1541 radix_tree_replace_slot(&mapping->page_tree,
6d75f366 Johannes Weiner 2016-12-12 1542 slot, page);
148deab2 Matthew Wilcox 2016-12-14 1543 slot = radix_tree_iter_resume(slot, &iter);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1544 spin_unlock_irq(&mapping->tree_lock);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1545 putback_lru_page(page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1546 unlock_page(page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1547 spin_lock_irq(&mapping->tree_lock);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1548 }
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1549 VM_BUG_ON(nr_none);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1550 spin_unlock_irq(&mapping->tree_lock);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1551
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1552 /* Unfreeze new_page, caller would take care about freeing it */
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1553 page_ref_unfreeze(new_page, 1);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1554 mem_cgroup_cancel_charge(new_page, memcg, true);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1555 unlock_page(new_page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1556 new_page->mapping = NULL;
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1557 }
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1558 out:
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1559 VM_BUG_ON(!list_empty(&pagelist));
a5318ea6 zhong jiang 2017-05-09 @1560 if (page != NULL && result != SCAN_SUCCEED)
a5318ea6 zhong jiang 2017-05-09 1561 put_page(new_page);
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1562 /* TODO: tracepoints */
f3f0e1d2 Kirill A. Shutemov 2016-07-26 1563 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page fails (fwd)
2017-05-09 15:43 [PATCH v2] mm: fix the memory leak after collapsing the huge page fails (fwd) Julia Lawall
@ 2017-05-10 5:55 ` zhong jiang
2017-05-10 6:25 ` Julia Lawall
0 siblings, 1 reply; 5+ messages in thread
From: zhong jiang @ 2017-05-10 5:55 UTC (permalink / raw)
To: Julia Lawall
Cc: akpm, vbabka, kirill.shutemov, hannes, mgorman, linux-mm,
kbuild-all
On 2017/5/9 23:43, Julia Lawall wrote:
> Hello,
>
> I don't know if there is a bug here, but it could e worth checking on. If
> the loop on line 1481 is executed, page will not be NULL at the out label
> on line 1560. Instead it will have a dummy value. Perhaps the value of
> result keeps the if at the out label from being taken.
>
> julia
Hi, Julia
it has no memory leak. so my initial thought is not correct. but I do not know you mean.
The page is local variable. it aybe a dummy value. but it should not cause any issue.
is it right? or I miss something.
Thanks
zhongjiang
> ---------- Forwarded message ----------
> Date: Tue, 9 May 2017 23:27:43 +0800
> From: kbuild test robot <fengguang.wu@intel.com>
> To: kbuild@01.org
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Subject: Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page
> fails
>
> Hi zhong,
>
> [auto build test WARNING on mmotm/master]
> [also build test WARNING on v4.11 next-20170509]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/zhongjiang/mm-fix-the-memory-leak-after-collapsing-the-huge-page-fails/20170509-193011
> base: git://git.cmpxchg.org/linux-mmotm.git master
> :::::: branch date: 4 hours ago
> :::::: commit date: 4 hours ago
>
>>> mm/khugepaged.c:1560:5-9: ERROR: invalid reference to the index variable of the iterator on line 1481
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout a5318ea654d5b764d6e06c6cfbfc21e44ce56e2b
> vim +1560 mm/khugepaged.c
>
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1475 struct zone *zone = page_zone(new_page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1476
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1477 /*
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1478 * Replacing old pages with new one has succeed, now we need to
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1479 * copy the content and free old pages.
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1480 */
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 @1481 list_for_each_entry_safe(page, tmp, &pagelist, lru) {
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1482 copy_highpage(new_page + (page->index % HPAGE_PMD_NR),
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1483 page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1484 list_del(&page->lru);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1485 unlock_page(page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1486 page_ref_unfreeze(page, 1);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1487 page->mapping = NULL;
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1488 ClearPageActive(page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1489 ClearPageUnevictable(page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1490 put_page(page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1491 }
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1492
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1493 local_irq_save(flags);
> 11fb9989 Mel Gorman 2016-07-28 1494 __inc_node_page_state(new_page, NR_SHMEM_THPS);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1495 if (nr_none) {
> 11fb9989 Mel Gorman 2016-07-28 1496 __mod_node_page_state(zone->zone_pgdat, NR_FILE_PAGES, nr_none);
> 11fb9989 Mel Gorman 2016-07-28 1497 __mod_node_page_state(zone->zone_pgdat, NR_SHMEM, nr_none);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1498 }
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1499 local_irq_restore(flags);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1500
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1501 /*
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1502 * Remove pte page tables, so we can re-faulti
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1503 * the page as huge.
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1504 */
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1505 retract_page_tables(mapping, start);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1506
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1507 /* Everything is ready, let's unfreeze the new_page */
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1508 set_page_dirty(new_page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1509 SetPageUptodate(new_page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1510 page_ref_unfreeze(new_page, HPAGE_PMD_NR);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1511 mem_cgroup_commit_charge(new_page, memcg, false, true);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1512 lru_cache_add_anon(new_page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1513 unlock_page(new_page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1514
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1515 *hpage = NULL;
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1516 } else {
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1517 /* Something went wrong: rollback changes to the radix-tree */
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1518 shmem_uncharge(mapping->host, nr_none);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1519 spin_lock_irq(&mapping->tree_lock);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1520 radix_tree_for_each_slot(slot, &mapping->page_tree, &iter,
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1521 start) {
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1522 if (iter.index >= end)
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1523 break;
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1524 page = list_first_entry_or_null(&pagelist,
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1525 struct page, lru);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1526 if (!page || iter.index < page->index) {
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1527 if (!nr_none)
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1528 break;
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1529 nr_none--;
> 59749e6c Johannes Weiner 2016-12-12 1530 /* Put holes back where they were */
> 59749e6c Johannes Weiner 2016-12-12 1531 radix_tree_delete(&mapping->page_tree,
> 59749e6c Johannes Weiner 2016-12-12 1532 iter.index);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1533 continue;
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1534 }
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1535
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1536 VM_BUG_ON_PAGE(page->index != iter.index, page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1537
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1538 /* Unfreeze the page. */
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1539 list_del(&page->lru);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1540 page_ref_unfreeze(page, 2);
> 6d75f366 Johannes Weiner 2016-12-12 1541 radix_tree_replace_slot(&mapping->page_tree,
> 6d75f366 Johannes Weiner 2016-12-12 1542 slot, page);
> 148deab2 Matthew Wilcox 2016-12-14 1543 slot = radix_tree_iter_resume(slot, &iter);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1544 spin_unlock_irq(&mapping->tree_lock);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1545 putback_lru_page(page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1546 unlock_page(page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1547 spin_lock_irq(&mapping->tree_lock);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1548 }
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1549 VM_BUG_ON(nr_none);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1550 spin_unlock_irq(&mapping->tree_lock);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1551
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1552 /* Unfreeze new_page, caller would take care about freeing it */
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1553 page_ref_unfreeze(new_page, 1);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1554 mem_cgroup_cancel_charge(new_page, memcg, true);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1555 unlock_page(new_page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1556 new_page->mapping = NULL;
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1557 }
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1558 out:
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1559 VM_BUG_ON(!list_empty(&pagelist));
> a5318ea6 zhong jiang 2017-05-09 @1560 if (page != NULL && result != SCAN_SUCCEED)
> a5318ea6 zhong jiang 2017-05-09 1561 put_page(new_page);
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1562 /* TODO: tracepoints */
> f3f0e1d2 Kirill A. Shutemov 2016-07-26 1563 }
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
> .
>
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page fails (fwd)
2017-05-10 5:55 ` zhong jiang
@ 2017-05-10 6:25 ` Julia Lawall
2017-05-10 6:48 ` Vlastimil Babka
0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2017-05-10 6:25 UTC (permalink / raw)
To: zhong jiang
Cc: akpm, vbabka, kirill.shutemov, hannes, mgorman, linux-mm,
kbuild-all
On Wed, 10 May 2017, zhong jiang wrote:
> On 2017/5/9 23:43, Julia Lawall wrote:
> > Hello,
> >
> > I don't know if there is a bug here, but it could e worth checking on. If
> > the loop on line 1481 is executed, page will not be NULL at the out label
> > on line 1560. Instead it will have a dummy value. Perhaps the value of
> > result keeps the if at the out label from being taken.
> >
> > julia
> Hi, Julia
>
> it has no memory leak. so my initial thought is not correct. but I do not know you mean.
> The page is local variable. it aybe a dummy value. but it should not cause any issue.
> is it right? or I miss something.
I had first been thinking that the if branch was referencing page. In
that case, if page were a dummy value, then there could be a problem. But
now I see that the branch does not refer to page. So the question is
just, if the loop on lines 1481-1491 is executed, is it correct to execute
the code put_page(new_page)? Or will result be SCAN_SUCCEEDED in that
case?
julia
>
> Thanks
> zhongjiang
> > ---------- Forwarded message ----------
> > Date: Tue, 9 May 2017 23:27:43 +0800
> > From: kbuild test robot <fengguang.wu@intel.com>
> > To: kbuild@01.org
> > Cc: Julia Lawall <julia.lawall@lip6.fr>
> > Subject: Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page
> > fails
> >
> > Hi zhong,
> >
> > [auto build test WARNING on mmotm/master]
> > [also build test WARNING on v4.11 next-20170509]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url: https://github.com/0day-ci/linux/commits/zhongjiang/mm-fix-the-memory-leak-after-collapsing-the-huge-page-fails/20170509-193011
> > base: git://git.cmpxchg.org/linux-mmotm.git master
> > :::::: branch date: 4 hours ago
> > :::::: commit date: 4 hours ago
> >
> >>> mm/khugepaged.c:1560:5-9: ERROR: invalid reference to the index variable of the iterator on line 1481
> > git remote add linux-review https://github.com/0day-ci/linux
> > git remote update linux-review
> > git checkout a5318ea654d5b764d6e06c6cfbfc21e44ce56e2b
> > vim +1560 mm/khugepaged.c
> >
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1475 struct zone *zone = page_zone(new_page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1476
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1477 /*
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1478 * Replacing old pages with new one has succeed, now we need to
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1479 * copy the content and free old pages.
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1480 */
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 @1481 list_for_each_entry_safe(page, tmp, &pagelist, lru) {
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1482 copy_highpage(new_page + (page->index % HPAGE_PMD_NR),
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1483 page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1484 list_del(&page->lru);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1485 unlock_page(page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1486 page_ref_unfreeze(page, 1);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1487 page->mapping = NULL;
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1488 ClearPageActive(page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1489 ClearPageUnevictable(page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1490 put_page(page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1491 }
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1492
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1493 local_irq_save(flags);
> > 11fb9989 Mel Gorman 2016-07-28 1494 __inc_node_page_state(new_page, NR_SHMEM_THPS);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1495 if (nr_none) {
> > 11fb9989 Mel Gorman 2016-07-28 1496 __mod_node_page_state(zone->zone_pgdat, NR_FILE_PAGES, nr_none);
> > 11fb9989 Mel Gorman 2016-07-28 1497 __mod_node_page_state(zone->zone_pgdat, NR_SHMEM, nr_none);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1498 }
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1499 local_irq_restore(flags);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1500
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1501 /*
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1502 * Remove pte page tables, so we can re-faulti
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1503 * the page as huge.
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1504 */
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1505 retract_page_tables(mapping, start);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1506
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1507 /* Everything is ready, let's unfreeze the new_page */
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1508 set_page_dirty(new_page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1509 SetPageUptodate(new_page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1510 page_ref_unfreeze(new_page, HPAGE_PMD_NR);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1511 mem_cgroup_commit_charge(new_page, memcg, false, true);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1512 lru_cache_add_anon(new_page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1513 unlock_page(new_page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1514
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1515 *hpage = NULL;
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1516 } else {
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1517 /* Something went wrong: rollback changes to the radix-tree */
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1518 shmem_uncharge(mapping->host, nr_none);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1519 spin_lock_irq(&mapping->tree_lock);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1520 radix_tree_for_each_slot(slot, &mapping->page_tree, &iter,
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1521 start) {
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1522 if (iter.index >= end)
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1523 break;
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1524 page = list_first_entry_or_null(&pagelist,
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1525 struct page, lru);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1526 if (!page || iter.index < page->index) {
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1527 if (!nr_none)
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1528 break;
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1529 nr_none--;
> > 59749e6c Johannes Weiner 2016-12-12 1530 /* Put holes back where they were */
> > 59749e6c Johannes Weiner 2016-12-12 1531 radix_tree_delete(&mapping->page_tree,
> > 59749e6c Johannes Weiner 2016-12-12 1532 iter.index);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1533 continue;
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1534 }
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1535
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1536 VM_BUG_ON_PAGE(page->index != iter.index, page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1537
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1538 /* Unfreeze the page. */
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1539 list_del(&page->lru);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1540 page_ref_unfreeze(page, 2);
> > 6d75f366 Johannes Weiner 2016-12-12 1541 radix_tree_replace_slot(&mapping->page_tree,
> > 6d75f366 Johannes Weiner 2016-12-12 1542 slot, page);
> > 148deab2 Matthew Wilcox 2016-12-14 1543 slot = radix_tree_iter_resume(slot, &iter);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1544 spin_unlock_irq(&mapping->tree_lock);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1545 putback_lru_page(page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1546 unlock_page(page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1547 spin_lock_irq(&mapping->tree_lock);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1548 }
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1549 VM_BUG_ON(nr_none);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1550 spin_unlock_irq(&mapping->tree_lock);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1551
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1552 /* Unfreeze new_page, caller would take care about freeing it */
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1553 page_ref_unfreeze(new_page, 1);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1554 mem_cgroup_cancel_charge(new_page, memcg, true);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1555 unlock_page(new_page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1556 new_page->mapping = NULL;
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1557 }
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1558 out:
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1559 VM_BUG_ON(!list_empty(&pagelist));
> > a5318ea6 zhong jiang 2017-05-09 @1560 if (page != NULL && result != SCAN_SUCCEED)
> > a5318ea6 zhong jiang 2017-05-09 1561 put_page(new_page);
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1562 /* TODO: tracepoints */
> > f3f0e1d2 Kirill A. Shutemov 2016-07-26 1563 }
> >
> > ---
> > 0-DAY kernel test infrastructure Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all Intel Corporation
> >
> > .
> >
>
>
>
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page fails (fwd)
2017-05-10 6:25 ` Julia Lawall
@ 2017-05-10 6:48 ` Vlastimil Babka
2017-05-10 6:50 ` Julia Lawall
0 siblings, 1 reply; 5+ messages in thread
From: Vlastimil Babka @ 2017-05-10 6:48 UTC (permalink / raw)
To: Julia Lawall, zhong jiang
Cc: akpm, kirill.shutemov, hannes, mgorman, linux-mm, kbuild-all
On 05/10/2017 08:25 AM, Julia Lawall wrote:
>
>
> On Wed, 10 May 2017, zhong jiang wrote:
>
>> On 2017/5/9 23:43, Julia Lawall wrote:
>>> Hello,
>>>
>>> I don't know if there is a bug here, but it could e worth checking on. If
>>> the loop on line 1481 is executed, page will not be NULL at the out label
>>> on line 1560. Instead it will have a dummy value. Perhaps the value of
>>> result keeps the if at the out label from being taken.
>>>
>>> julia
>> Hi, Julia
>>
>> it has no memory leak. so my initial thought is not correct. but I do not know you mean.
>> The page is local variable. it aybe a dummy value. but it should not cause any issue.
>> is it right? or I miss something.
>
> I had first been thinking that the if branch was referencing page. In
> that case, if page were a dummy value, then there could be a problem. But
> now I see that the branch does not refer to page. So the question is
> just, if the loop on lines 1481-1491 is executed, is it correct to execute
> the code put_page(new_page)? Or will result be SCAN_SUCCEEDED in that
> case?
That loop is under "if (result == SCAN_SUCCEED)", so yeah. It also ends
with "*hpage = NULL;", so the put_page(hpage) in khugepaged_do_scan()
won't apply. I see no problem besides the very non-obvious code :/
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page fails (fwd)
2017-05-10 6:48 ` Vlastimil Babka
@ 2017-05-10 6:50 ` Julia Lawall
0 siblings, 0 replies; 5+ messages in thread
From: Julia Lawall @ 2017-05-10 6:50 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Julia Lawall, zhong jiang, akpm, kirill.shutemov, hannes, mgorman,
linux-mm, kbuild-all
On Wed, 10 May 2017, Vlastimil Babka wrote:
> On 05/10/2017 08:25 AM, Julia Lawall wrote:
> >
> >
> > On Wed, 10 May 2017, zhong jiang wrote:
> >
> >> On 2017/5/9 23:43, Julia Lawall wrote:
> >>> Hello,
> >>>
> >>> I don't know if there is a bug here, but it could e worth checking on. If
> >>> the loop on line 1481 is executed, page will not be NULL at the out label
> >>> on line 1560. Instead it will have a dummy value. Perhaps the value of
> >>> result keeps the if at the out label from being taken.
> >>>
> >>> julia
> >> Hi, Julia
> >>
> >> it has no memory leak. so my initial thought is not correct. but I do not know you mean.
> >> The page is local variable. it aybe a dummy value. but it should not cause any issue.
> >> is it right? or I miss something.
> >
> > I had first been thinking that the if branch was referencing page. In
> > that case, if page were a dummy value, then there could be a problem. But
> > now I see that the branch does not refer to page. So the question is
> > just, if the loop on lines 1481-1491 is executed, is it correct to execute
> > the code put_page(new_page)? Or will result be SCAN_SUCCEEDED in that
> > case?
>
> That loop is under "if (result == SCAN_SUCCEED)", so yeah. It also ends
> with "*hpage = NULL;", so the put_page(hpage) in khugepaged_do_scan()
> won't apply. I see no problem besides the very non-obvious code :/
OK, thanks for the clarification.
julia
--
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-10 6:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-09 15:43 [PATCH v2] mm: fix the memory leak after collapsing the huge page fails (fwd) Julia Lawall
2017-05-10 5:55 ` zhong jiang
2017-05-10 6:25 ` Julia Lawall
2017-05-10 6:48 ` Vlastimil Babka
2017-05-10 6:50 ` Julia Lawall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).