* Re: [PATCH] mm: ensure alloc_flags in slow path are initialized
2017-01-23 12:16 [PATCH] mm: ensure alloc_flags in slow path are initialized Arnd Bergmann
@ 2017-01-23 12:55 ` Vlastimil Babka
2017-01-23 16:02 ` Arnd Bergmann
2017-01-23 22:54 ` David Rientjes
2017-01-23 23:56 ` Andrew Morton
2 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2017-01-23 12:55 UTC (permalink / raw)
To: Arnd Bergmann, Andrew Morton
Cc: Mel Gorman, Michal Hocko, Johannes Weiner, Joonsoo Kim, linux-mm,
linux-kernel
On 01/23/2017 01:16 PM, Arnd Bergmann wrote:
> The __alloc_pages_slowpath() has gotten rather complex and gcc
> is no longer able to follow the gotos and prove that the
> alloc_flags variable is initialized at the time it is used:
>
> mm/page_alloc.c: In function '__alloc_pages_slowpath':
> mm/page_alloc.c:3565:15: error: 'alloc_flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> To be honest, I can't figure that out either, maybe it is or
> maybe not,
Seems the report is correct and not false positive, in scenario when we goto
nopage before the assignment, and then goto retry because of __GFP_NOFAIL.
> but moving the existing initialization up a little
> higher looks safe and makes it obvious to both me and gcc that
> the initialization comes before the first use.
>
> Fixes: 74eaa4a97e8e ("mm: consolidate GFP_NOFAIL checks in the allocator slowpath")
That's a non-stable -next commit ID for mmotm patch:
mm-consolidate-gfp_nofail-checks-in-the-allocator-slowpath.patch
The patch itself was OK, the problem only comes from integration with another
mmotm patch (also independently OK):
mm-page_alloc-fix-premature-oom-when-racing-with-cpuset-mems-update.patch
By their ordering in mmotm, it would work to treat this as a fix for the
GFP_NOFAIL patch, possibly merged into it.
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> mm/page_alloc.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index cf641932c015..d9fa4564524f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3591,6 +3591,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> (__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)))
> gfp_mask &= ~__GFP_ATOMIC;
>
> + /*
> + * The fast path uses conservative alloc_flags to succeed only until
> + * kswapd needs to be woken up, and to avoid the cost of setting up
> + * alloc_flags precisely. So we do that now.
> + */
> + alloc_flags = gfp_to_alloc_flags(gfp_mask);
> +
> retry_cpuset:
> compaction_retries = 0;
> no_progress_loops = 0;
> @@ -3607,14 +3614,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> if (!ac->preferred_zoneref->zone)
> goto nopage;
>
> -
> - /*
> - * The fast path uses conservative alloc_flags to succeed only until
> - * kswapd needs to be woken up, and to avoid the cost of setting up
> - * alloc_flags precisely. So we do that now.
> - */
> - alloc_flags = gfp_to_alloc_flags(gfp_mask);
> -
> if (gfp_mask & __GFP_KSWAPD_RECLAIM)
> wake_all_kswapds(order, ac);
>
>
--
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] 6+ messages in thread* Re: [PATCH] mm: ensure alloc_flags in slow path are initialized
2017-01-23 12:55 ` Vlastimil Babka
@ 2017-01-23 16:02 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-01-23 16:02 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Mel Gorman, Michal Hocko, Johannes Weiner,
Joonsoo Kim, linux-mm, Linux Kernel Mailing List
On Mon, Jan 23, 2017 at 1:55 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> On 01/23/2017 01:16 PM, Arnd Bergmann wrote:
>> To be honest, I can't figure that out either, maybe it is or
>> maybe not,
>
>
> Seems the report is correct and not false positive, in scenario when we goto
> nopage before the assignment, and then goto retry because of __GFP_NOFAIL.
Ok, thanks for checking!
>> but moving the existing initialization up a little
>> higher looks safe and makes it obvious to both me and gcc that
>> the initialization comes before the first use.
>>
>> Fixes: 74eaa4a97e8e ("mm: consolidate GFP_NOFAIL checks in the allocator
>> slowpath")
>
>
> That's a non-stable -next commit ID for mmotm patch:
> mm-consolidate-gfp_nofail-checks-in-the-allocator-slowpath.patch
>
> The patch itself was OK, the problem only comes from integration with
> another mmotm patch (also independently OK):
> mm-page_alloc-fix-premature-oom-when-racing-with-cpuset-mems-update.patch
>
> By their ordering in mmotm, it would work to treat this as a fix for the
> GFP_NOFAIL patch, possibly merged into it.
Ok. I only tracked down which commit introduced the warning, which was
the one above.
Arnd
--
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] 6+ messages in thread
* Re: [PATCH] mm: ensure alloc_flags in slow path are initialized
2017-01-23 12:16 [PATCH] mm: ensure alloc_flags in slow path are initialized Arnd Bergmann
2017-01-23 12:55 ` Vlastimil Babka
@ 2017-01-23 22:54 ` David Rientjes
2017-01-23 23:56 ` Andrew Morton
2 siblings, 0 replies; 6+ messages in thread
From: David Rientjes @ 2017-01-23 22:54 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andrew Morton, Vlastimil Babka, Mel Gorman, Michal Hocko,
Johannes Weiner, Joonsoo Kim, linux-mm, linux-kernel
On Mon, 23 Jan 2017, Arnd Bergmann wrote:
> The __alloc_pages_slowpath() has gotten rather complex and gcc
> is no longer able to follow the gotos and prove that the
> alloc_flags variable is initialized at the time it is used:
>
> mm/page_alloc.c: In function '__alloc_pages_slowpath':
> mm/page_alloc.c:3565:15: error: 'alloc_flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> To be honest, I can't figure that out either, maybe it is or
> maybe not, but moving the existing initialization up a little
> higher looks safe and makes it obvious to both me and gcc that
> the initialization comes before the first use.
>
> Fixes: 74eaa4a97e8e ("mm: consolidate GFP_NOFAIL checks in the allocator slowpath")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: David Rientjes <rientjes@google.com>
--
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] 6+ messages in thread* Re: [PATCH] mm: ensure alloc_flags in slow path are initialized
2017-01-23 12:16 [PATCH] mm: ensure alloc_flags in slow path are initialized Arnd Bergmann
2017-01-23 12:55 ` Vlastimil Babka
2017-01-23 22:54 ` David Rientjes
@ 2017-01-23 23:56 ` Andrew Morton
2017-01-24 9:19 ` Vlastimil Babka
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2017-01-23 23:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vlastimil Babka, Mel Gorman, Michal Hocko, Johannes Weiner,
Joonsoo Kim, linux-mm, linux-kernel
On Mon, 23 Jan 2017 13:16:12 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
> The __alloc_pages_slowpath() has gotten rather complex and gcc
> is no longer able to follow the gotos and prove that the
> alloc_flags variable is initialized at the time it is used:
>
> mm/page_alloc.c: In function '__alloc_pages_slowpath':
> mm/page_alloc.c:3565:15: error: 'alloc_flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> To be honest, I can't figure that out either, maybe it is or
> maybe not, but moving the existing initialization up a little
> higher looks safe and makes it obvious to both me and gcc that
> the initialization comes before the first use.
>
> ...
>
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3591,6 +3591,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> (__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)))
> gfp_mask &= ~__GFP_ATOMIC;
>
> + /*
> + * The fast path uses conservative alloc_flags to succeed only until
> + * kswapd needs to be woken up, and to avoid the cost of setting up
> + * alloc_flags precisely. So we do that now.
> + */
> + alloc_flags = gfp_to_alloc_flags(gfp_mask);
> +
> retry_cpuset:
> compaction_retries = 0;
> no_progress_loops = 0;
> @@ -3607,14 +3614,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> if (!ac->preferred_zoneref->zone)
> goto nopage;
>
> -
> - /*
> - * The fast path uses conservative alloc_flags to succeed only until
> - * kswapd needs to be woken up, and to avoid the cost of setting up
> - * alloc_flags precisely. So we do that now.
> - */
> - alloc_flags = gfp_to_alloc_flags(gfp_mask);
> -
> if (gfp_mask & __GFP_KSWAPD_RECLAIM)
> wake_all_kswapds(order, ac);
hm. But we later do
if (gfp_pfmemalloc_allowed(gfp_mask))
alloc_flags = ALLOC_NO_WATERMARKS;
...
if (read_mems_allowed_retry(cpuset_mems_cookie))
goto retry_cpuset;
so with your patch there's a path where we can rerun everything with
alloc_flags == ALLOC_NO_WATERMARKS. That's changed behaviour.
When I saw the test robot warning I did this, which I think preserves
behaviour?
--- a/mm/page_alloc.c~mm-consolidate-gfp_nofail-checks-in-the-allocator-slowpath-fix
+++ a/mm/page_alloc.c
@@ -3577,6 +3577,14 @@ retry_cpuset:
no_progress_loops = 0;
compact_priority = DEF_COMPACT_PRIORITY;
cpuset_mems_cookie = read_mems_allowed_begin();
+
+ /*
+ * The fast path uses conservative alloc_flags to succeed only until
+ * kswapd needs to be woken up, and to avoid the cost of setting up
+ * alloc_flags precisely. So we do that now.
+ */
+ alloc_flags = gfp_to_alloc_flags(gfp_mask);
+
/*
* We need to recalculate the starting point for the zonelist iterator
* because we might have used different nodemask in the fast path, or
@@ -3588,14 +3596,6 @@ retry_cpuset:
if (!ac->preferred_zoneref->zone)
goto nopage;
-
- /*
- * The fast path uses conservative alloc_flags to succeed only until
- * kswapd needs to be woken up, and to avoid the cost of setting up
- * alloc_flags precisely. So we do that now.
- */
- alloc_flags = gfp_to_alloc_flags(gfp_mask);
-
if (gfp_mask & __GFP_KSWAPD_RECLAIM)
wake_all_kswapds(order, ac);
_
--
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] 6+ messages in thread
* Re: [PATCH] mm: ensure alloc_flags in slow path are initialized
2017-01-23 23:56 ` Andrew Morton
@ 2017-01-24 9:19 ` Vlastimil Babka
0 siblings, 0 replies; 6+ messages in thread
From: Vlastimil Babka @ 2017-01-24 9:19 UTC (permalink / raw)
To: Andrew Morton, Arnd Bergmann
Cc: Mel Gorman, Michal Hocko, Johannes Weiner, Joonsoo Kim, linux-mm,
linux-kernel
On 01/24/2017 12:56 AM, Andrew Morton wrote:
> On Mon, 23 Jan 2017 13:16:12 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
>
>> The __alloc_pages_slowpath() has gotten rather complex and gcc
>> is no longer able to follow the gotos and prove that the
>> alloc_flags variable is initialized at the time it is used:
>>
>> mm/page_alloc.c: In function '__alloc_pages_slowpath':
>> mm/page_alloc.c:3565:15: error: 'alloc_flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> To be honest, I can't figure that out either, maybe it is or
>> maybe not, but moving the existing initialization up a little
>> higher looks safe and makes it obvious to both me and gcc that
>> the initialization comes before the first use.
>>
>> ...
>>
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3591,6 +3591,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>> (__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)))
>> gfp_mask &= ~__GFP_ATOMIC;
>>
>> + /*
>> + * The fast path uses conservative alloc_flags to succeed only until
>> + * kswapd needs to be woken up, and to avoid the cost of setting up
>> + * alloc_flags precisely. So we do that now.
>> + */
>> + alloc_flags = gfp_to_alloc_flags(gfp_mask);
>> +
>> retry_cpuset:
>> compaction_retries = 0;
>> no_progress_loops = 0;
>> @@ -3607,14 +3614,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>> if (!ac->preferred_zoneref->zone)
>> goto nopage;
>>
>> -
>> - /*
>> - * The fast path uses conservative alloc_flags to succeed only until
>> - * kswapd needs to be woken up, and to avoid the cost of setting up
>> - * alloc_flags precisely. So we do that now.
>> - */
>> - alloc_flags = gfp_to_alloc_flags(gfp_mask);
>> -
>> if (gfp_mask & __GFP_KSWAPD_RECLAIM)
>> wake_all_kswapds(order, ac);
>
> hm. But we later do
>
> if (gfp_pfmemalloc_allowed(gfp_mask))
> alloc_flags = ALLOC_NO_WATERMARKS;
>
> ...
> if (read_mems_allowed_retry(cpuset_mems_cookie))
> goto retry_cpuset;
>
> so with your patch there's a path where we can rerun everything with
> alloc_flags == ALLOC_NO_WATERMARKS. That's changed behaviour.
Right.
> When I saw the test robot warning I did this, which I think preserves
> behaviour?
Yes, that's cleaner. Thanks.
> --- a/mm/page_alloc.c~mm-consolidate-gfp_nofail-checks-in-the-allocator-slowpath-fix
> +++ a/mm/page_alloc.c
> @@ -3577,6 +3577,14 @@ retry_cpuset:
> no_progress_loops = 0;
> compact_priority = DEF_COMPACT_PRIORITY;
> cpuset_mems_cookie = read_mems_allowed_begin();
> +
> + /*
> + * The fast path uses conservative alloc_flags to succeed only until
> + * kswapd needs to be woken up, and to avoid the cost of setting up
> + * alloc_flags precisely. So we do that now.
> + */
> + alloc_flags = gfp_to_alloc_flags(gfp_mask);
> +
> /*
> * We need to recalculate the starting point for the zonelist iterator
> * because we might have used different nodemask in the fast path, or
> @@ -3588,14 +3596,6 @@ retry_cpuset:
> if (!ac->preferred_zoneref->zone)
> goto nopage;
>
> -
> - /*
> - * The fast path uses conservative alloc_flags to succeed only until
> - * kswapd needs to be woken up, and to avoid the cost of setting up
> - * alloc_flags precisely. So we do that now.
> - */
> - alloc_flags = gfp_to_alloc_flags(gfp_mask);
> -
> if (gfp_mask & __GFP_KSWAPD_RECLAIM)
> wake_all_kswapds(order, ac);
>
> _
>
--
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] 6+ messages in thread