Re: [RFC PATCH v2 3/5] mm: Add explicit page decrement in exception path for isolate_lru_pages
On Wed, Aug 19, 2020 at 12:52 AM Alex Shi wrote:
>
>
>
> 在 2020/8/19 下午12:27, Alexander Duyck 写道:
> > From: Alexander Duyck
> >
> > In isolate_lru_pages we have an exception path where if we call
> > get_page_unless_zero and that succeeds, but TestClearPageLRU fails we call
> > put_page. Normally this would be problematic but due to the way that the
> > calls are ordered and the fact that we are holding the LRU lock we know
> > that the caller must be holding another reference for the page. Since we
> > can assume that we can replace the put_page with a call to
> > put_page_testzero contained within a WARN_ON. By doing this we should see
> > if we ever leak a page as a result of the reference count somehow hitting
> > zero when it shouldn't, and can avoid the overhead and confusion of using
> > the full put_page call.
> >
> > Signed-off-by: Alexander Duyck
> > ---
> > mm/vmscan.c |9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 5bc0c2322043..3ebe3f9b653b 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1688,10 +1688,13 @@ static unsigned long isolate_lru_pages(unsigned
> > long nr_to_scan,
> >
> > if (!TestClearPageLRU(page)) {
> > /*
> > - * This page may in other isolation path,
> > - * but we still hold lru_lock.
> > + * This page is being isolated in another
> > + * thread, but we still hold lru_lock. The
> > + * other thread must be holding a reference
> > + * to the page so this should never hit a
> > + * reference count of 0.
> >*/
> > - put_page(page);
> > + WARN_ON(put_page_testzero(page));
>
> seems WARN_ON is always enabled.
>
> Reviewed-by: Alex Shi
Yeah, it is always enabled however it should never be triggered. I had
considered just putting a page_ref_dec here since in theory this path
should never be triggered but I thought as a debug catch I add the
WARN_ON and put_page_testzero. If we ever do encounter this being
triggered then it will leak a page of memory which isn't the end of
the world but I thought would warrant a WARN_ON.
Re: [RFC PATCH v2 3/5] mm: Add explicit page decrement in exception path for isolate_lru_pages
在 2020/8/19 下午12:27, Alexander Duyck 写道:
> From: Alexander Duyck
>
> In isolate_lru_pages we have an exception path where if we call
> get_page_unless_zero and that succeeds, but TestClearPageLRU fails we call
> put_page. Normally this would be problematic but due to the way that the
> calls are ordered and the fact that we are holding the LRU lock we know
> that the caller must be holding another reference for the page. Since we
> can assume that we can replace the put_page with a call to
> put_page_testzero contained within a WARN_ON. By doing this we should see
> if we ever leak a page as a result of the reference count somehow hitting
> zero when it shouldn't, and can avoid the overhead and confusion of using
> the full put_page call.
>
> Signed-off-by: Alexander Duyck
> ---
> mm/vmscan.c |9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 5bc0c2322043..3ebe3f9b653b 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1688,10 +1688,13 @@ static unsigned long isolate_lru_pages(unsigned long
> nr_to_scan,
>
> if (!TestClearPageLRU(page)) {
> /*
> - * This page may in other isolation path,
> - * but we still hold lru_lock.
> + * This page is being isolated in another
> + * thread, but we still hold lru_lock. The
> + * other thread must be holding a reference
> + * to the page so this should never hit a
> + * reference count of 0.
>*/
> - put_page(page);
> + WARN_ON(put_page_testzero(page));
seems WARN_ON is always enabled.
Reviewed-by: Alex Shi
> goto busy;
> }
>
>
[RFC PATCH v2 3/5] mm: Add explicit page decrement in exception path for isolate_lru_pages
From: Alexander Duyck
In isolate_lru_pages we have an exception path where if we call
get_page_unless_zero and that succeeds, but TestClearPageLRU fails we call
put_page. Normally this would be problematic but due to the way that the
calls are ordered and the fact that we are holding the LRU lock we know
that the caller must be holding another reference for the page. Since we
can assume that we can replace the put_page with a call to
put_page_testzero contained within a WARN_ON. By doing this we should see
if we ever leak a page as a result of the reference count somehow hitting
zero when it shouldn't, and can avoid the overhead and confusion of using
the full put_page call.
Signed-off-by: Alexander Duyck
---
mm/vmscan.c |9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 5bc0c2322043..3ebe3f9b653b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1688,10 +1688,13 @@ static unsigned long isolate_lru_pages(unsigned long
nr_to_scan,
if (!TestClearPageLRU(page)) {
/*
-* This page may in other isolation path,
-* but we still hold lru_lock.
+* This page is being isolated in another
+* thread, but we still hold lru_lock. The
+* other thread must be holding a reference
+* to the page so this should never hit a
+* reference count of 0.
*/
- put_page(page);
+ WARN_ON(put_page_testzero(page));
goto busy;
}
