Skip to contentSkip to conversation

mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page

bug report Dan Carpenter 3 participants 4 messages Aug 25, 2020 – Aug 26, 2020 Memory management

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Oscar Salvador,

The patch e5bcf91e5cc6: "mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page" from Aug 20, 2020, leads to the following static checker warning:

	mm/memory-failure.c:1765 isolate_page()
	warn: signedness bug returning '(-16)'

mm/memory-failure.c
1749 static bool isolate_page(struct page *page, struct list_head *pagelist) 1750 {
1751 bool isolated = false;
1752 bool lru = PageLRU(page);
1753
1754 if (PageHuge(page)) {
1755 isolated = isolate_huge_page(page, pagelist);
1756 } else {
1757 if (lru)
1758 isolated = !isolate_lru_page(page);
1759 else
1760 isolated = !isolate_movable_page(page, ISOLATE_UNEVICTABLE);
1761
1762 if (isolated)
1763 list_add(&page->lru, pagelist);
1764
1765 return -EBUSY;

return false;?

1766 }
1767
1768 if (isolated && lru)
1769 inc_node_page_state(page, NR_ISOLATED_ANON +
1770 page_is_file_lru(page));
1771
1772 /*

  1773           * If we succeed to isolate the page, we grabbed another refcount on
  1774           * the page, so we can safely drop the one we got from get_any_pages().
  1775           * If we failed to isolate the page, it means that we cannot go further
  1776           * and we will return an error, so drop the reference we got from
  1777           * get_any_pages() as well.
  1778           */

1779 put_page(page);
1780 return isolated;
1781 }

regards,
dan carpenter

#1 lore raw
Quoted 8 lines · Dan Carpenter

Hello Oscar Salvador,

The patch e5bcf91e5cc6: "mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page" from Aug 20, 2020, leads to the following static checker warning:

	mm/memory-failure.c:1765 isolate_page()
	warn: signedness bug returning '(-16)'

Hi Dan,

thanks for spotting this.

Quoted 20 lines

mm/memory-failure.c
1749 static bool isolate_page(struct page *page, struct list_head *pagelist) 1750 {
1751 bool isolated = false;
1752 bool lru = PageLRU(page);
1753
1754 if (PageHuge(page)) {
1755 isolated = isolate_huge_page(page, pagelist);
1756 } else {
1757 if (lru)
1758 isolated = !isolate_lru_page(page);
1759 else
1760 isolated = !isolate_movable_page(page, ISOLATE_UNEVICTABLE);
1761
1762 if (isolated)
1763 list_add(&page->lru, pagelist);
1764
1765 return -EBUSY;

Quoted 1 line

return false;?

No, I this is a mistake, I guess it popped there when Naoya rebased.

That return -EBUSY should be go.
I'll let Andrew know, thanks.

Signature · 2 lines
Oscar Salvador
SUSE L3
#2 lore raw
Quoted 19 lines · Oscar Salvador
Quoted 19 lines

mm/memory-failure.c
1749 static bool isolate_page(struct page *page, struct list_head *pagelist) 1750 {
1751 bool isolated = false;
1752 bool lru = PageLRU(page);
1753
1754 if (PageHuge(page)) {
1755 isolated = isolate_huge_page(page, pagelist);
1756 } else {
1757 if (lru)
1758 isolated = !isolate_lru_page(page);
1759 else
1760 isolated = !isolate_movable_page(page, ISOLATE_UNEVICTABLE);
1761
1762 if (isolated)
1763 list_add(&page->lru, pagelist);
1764
1765 return -EBUSY;

Quoted 6 lines
Quoted 1 line

return false;?

No, I this is a mistake, I guess it popped there when Naoya rebased.

That return -EBUSY should be go.
I'll let Andrew know, thanks.

Unless I'm blind, it seems it wasn't Naoya's mistake either [1]

@Andrew, any idea what happened here? That "return -EBUSY" should not be there.

[1] https://patchwork.kernel.org/patch/11704101/

Signature · 2 lines
Oscar Salvador
SUSE L3
maintainer
#3 lore raw
Quoted 24 lines · On Tue, 25 Aug 2020 12:39:22 +0200 Oscar Salvador <[email protected]> wrote:
Quoted 23 lines · Oscar Salvador
Quoted 19 lines

mm/memory-failure.c
1749 static bool isolate_page(struct page *page, struct list_head *pagelist) 1750 {
1751 bool isolated = false;
1752 bool lru = PageLRU(page);
1753
1754 if (PageHuge(page)) {
1755 isolated = isolate_huge_page(page, pagelist);
1756 } else {
1757 if (lru)
1758 isolated = !isolate_lru_page(page);
1759 else
1760 isolated = !isolate_movable_page(page, ISOLATE_UNEVICTABLE);
1761
1762 if (isolated)
1763 list_add(&page->lru, pagelist);
1764
1765 return -EBUSY;

Quoted 1 line

return false;?

No, I this is a mistake, I guess it popped there when Naoya rebased.

Quoted 8 lines
Quoted 2 lines

That return -EBUSY should be go.
I'll let Andrew know, thanks.

Unless I'm blind, it seems it wasn't Naoya's mistake either [1]

@Andrew, any idea what happened here? That "return -EBUSY" should not be there.

[1] https://patchwork.kernel.org/patch/11704101/

Yup, thanks. That patchset has required quite a bit of conflict resolution and I obviously failed to delete that line.