| Message ID | 20190702005122.41036-1-henryburns@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable() | expand |
On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> 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 <henryburns@google.com> > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > --- > 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 >
On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> 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 <henryburns@google.com> > > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > > --- > > 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 > >
On Tue, Jul 2, 2019 at 3:51 AM Henry Burns <henryburns@google.com> 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 <henryburns@google.com> > Suggested-by: Vitaly Wool <vitalywool@gmail.com> Acked-by: Vitaly Wool <vitalywool@gmail.com> 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 >
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 <henryburns@google.com> > Suggested-by: Vitaly Wool <vitalywool@gmail.com> Acked-by: David Rientjes <rientjes@google.com>
On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote: > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com> 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 <shakeelb@google.com> wrote: > > > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> 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 <henryburns@google.com> > > > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > > > --- > > > 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.
On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote: > > > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com> > > 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 <shakeelb@google.com> wrote: > > > > > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> 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 <henryburns@google.com> > > > > Suggested-by: Vitaly Wool <vitalywool@gmail.com> > > > > --- > > > > 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); > > > > + } > > > > + }
On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> 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, ...)? > > > > > + } > > > > > + }
On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> 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
On Wed, Jul 3, 2019 at 12:18 AM Henry Burns <henryburns@google.com> wrote: > > On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote: > > > > > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com> > > > > 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
On Tue, Jul 2, 2019 at 10:54 PM Vitaly Wool <vitalywool@gmail.com> wrote: > > On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> 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.
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);
__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 <henryburns@google.com> Suggested-by: Vitaly Wool <vitalywool@gmail.com> --- 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(-)