Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1614165 > unrolled thread

Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store() Shakeel Butt <shakeelb@google.com> - 2017-03-31 19:10 +0200
    Re: [PATCH] mm/zswap: fix potential deadlock in  zswap_frontswap_store() Michal Hocko <mhocko@kernel.org> - 2017-04-03 10:50 +0200
      Re: [PATCH] mm/zswap: fix potential deadlock in  zswap_frontswap_store() Michal Hocko <mhocko@kernel.org> - 2017-04-03 14:30 +0200
      Re: [PATCH] mm/zswap: fix potential deadlock in  zswap_frontswap_store() Michal Hocko <mhocko@kernel.org> - 2017-04-03 14:50 +0200
        Re: [PATCH] mm/zswap: fix potential deadlock in  zswap_frontswap_store() Michal Hocko <mhocko@kernel.org> - 2017-04-03 15:30 +0200
      Re: [PATCH] mm/zswap: fix potential deadlock in  zswap_frontswap_store() Vlastimil Babka <vbabka@suse.cz> - 2017-04-03 15:30 +0200

#1614165 — Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()

On Fri, Mar 31, 2017 at 8:30 AM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> zswap_frontswap_store() is called during memory reclaim from
> __frontswap_store() from swap_writepage() from shrink_page_list().
> This may happen in NOFS context, thus zswap shouldn't use __GFP_FS,
> otherwise we may renter into fs code and deadlock.
> zswap_frontswap_store() also shouldn't use __GFP_IO to avoid recursion
> into itself.
>

Is it possible to enter fs code (or IO) from zswap_frontswap_store()
other than recursive memory reclaim? However recursive memory reclaim
is protected through PF_MEMALLOC task flag. The change seems fine but
IMHO reasoning needs an update. Adding Michal for expert opinion.

> zswap_frontswap_store() call zpool_malloc() with __GFP_NORETRY |
> __GFP_NOWARN | __GFP_KSWAPD_RECLAIM, so let's use the same flags for
> zswap_entry_cache_alloc() as well, instead of GFP_KERNEL.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
>  mm/zswap.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index eedc278..12ad7e9 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -966,6 +966,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
>         struct zswap_tree *tree = zswap_trees[type];
>         struct zswap_entry *entry, *dupentry;
>         struct crypto_comp *tfm;
> +       gfp_t gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
>         int ret;
>         unsigned int dlen = PAGE_SIZE, len;
>         unsigned long handle;
> @@ -989,7 +990,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
>         }
>
>         /* allocate entry */
> -       entry = zswap_entry_cache_alloc(GFP_KERNEL);
> +       entry = zswap_entry_cache_alloc(gfp);
>         if (!entry) {
>                 zswap_reject_kmemcache_fail++;
>                 ret = -ENOMEM;
> @@ -1017,9 +1018,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
>
>         /* store */
>         len = dlen + sizeof(struct zswap_header);
> -       ret = zpool_malloc(entry->pool->zpool, len,
> -                          __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
> -                          &handle);
> +       ret = zpool_malloc(entry->pool->zpool, len, gfp, &handle);
>         if (ret == -ENOSPC) {
>                 zswap_reject_compress_poor++;
>                 goto put_dstmem;
> --
> 2.10.2
>

[toc] | [next] | [standalone]

#1614987 — Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()

On Fri 31-03-17 10:00:30, Shakeel Butt wrote:
> On Fri, Mar 31, 2017 at 8:30 AM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
> > zswap_frontswap_store() is called during memory reclaim from
> > __frontswap_store() from swap_writepage() from shrink_page_list().
> > This may happen in NOFS context, thus zswap shouldn't use __GFP_FS,
> > otherwise we may renter into fs code and deadlock.
> > zswap_frontswap_store() also shouldn't use __GFP_IO to avoid recursion
> > into itself.
> >
> 
> Is it possible to enter fs code (or IO) from zswap_frontswap_store()
> other than recursive memory reclaim? However recursive memory reclaim
> is protected through PF_MEMALLOC task flag. The change seems fine but
> IMHO reasoning needs an update. Adding Michal for expert opinion.

Yes this is true. I haven't checked all the callers of
zswap_frontswap_store but is it fixing any real problem or just trying
to be overly cautious.
 
Btw...

> > zswap_frontswap_store() call zpool_malloc() with __GFP_NORETRY |
> > __GFP_NOWARN | __GFP_KSWAPD_RECLAIM, so let's use the same flags for
> > zswap_entry_cache_alloc() as well, instead of GFP_KERNEL.
> >
> > Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > ---
> >  mm/zswap.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index eedc278..12ad7e9 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -966,6 +966,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> >         struct zswap_tree *tree = zswap_trees[type];
> >         struct zswap_entry *entry, *dupentry;
> >         struct crypto_comp *tfm;
> > +       gfp_t gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;

This doesn't trigger direct reclaim so __GFP_NORETRY is bogus. I suspect
you didn't want GFP_NOWAIT alternative.

[...]
> > @@ -1017,9 +1018,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> >
> >         /* store */
> >         len = dlen + sizeof(struct zswap_header);
> > -       ret = zpool_malloc(entry->pool->zpool, len,
> > -                          __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
> > -                          &handle);
> > +       ret = zpool_malloc(entry->pool->zpool, len, gfp, &handle);

and here we used to do GFP_NOWAIT alternative already. What is going on
here?

> >         if (ret == -ENOSPC) {
> >                 zswap_reject_compress_poor++;
> >                 goto put_dstmem;
> > --
> > 2.10.2
> >

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1615167 — Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()

On Mon 03-04-17 14:57:11, Andrey Ryabinin wrote:
> On 04/03/2017 11:47 AM, Michal Hocko wrote:
> > On Fri 31-03-17 10:00:30, Shakeel Butt wrote:
[...]
> >>> @@ -1017,9 +1018,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> >>>
> >>>         /* store */
> >>>         len = dlen + sizeof(struct zswap_header);
> >>> -       ret = zpool_malloc(entry->pool->zpool, len,
> >>> -                          __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
> >>> -                          &handle);
> >>> +       ret = zpool_malloc(entry->pool->zpool, len, gfp, &handle);
> > 
> > and here we used to do GFP_NOWAIT alternative already. What is going on
> > here?
> 
> 
> I suspect that there was no particular reason to assemble this
> custom set of gfp flags.  This code probably should have been using
> GFP_NOWAIT|__GFP_NOWARN from the very beginning.

Or just use GFP_KERNEL with a comment that this is called from the
reclaim context and as such is properly addressed at the page allocator
layer. One reason why this makes more sense than GFP_NOWAIT is that
this is easier to follow. When you see GFP_NOWAIT then you usually
expect a best efford opportunistic allocation attempt (especially with
__GFP_NOWARN) which is not the case here because this paths gets a full
memory reserves access. If this is not intentional then use GFP_NOWAIT |
__GFP_NOMEMALLOC | __GFP_NOWARN.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1615177 — Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()

On Mon 03-04-17 15:37:07, Andrey Ryabinin wrote:
> 
> 
> On 04/03/2017 11:47 AM, Michal Hocko wrote:
> > On Fri 31-03-17 10:00:30, Shakeel Butt wrote:
> >> On Fri, Mar 31, 2017 at 8:30 AM, Andrey Ryabinin
> >> <aryabinin@virtuozzo.com> wrote:
> >>> zswap_frontswap_store() is called during memory reclaim from
> >>> __frontswap_store() from swap_writepage() from shrink_page_list().
> >>> This may happen in NOFS context, thus zswap shouldn't use __GFP_FS,
> >>> otherwise we may renter into fs code and deadlock.
> >>> zswap_frontswap_store() also shouldn't use __GFP_IO to avoid recursion
> >>> into itself.
> >>>
> >>
> >> Is it possible to enter fs code (or IO) from zswap_frontswap_store()
> >> other than recursive memory reclaim? However recursive memory reclaim
> >> is protected through PF_MEMALLOC task flag. The change seems fine but
> >> IMHO reasoning needs an update. Adding Michal for expert opinion.
> > 
> > Yes this is true. 
> 
> Actually, no. I think we have a bug in allocator which may lead to
> recursive direct reclaim.
>
> E.g. for costly order allocations (or order > 0 &&
> ac->migratetype != MIGRATE_MOVABLE) with __GFP_NOMEMALLOC
> (gfp_pfmemalloc_allowed() returns false) __alloc_pages_slowpath()
> may call __alloc_pages_direct_compact() and unconditionally clear
> PF_MEMALLOC:

Not sure what is the bug here. __GFP_NOMEMALLOC is supposed to inhibit
PF_MEMALLOC. And we do not recurse to the reclaim path. We only do the
compaction. Or what am I missing?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1615195 — Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()

On Mon 03-04-17 16:14:51, Andrey Ryabinin wrote:
> 
> 
> On 04/03/2017 03:45 PM, Michal Hocko wrote:
> > On Mon 03-04-17 15:37:07, Andrey Ryabinin wrote:
> >>
> >>
> >> On 04/03/2017 11:47 AM, Michal Hocko wrote:
> >>> On Fri 31-03-17 10:00:30, Shakeel Butt wrote:
> >>>> On Fri, Mar 31, 2017 at 8:30 AM, Andrey Ryabinin
> >>>> <aryabinin@virtuozzo.com> wrote:
> >>>>> zswap_frontswap_store() is called during memory reclaim from
> >>>>> __frontswap_store() from swap_writepage() from shrink_page_list().
> >>>>> This may happen in NOFS context, thus zswap shouldn't use __GFP_FS,
> >>>>> otherwise we may renter into fs code and deadlock.
> >>>>> zswap_frontswap_store() also shouldn't use __GFP_IO to avoid recursion
> >>>>> into itself.
> >>>>>
> >>>>
> >>>> Is it possible to enter fs code (or IO) from zswap_frontswap_store()
> >>>> other than recursive memory reclaim? However recursive memory reclaim
> >>>> is protected through PF_MEMALLOC task flag. The change seems fine but
> >>>> IMHO reasoning needs an update. Adding Michal for expert opinion.
> >>>
> >>> Yes this is true. 
> >>
> >> Actually, no. I think we have a bug in allocator which may lead to
> >> recursive direct reclaim.
> >>
> >> E.g. for costly order allocations (or order > 0 &&
> >> ac->migratetype != MIGRATE_MOVABLE) with __GFP_NOMEMALLOC
> >> (gfp_pfmemalloc_allowed() returns false) __alloc_pages_slowpath()
> >> may call __alloc_pages_direct_compact() and unconditionally clear
> >> PF_MEMALLOC:
> > 
> > Not sure what is the bug here. __GFP_NOMEMALLOC is supposed to inhibit
> > PF_MEMALLOC. And we do not recurse to the reclaim path. We only do the
> > compaction. Or what am I missing?
> > 
> 
> The bug here is that __alloc_pages_direct_compact() will
> *unconditionally* clear PF_MEMALLOC.  So if we already
> under direct reclaim (so PF_MEMALLOC was already set)
> __alloc_pages_direct_compact() will clear that PF_MEMALLOC. If
> compaction failed we may go into direct reclaim again because the
> following following if in __alloc_pages_slowpath() is false:

Ohh, I see what you mean. Yes this is true but I guess we do not
have any real costly order __GFP_NOMEMALLOC users (not sure about
MIGRATE_MOVABLE branch) so nobody has noticed this.  Still worth fixing
I guess. I already have a plan to change direct PF_MEMALLOC to use
memalloc_noreclaim_{save,restore} API on my todo list. Just didn't get
to it yet. Care to send a patch?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1615198 — Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()

On 04/03/2017 02:38 PM, Andrey Ryabinin wrote:
> 
> 
> On 04/03/2017 03:37 PM, Andrey Ryabinin wrote:
>>
>>
>> On 04/03/2017 11:47 AM, Michal Hocko wrote:
>>> On Fri 31-03-17 10:00:30, Shakeel Butt wrote:
>>>> On Fri, Mar 31, 2017 at 8:30 AM, Andrey Ryabinin
>>>> <aryabinin@virtuozzo.com> wrote:
>>>>> zswap_frontswap_store() is called during memory reclaim from
>>>>> __frontswap_store() from swap_writepage() from shrink_page_list().
>>>>> This may happen in NOFS context, thus zswap shouldn't use __GFP_FS,
>>>>> otherwise we may renter into fs code and deadlock.
>>>>> zswap_frontswap_store() also shouldn't use __GFP_IO to avoid recursion
>>>>> into itself.
>>>>>
>>>>
>>>> Is it possible to enter fs code (or IO) from zswap_frontswap_store()
>>>> other than recursive memory reclaim? However recursive memory reclaim
>>>> is protected through PF_MEMALLOC task flag. The change seems fine but
>>>> IMHO reasoning needs an update. Adding Michal for expert opinion.
>>>
>>> Yes this is true. 
>>
>> Actually, no. I think we have a bug in allocator which may lead to recursive direct reclaim.
>>
>> E.g. for costly order allocations (or order > 0 && ac->migratetype != MIGRATE_MOVABLE)
>> with __GFP_NOMEMALLOC (gfp_pfmemalloc_allowed() returns false)
>> __alloc_pages_slowpath() may call __alloc_pages_direct_compact() and unconditionally clear PF_MEMALLOC:
>>
>> __alloc_pages_direct_compact():
>> ...
>> 	current->flags |= PF_MEMALLOC;
>> 	*compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
>> 									prio);
>> 	current->flags &= ~PF_MEMALLOC;
>>
>>
>>
>> And later in __alloc_pages_slowpath():
>>
>> 	/* Avoid recursion of direct reclaim */
>> 	if (current->flags & PF_MEMALLOC)        <=== false
>> 		goto nopage;
>>
>> 	/* Try direct reclaim and then allocating */
>> 	page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
>> 							&did_some_progress);
>>
> 
> 
> Seems it was broken by
> 
> a8161d1ed6098506303c65b3701dedba876df42a
> Author: Vlastimil Babka <vbabka@suse.cz>
> Date:   Thu Jul 28 15:49:19 2016 -0700
> 
>     mm, page_alloc: restructure direct compaction handling in slowpath

Yeah, looks like previously the code subtly relied on compaction being
called only after the PF_MEMALLOC -> goto nopage check and I didn't
notice it. Tell me if I should add a check or you plan to send a patch.
Thanks!

> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web