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

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

[Andrey Ryabinin]

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.

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

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

[Andrey Ryabinin]

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.

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

--
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>

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

[Shakeel Butt]

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
>

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

[Shakeel Butt]

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
>

--
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>

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

[Michal Hocko]

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

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

[Michal Hocko]

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>

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

[Andrey Ryabinin]

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
>>>
> 

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

[Andrey Ryabinin]

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>

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

[Michal Hocko]

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

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

[Michal Hocko]

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>

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

[Andrey Ryabinin]

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);

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

[Andrey Ryabinin]

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>

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

[Andrey Ryabinin]

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

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

[Andrey Ryabinin]

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>

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

[Michal Hocko]

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

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

[Michal Hocko]

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>

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

[Andrey Ryabinin]

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()
					
					

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

[Andrey Ryabinin]

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>

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

[Michal Hocko]

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

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

[Michal Hocko]

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>

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

[Vlastimil Babka]

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>
> 

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

[Vlastimil Babka]

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>

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

[Andrey Ryabinin]

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.

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

[Andrey Ryabinin]

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>