[bug report] mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page

[bug report] mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page

[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)'

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

Re: [bug report] mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page

[Oscar Salvador]

On Tue, Aug 25, 2020 at 01:25:46PM +0300, Dan Carpenter wrote:
> 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.

> 
> 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;?

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.

-- 
Oscar Salvador
SUSE L3

Re: [bug report] mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page

[Oscar Salvador]

On Tue, Aug 25, 2020 at 12:35:17PM +0200, Oscar Salvador wrote:
> > 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;?
> 
> 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/

-- 
Oscar Salvador
SUSE L3

Re: [bug report] mm,hwpoison: refactor soft_offline_huge_page and __soft_offline_page

[Andrew Morton]

On Tue, 25 Aug 2020 12:39:22 +0200 Oscar Salvador <osalvador@suse.de> wrote:

> On Tue, Aug 25, 2020 at 12:35:17PM +0200, Oscar Salvador wrote:
> > > 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;?
> > 
> > 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/

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