Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Tue, Jul 2, 2019 at 10:54 PM Vitaly Wool wrote:
>
> On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton
> wrote:
> >
> > On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns wrote:
> >
> > > > > > > + if (can_sleep) {
> > > > > > > + lock_page(page);
> > > > > > > + __SetPageMovable(page, pool->inode->i_mapping);
> > > > > > > + unlock_page(page);
> > > > > > > + } else {
> > > > > > > + if (!WARN_ON(!trylock_page(page))) {
> > > > > > > + __SetPageMovable(page,
> > > > > > > pool->inode->i_mapping);
> > > > > > > + unlock_page(page);
> > > > > > > + } else {
> > > > > > > + pr_err("Newly allocated z3fold page is
> > > > > > > locked\n");
> > > > > > > + WARN_ON(1);
> >
> > The WARN_ON will have already warned in this case.
> >
> > But the whole idea of warning in this case may be undesirable. We KNOW
> > that the warning will sometimes trigger (yes?). So what's the point in
> > scaring users?
>
> Well, normally a newly allocated page that we own should not be locked
> by someone else so this is worth a warning IMO. With that said, the
> else branch here appears to be redundant.
The else branch has been removed, and I think it's possible (albeit unlikely)
that the trylock could fail due to either compaction or kstaled
(In which case the page just won't be movable).
Also Vitaly, do you have a preference between the two emails? I'm not sure which
one to include.
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Wed, Jul 3, 2019 at 12:18 AM Henry Burns wrote: > > On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton > wrote: > > > > On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns wrote: > > > > > Cc: Vitaly Wool , Vitaly Vul > > > > Are these the same person? > I Think it's the same person, but i wasn't sure which email to include > because one was > in the list of maintainers and I had contacted the other earlier. This is the same person, it's the transliteration done differently that caused this :) ~Vitaly
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton wrote:
>
> On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns wrote:
>
> > > > > > + if (can_sleep) {
> > > > > > + lock_page(page);
> > > > > > + __SetPageMovable(page, pool->inode->i_mapping);
> > > > > > + unlock_page(page);
> > > > > > + } else {
> > > > > > + if (!WARN_ON(!trylock_page(page))) {
> > > > > > + __SetPageMovable(page,
> > > > > > pool->inode->i_mapping);
> > > > > > + unlock_page(page);
> > > > > > + } else {
> > > > > > + pr_err("Newly allocated z3fold page is
> > > > > > locked\n");
> > > > > > + WARN_ON(1);
>
> The WARN_ON will have already warned in this case.
>
> But the whole idea of warning in this case may be undesirable. We KNOW
> that the warning will sometimes trigger (yes?). So what's the point in
> scaring users?
Well, normally a newly allocated page that we own should not be locked
by someone else so this is worth a warning IMO. With that said, the
else branch here appears to be redundant.
~Vitaly
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns wrote:
> Cc: Vitaly Wool , Vitaly Vul
Are these the same person?
> Subject: Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before
> __SetPageMovable()
> Date: Mon, 1 Jul 2019 18:16:30 -0700
>
> On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt wrote:
> >
> > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns wrote:
> > >
> > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> > > lock the page. Following zsmalloc.c's example we call trylock_page() and
> > > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> > > passed in locked, as documentation.
The changelog still doesn't mention that this bug triggers a
VM_BUG_ON_PAGE(). It should do so. I did this:
: __SetPageMovable() expects its page to be locked, but z3fold.c doesn't
: lock the page. This triggers the VM_BUG_ON_PAGE(!PageLocked(page), page)
: in __SetPageMovable().
:
: Following zsmalloc.c's example we call trylock_page() and unlock_page().
: Also make z3fold_page_migrate() assert that newpage is passed in locked,
: as per the documentation.
I'll add a cc:stable to this fix.
> > > Signed-off-by: Henry Burns
> > > Suggested-by: Vitaly Wool
> > > ---
> > > Changelog since v1:
> > > - Added an if statement around WARN_ON(trylock_page(page)) to avoid
> > >unlocking a page locked by a someone else.
> > >
> > > mm/z3fold.c | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/z3fold.c b/mm/z3fold.c
> > > index e174d1549734..6341435b9610 100644
> > > --- a/mm/z3fold.c
> > > +++ b/mm/z3fold.c
> > > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool,
> > > size_t size, gfp_t gfp,
> > > set_bit(PAGE_HEADLESS, &page->private);
> > > goto headless;
> > > }
> > > - __SetPageMovable(page, pool->inode->i_mapping);
> > > + if (!WARN_ON(!trylock_page(page))) {
> > > + __SetPageMovable(page, pool->inode->i_mapping);
> > > + unlock_page(page);
> > > + }
> >
> > Can you please comment why lock_page() is not used here?
Shakeel asked "please comment" (ie, please add a code comment), not
"please comment on". Subtle ;)
> Since z3fold_alloc can be called in atomic or non atomic context,
> calling lock_page() could trigger a number of
> warnings about might_sleep() being called in atomic context. WARN_ON
> should avoid the problem described
> above as well, and in any weird condition where someone else has the
> page lock, we can avoid calling
> __SetPageMovable().
I think this will suffice:
--- a/mm/z3fold.c~mm-z3foldc-lock-z3fold-page-before-__setpagemovable-fix
+++ a/mm/z3fold.c
@@ -919,6 +919,9 @@ retry:
set_bit(PAGE_HEADLESS, &page->private);
goto headless;
}
+ /*
+* z3fold_alloc() can be called from atomic contexts, hence the trylock
+*/
if (!WARN_ON(!trylock_page(page))) {
__SetPageMovable(page, pool->inode->i_mapping);
unlock_page(page);
However this code would be more effective if z3fold_alloc() were to be
told when it is running in non-atomic context so it can perform a
sleeping lock_page() in that case. That's an improvement to consider
for later, please.
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton wrote:
>
> On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns wrote:
>
> > Cc: Vitaly Wool , Vitaly Vul
>
> Are these the same person?
I Think it's the same person, but i wasn't sure which email to include
because one was
in the list of maintainers and I had contacted the other earlier.
>
> > Subject: Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before
> > __SetPageMovable()
> > Date: Mon, 1 Jul 2019 18:16:30 -0700
> >
> > On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt wrote:
> > >
> > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns wrote:
> > > >
> > > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> > > > lock the page. Following zsmalloc.c's example we call trylock_page() and
> > > > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> > > > passed in locked, as documentation.
>
> The changelog still doesn't mention that this bug triggers a
> VM_BUG_ON_PAGE(). It should do so. I did this:
>
> : __SetPageMovable() expects its page to be locked, but z3fold.c doesn't
> : lock the page. This triggers the VM_BUG_ON_PAGE(!PageLocked(page), page)
> : in __SetPageMovable().
> :
> : Following zsmalloc.c's example we call trylock_page() and unlock_page().
> : Also make z3fold_page_migrate() assert that newpage is passed in locked,
> : as per the documentation.
>
> I'll add a cc:stable to this fix.
>
> > > > Signed-off-by: Henry Burns
> > > > Suggested-by: Vitaly Wool
> > > > ---
> > > > Changelog since v1:
> > > > - Added an if statement around WARN_ON(trylock_page(page)) to avoid
> > > >unlocking a page locked by a someone else.
> > > >
> > > > mm/z3fold.c | 6 +-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/mm/z3fold.c b/mm/z3fold.c
> > > > index e174d1549734..6341435b9610 100644
> > > > --- a/mm/z3fold.c
> > > > +++ b/mm/z3fold.c
> > > > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool,
> > > > size_t size, gfp_t gfp,
> > > > set_bit(PAGE_HEADLESS, &page->private);
> > > > goto headless;
> > > > }
> > > > - __SetPageMovable(page, pool->inode->i_mapping);
> > > > + if (!WARN_ON(!trylock_page(page))) {
> > > > + __SetPageMovable(page, pool->inode->i_mapping);
> > > > + unlock_page(page);
> > > > + }
> > >
> > > Can you please comment why lock_page() is not used here?
>
> Shakeel asked "please comment" (ie, please add a code comment), not
> "please comment on". Subtle ;)
>
> > Since z3fold_alloc can be called in atomic or non atomic context,
> > calling lock_page() could trigger a number of
> > warnings about might_sleep() being called in atomic context. WARN_ON
> > should avoid the problem described
> > above as well, and in any weird condition where someone else has the
> > page lock, we can avoid calling
> > __SetPageMovable().
>
> I think this will suffice:
>
> --- a/mm/z3fold.c~mm-z3foldc-lock-z3fold-page-before-__setpagemovable-fix
> +++ a/mm/z3fold.c
> @@ -919,6 +919,9 @@ retry:
> set_bit(PAGE_HEADLESS, &page->private);
> goto headless;
> }
> + /*
> +* z3fold_alloc() can be called from atomic contexts, hence the
> trylock
> +*/
> if (!WARN_ON(!trylock_page(page))) {
> __SetPageMovable(page, pool->inode->i_mapping);
> unlock_page(page);
>
> However this code would be more effective if z3fold_alloc() were to be
> told when it is running in non-atomic context so it can perform a
> sleeping lock_page() in that case. That's an improvement to consider
> for later, please.
>
z3fold_alloc() can tell when its called in atomic context, new patch incoming!
I'm thinking something like this:
> > > > + if (can_sleep) {
> > > > + lock_page(page);
> > > > + __SetPageMovable(page, pool->inode->i_mapping);
> > > > + unlock_page(page);
> > > > + } else {
> > > > + if (!WARN_ON(!trylock_page(page))) {
> > > > + __SetPageMovable(page, pool->inode->i_mapping);
> > > > + unlock_page(page);
> > > > + } else {
> > > > + pr_err("Newly allocated z3fold page is
> > > > locked\n");
> > > > + WARN_ON(1);
> > > > + }
> > > > + }
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns wrote:
> > > > > + if (can_sleep) {
> > > > > + lock_page(page);
> > > > > + __SetPageMovable(page, pool->inode->i_mapping);
> > > > > + unlock_page(page);
> > > > > + } else {
> > > > > + if (!WARN_ON(!trylock_page(page))) {
> > > > > + __SetPageMovable(page,
> > > > > pool->inode->i_mapping);
> > > > > + unlock_page(page);
> > > > > + } else {
> > > > > + pr_err("Newly allocated z3fold page is
> > > > > locked\n");
> > > > > + WARN_ON(1);
The WARN_ON will have already warned in this case.
But the whole idea of warning in this case may be undesirable. We KNOW
that the warning will sometimes trigger (yes?). So what's the point in
scaring users?
Also, pr_err(...)+WARN_ON(1) can basically be replaced with WARN(1, ...)?
> > > > > + }
> > > > > + }
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Mon, 1 Jul 2019, Henry Burns wrote: > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't > lock the page. Following zsmalloc.c's example we call trylock_page() and > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is > passed in locked, as documentation. > > Signed-off-by: Henry Burns > Suggested-by: Vitaly Wool Acked-by: David Rientjes
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Tue, Jul 2, 2019 at 3:51 AM Henry Burns wrote:
>
> __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> lock the page. Following zsmalloc.c's example we call trylock_page() and
> unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> passed in locked, as documentation.
>
> Signed-off-by: Henry Burns
> Suggested-by: Vitaly Wool
Acked-by: Vitaly Wool
Thanks!
> ---
> Changelog since v1:
> - Added an if statement around WARN_ON(trylock_page(page)) to avoid
>unlocking a page locked by a someone else.
>
> mm/z3fold.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e174d1549734..6341435b9610 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t
> size, gfp_t gfp,
> set_bit(PAGE_HEADLESS, &page->private);
> goto headless;
> }
> - __SetPageMovable(page, pool->inode->i_mapping);
> + if (!WARN_ON(!trylock_page(page))) {
> + __SetPageMovable(page, pool->inode->i_mapping);
> + unlock_page(page);
> + }
> z3fold_page_lock(zhdr);
>
> found:
> @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space
> *mapping, struct page *newpa
>
> VM_BUG_ON_PAGE(!PageMovable(page), page);
> VM_BUG_ON_PAGE(!PageIsolated(page), page);
> + VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
>
> zhdr = page_address(page);
> pool = zhdr_to_pool(zhdr);
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt wrote:
>
> On Mon, Jul 1, 2019 at 5:51 PM Henry Burns wrote:
> >
> > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> > lock the page. Following zsmalloc.c's example we call trylock_page() and
> > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> > passed in locked, as documentation.
> >
> > Signed-off-by: Henry Burns
> > Suggested-by: Vitaly Wool
> > ---
> > Changelog since v1:
> > - Added an if statement around WARN_ON(trylock_page(page)) to avoid
> >unlocking a page locked by a someone else.
> >
> > mm/z3fold.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/z3fold.c b/mm/z3fold.c
> > index e174d1549734..6341435b9610 100644
> > --- a/mm/z3fold.c
> > +++ b/mm/z3fold.c
> > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool,
> > size_t size, gfp_t gfp,
> > set_bit(PAGE_HEADLESS, &page->private);
> > goto headless;
> > }
> > - __SetPageMovable(page, pool->inode->i_mapping);
> > + if (!WARN_ON(!trylock_page(page))) {
> > + __SetPageMovable(page, pool->inode->i_mapping);
> > + unlock_page(page);
> > + }
>
> Can you please comment why lock_page() is not used here?
Since z3fold_alloc can be called in atomic or non atomic context,
calling lock_page() could trigger a number of
warnings about might_sleep() being called in atomic context. WARN_ON
should avoid the problem described
above as well, and in any weird condition where someone else has the
page lock, we can avoid calling
__SetPageMovable().
>
> > z3fold_page_lock(zhdr);
> >
> > found:
> > @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space
> > *mapping, struct page *newpa
> >
> > VM_BUG_ON_PAGE(!PageMovable(page), page);
> > VM_BUG_ON_PAGE(!PageIsolated(page), page);
> > + VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
> >
> > zhdr = page_address(page);
> > pool = zhdr_to_pool(zhdr);
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
On Mon, Jul 1, 2019 at 5:51 PM Henry Burns wrote:
>
> __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> lock the page. Following zsmalloc.c's example we call trylock_page() and
> unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> passed in locked, as documentation.
>
> Signed-off-by: Henry Burns
> Suggested-by: Vitaly Wool
> ---
> Changelog since v1:
> - Added an if statement around WARN_ON(trylock_page(page)) to avoid
>unlocking a page locked by a someone else.
>
> mm/z3fold.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e174d1549734..6341435b9610 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t
> size, gfp_t gfp,
> set_bit(PAGE_HEADLESS, &page->private);
> goto headless;
> }
> - __SetPageMovable(page, pool->inode->i_mapping);
> + if (!WARN_ON(!trylock_page(page))) {
> + __SetPageMovable(page, pool->inode->i_mapping);
> + unlock_page(page);
> + }
Can you please comment why lock_page() is not used here?
> z3fold_page_lock(zhdr);
>
> found:
> @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space
> *mapping, struct page *newpa
>
> VM_BUG_ON_PAGE(!PageMovable(page), page);
> VM_BUG_ON_PAGE(!PageIsolated(page), page);
> + VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
>
> zhdr = page_address(page);
> pool = zhdr_to_pool(zhdr);
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
[PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
__SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
lock the page. Following zsmalloc.c's example we call trylock_page() and
unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
passed in locked, as documentation.
Signed-off-by: Henry Burns
Suggested-by: Vitaly Wool
---
Changelog since v1:
- Added an if statement around WARN_ON(trylock_page(page)) to avoid
unlocking a page locked by a someone else.
mm/z3fold.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index e174d1549734..6341435b9610 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t
size, gfp_t gfp,
set_bit(PAGE_HEADLESS, &page->private);
goto headless;
}
- __SetPageMovable(page, pool->inode->i_mapping);
+ if (!WARN_ON(!trylock_page(page))) {
+ __SetPageMovable(page, pool->inode->i_mapping);
+ unlock_page(page);
+ }
z3fold_page_lock(zhdr);
found:
@@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space
*mapping, struct page *newpa
VM_BUG_ON_PAGE(!PageMovable(page), page);
VM_BUG_ON_PAGE(!PageIsolated(page), page);
+ VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
zhdr = page_address(page);
pool = zhdr_to_pool(zhdr);
--
2.22.0.410.gd8fdbe21b5-goog
