Date:   Wed, 22 Feb 2023 11:24:54 -0500
From:   Peter Xu <peterx@redhat.com>
To:     David Stevens <stevensd@chromium.org>
Cc:     linux-mm@kvack.org, Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Yang Shi <shy828301@gmail.com>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 1/3] mm/khugepaged: refactor collapse_file control flow
Message-ID: <Y/ZB1tB2Dc/uvJ9S@x1n>
References: <20230217085439.2826375-1-stevensd@google.com>
 <20230217085439.2826375-2-stevensd@google.com>
 <Y/U9fBxVJdhxiZ1v@x1n>
 <CAD=HUj5DwoFHsC8TpN6PU3ThypVq=jhJST5vs0d17U9d7EX8+g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAD=HUj5DwoFHsC8TpN6PU3ThypVq=jhJST5vs0d17U9d7EX8+g@mail.gmail.com>
Precedence: bulk

On Wed, Feb 22, 2023 at 01:08:19PM +0900, David Stevens wrote:
> > >  out:
> > >       VM_BUG_ON(!list_empty(&pagelist));
> > > -     if (hpage) {
> > > -             mem_cgroup_uncharge(page_folio(hpage));
> > > -             put_page(hpage);
> > > -     }
> >
> > Moving this chunk seems wrong, as it can leak the huge page if
> > alloc_charge_hpage() failed on mem charging, iiuc.
> >
> > And I found that keeping it seems wrong either, because if mem charge
> > failed we'll uncharge even without charging it before.  But that's nothing
> > about this patch alone.
> >
> > Posted a patch for this:
> >
> > https://lore.kernel.org/all/20230221214344.609226-1-peterx@redhat.com/

[1]

> >
> > I _think_ this patch will make sense after rebasing to that fix if that's
> > correct, but please review it and double check.
> >
> 
> Ah, good catch. I didn't notice that alloc_charge_hpage could allocate
> *hpage even on failure. The failure path should work properly with
> your fix.

Thanks for confirming.

If we can merge above [1] before this patch, then I think this patch is
correct.  Only if so:

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu