* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 8:47 ` Michal Hocko
0 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2017-04-03 8:47 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrey Ryabinin, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 8:47 ` Michal Hocko
@ 2017-04-03 11:57 ` Andrey Ryabinin
-1 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 11:57 UTC (permalink / raw)
To: Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton
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.
Indeed, I missed that detail.
> 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.
>
zswap_frontswap_store() is called only from swap_writepage().
Given that swap_writepage() is called only during reclaim or swapoff
shouldn't be a real problem.
> 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?
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.
>>> if (ret == -ENOSPC) {
>>> zswap_reject_compress_poor++;
>>> goto put_dstmem;
>>> --
>>> 2.10.2
>>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 11:57 ` Andrey Ryabinin
0 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 11:57 UTC (permalink / raw)
To: Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton
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.
Indeed, I missed that detail.
> 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.
>
zswap_frontswap_store() is called only from swap_writepage().
Given that swap_writepage() is called only during reclaim or swapoff
shouldn't be a real problem.
> 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?
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.
>>> if (ret == -ENOSPC) {
>>> zswap_reject_compress_poor++;
>>> goto put_dstmem;
>>> --
>>> 2.10.2
>>>
>
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 11:57 ` Andrey Ryabinin
@ 2017-04-03 12:29 ` Michal Hocko
-1 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2017-04-03 12:29 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 12:29 ` Michal Hocko
0 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2017-04-03 12:29 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 8:47 ` Michal Hocko
@ 2017-04-03 12:37 ` Andrey Ryabinin
-1 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 12:37 UTC (permalink / raw)
To: Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton
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);
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 12:37 ` Andrey Ryabinin
0 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 12:37 UTC (permalink / raw)
To: Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton
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);
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 12:37 ` Andrey Ryabinin
@ 2017-04-03 12:38 ` Andrey Ryabinin
-1 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 12:38 UTC (permalink / raw)
To: Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton,
Vlastimil Babka
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 12:38 ` Andrey Ryabinin
0 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 12:38 UTC (permalink / raw)
To: Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton,
Vlastimil Babka
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
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 12:38 ` Andrey Ryabinin
@ 2017-04-03 13:28 ` Vlastimil Babka
-1 siblings, 0 replies; 24+ messages in thread
From: Vlastimil Babka @ 2017-04-03 13:28 UTC (permalink / raw)
To: Andrey Ryabinin, Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton
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>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 13:28 ` Vlastimil Babka
0 siblings, 0 replies; 24+ messages in thread
From: Vlastimil Babka @ 2017-04-03 13:28 UTC (permalink / raw)
To: Andrey Ryabinin, Michal Hocko, Shakeel Butt
Cc: Seth Jennings, Dan Streetman, Linux MM, LKML, Andrew Morton
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>
>
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 13:28 ` Vlastimil Babka
@ 2017-04-03 13:46 ` Andrey Ryabinin
-1 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 13:46 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Michal Hocko, Shakeel Butt, Seth Jennings, Dan Streetman,
Linux MM, LKML, Andrew Morton
On 04/03/2017 04:28 PM, Vlastimil Babka wrote:
>>
>>
>> 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!
I would be glad if you could take care of this.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 13:46 ` Andrey Ryabinin
0 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 13:46 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Michal Hocko, Shakeel Butt, Seth Jennings, Dan Streetman,
Linux MM, LKML, Andrew Morton
On 04/03/2017 04:28 PM, Vlastimil Babka wrote:
>>
>>
>> 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!
I would be glad if you could take care of this.
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 12:37 ` Andrey Ryabinin
@ 2017-04-03 12:45 ` Michal Hocko
-1 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2017-04-03 12:45 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 12:45 ` Michal Hocko
0 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2017-04-03 12:45 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 12:45 ` Michal Hocko
@ 2017-04-03 13:14 ` Andrey Ryabinin
-1 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 13:14 UTC (permalink / raw)
To: Michal Hocko
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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:
/* Avoid recursion of direct reclaim */
if (current->flags & PF_MEMALLOC)
goto nopage;
/* Try direct reclaim and then allocating */
page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
So, recursion might look like this:
alloc_pages()
__perform_reclaim()
current->flags |= PF_MEMALLOC;
try_to_free_pages()
alloc_pages(__GFP_NONMEMALLOC):
__alloc_pages_direct_compact():
current->flags &= ~PF_MEMALLOC;
if (current->flags & PF_MEMALLOC) //now it's false
goto nopage;
__alloc_pages_direct_reclaim()
__perform_reclaim()
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 13:14 ` Andrey Ryabinin
0 siblings, 0 replies; 24+ messages in thread
From: Andrey Ryabinin @ 2017-04-03 13:14 UTC (permalink / raw)
To: Michal Hocko
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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:
/* Avoid recursion of direct reclaim */
if (current->flags & PF_MEMALLOC)
goto nopage;
/* Try direct reclaim and then allocating */
page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
So, recursion might look like this:
alloc_pages()
__perform_reclaim()
current->flags |= PF_MEMALLOC;
try_to_free_pages()
alloc_pages(__GFP_NONMEMALLOC):
__alloc_pages_direct_compact():
current->flags &= ~PF_MEMALLOC;
if (current->flags & PF_MEMALLOC) //now it's false
goto nopage;
__alloc_pages_direct_reclaim()
__perform_reclaim()
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
2017-04-03 13:14 ` Andrey Ryabinin
@ 2017-04-03 13:23 ` Michal Hocko
-1 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2017-04-03 13:23 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] mm/zswap: fix potential deadlock in zswap_frontswap_store()
@ 2017-04-03 13:23 ` Michal Hocko
0 siblings, 0 replies; 24+ messages in thread
From: Michal Hocko @ 2017-04-03 13:23 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Shakeel Butt, Seth Jennings, Dan Streetman, Linux MM, LKML,
Andrew Morton
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
--
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>
^ permalink raw reply [flat|nested] 24+ messages in thread