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

Groups > linux.kernel > #1235554 > unrolled thread

Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for order-0 allocations

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for  order-0 allocations Andrew Morton <akpm@linux-foundation.org> - 2015-09-29 23:10 +0200
    Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for  order-0 allocations Mel Gorman <mgorman@techsingularity.net> - 2015-09-30 10:50 +0200
      Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for order-0  allocations Vlastimil Babka <vbabka@suse.cz> - 2015-09-30 16:20 +0200
        Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for  order-0 allocations Mel Gorman <mgorman@techsingularity.net> - 2015-09-30 17:20 +0200
          Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for  order-0 allocations Andrew Morton <akpm@linux-foundation.org> - 2015-09-30 22:40 +0200

#1235554 — Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for order-0 allocations

On Mon, 21 Sep 2015 13:03:17 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:

> The primary purpose of watermarks is to ensure that reclaim can always
> make forward progress in PF_MEMALLOC context (kswapd and direct reclaim).
> These assume that order-0 allocations are all that is necessary for
> forward progress.
> 
> High-order watermarks serve a different purpose. Kswapd
> had no high-order awareness before they were introduced
> (https://lkml.kernel.org/r/413AA7B2.4000907@yahoo.com.au).  This was
> particularly important when there were high-order atomic requests.
> The watermarks both gave kswapd awareness and made a reserve for those
> atomic requests.
> 
> There are two important side-effects of this. The most important is that
> a non-atomic high-order request can fail even though free pages are available
> and the order-0 watermarks are ok. The second is that high-order watermark
> checks are expensive as the free list counts up to the requested order must
> be examined.
> 
> With the introduction of MIGRATE_HIGHATOMIC it is no longer necessary to
> have high-order watermarks. Kswapd and compaction still need high-order
> awareness which is handled by checking that at least one suitable high-order
> page is free.
> 
> With the patch applied, there was little difference in the allocation
> failure rates as the atomic reserves are small relative to the number of
> allocation attempts. The expected impact is that there will never be an
> allocation failure report that shows suitable pages on the free lists.
> 
> The one potential side-effect of this is that in a vanilla kernel, the
> watermark checks may have kept a free page for an atomic allocation. Now,
> we are 100% relying on the HighAtomic reserves and an early allocation to
> have allocated them.  If the first high-order atomic allocation is after
> the system is already heavily fragmented then it'll fail.
> 
> ...
>
>  static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>  			unsigned long mark, int classzone_idx, int alloc_flags,
> @@ -2317,7 +2319,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>  {
>  	long min = mark;
>  	int o;
> -	long free_cma = 0;
> +	const bool alloc_harder = (alloc_flags & ALLOC_HARDER);

hmpf.  Setting a bool to 0x10 is a bit grubby.
  
>  	/* free_pages may go negative - that's OK */
>  	free_pages -= (1 << order) - 1;
> @@ -2330,7 +2332,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>  	 * the high-atomic reserves. This will over-estimate the size of the
>  	 * atomic reserve but it avoids a search.
>  	 */
> -	if (likely(!(alloc_flags & ALLOC_HARDER)))
> +	if (likely(!alloc_harder))
>  		free_pages -= z->nr_reserved_highatomic;
>  	else
>  		min -= min / 4;
> @@ -2338,22 +2340,43 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>  #ifdef CONFIG_CMA
>  	/* If allocation can't use CMA areas don't use free CMA pages */
>  	if (!(alloc_flags & ALLOC_CMA))
> -		free_cma = zone_page_state(z, NR_FREE_CMA_PAGES);
> +		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
>  #endif
>  
> -	if (free_pages - free_cma <= min + z->lowmem_reserve[classzone_idx])
> +	if (free_pages <= min + z->lowmem_reserve[classzone_idx])
>  		return false;
> -	for (o = 0; o < order; o++) {
> -		/* At the next order, this order's pages become unavailable */
> -		free_pages -= z->free_area[o].nr_free << o;
>  
> -		/* Require fewer higher order pages to be free */
> -		min >>= 1;
> +	/* order-0 watermarks are ok */

because?

> +	if (!order)
> +		return true;
> +
> +	/* Check at least one high-order page is free */
> +	for (o = order; o < MAX_ORDER; o++) {
> +		struct free_area *area = &z->free_area[o];
> +		int mt;
> +
> +		if (!area->nr_free)
> +			continue;
> +
> +		if (alloc_harder) {
> +			if (area->nr_free)
> +				return true;
> +			continue;
> +		}
>  
> -		if (free_pages <= min)
> -			return false;
> +		for (mt = 0; mt < MIGRATE_PCPTYPES; mt++) {
> +			if (!list_empty(&area->free_list[mt]))
> +				return true;
> +		}
> +
> +#ifdef CONFIG_CMA
> +		if ((alloc_flags & ALLOC_CMA) &&
> +		    !list_empty(&area->free_list[MIGRATE_CMA])) {
> +			return true;
> +		}
> +#endif
>  	}
> -	return true;
> +	return false;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1235901

On Tue, Sep 29, 2015 at 02:05:07PM -0700, Andrew Morton wrote:
> >  static bool __zone_watermark_ok(struct zone *z, unsigned int order,
> >  			unsigned long mark, int classzone_idx, int alloc_flags,
> > @@ -2317,7 +2319,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
> >  {
> >  	long min = mark;
> >  	int o;
> > -	long free_cma = 0;
> > +	const bool alloc_harder = (alloc_flags & ALLOC_HARDER);
> 
> hmpf.  Setting a bool to 0x10 is a bit grubby.
>   

Should be safe, but I see your point. For any other type it would be
truncated and look like a bug.

> >  	/* free_pages may go negative - that's OK */
> >  	free_pages -= (1 << order) - 1;
> > @@ -2330,7 +2332,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
> >  	 * the high-atomic reserves. This will over-estimate the size of the
> >  	 * atomic reserve but it avoids a search.
> >  	 */
> > -	if (likely(!(alloc_flags & ALLOC_HARDER)))
> > +	if (likely(!alloc_harder))
> >  		free_pages -= z->nr_reserved_highatomic;
> >  	else
> >  		min -= min / 4;
> > @@ -2338,22 +2340,43 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
> >  #ifdef CONFIG_CMA
> >  	/* If allocation can't use CMA areas don't use free CMA pages */
> >  	if (!(alloc_flags & ALLOC_CMA))
> > -		free_cma = zone_page_state(z, NR_FREE_CMA_PAGES);
> > +		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
> >  #endif
> >  
> > -	if (free_pages - free_cma <= min + z->lowmem_reserve[classzone_idx])
> > +	if (free_pages <= min + z->lowmem_reserve[classzone_idx])
> >  		return false;
> > -	for (o = 0; o < order; o++) {
> > -		/* At the next order, this order's pages become unavailable */
> > -		free_pages -= z->free_area[o].nr_free << o;
> >  
> > -		/* Require fewer higher order pages to be free */
> > -		min >>= 1;
> > +	/* order-0 watermarks are ok */
> 
> because?
> 

The wizard of oz because because!

This should fix it up better than clicking my shoes three times.

---8<---
From: Mel Gorman <mgorman@techsingularity.net>
Subject: [PATCH] mm, page_alloc: only enforce watermarks for order-0
 allocations -fix

This patch is updating comments for clarity and converts a bool to an
int. The code as-is is ok as the compiler is meant to cast it correctly
but it looks odd to people who know the value would be truncated and
lost for other types.

This is a fix to the mmotm patch
mm-page_alloc-only-enforce-watermarks-for-order-0-allocations.patch

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 25731624d734..fedec98aafca 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2332,7 +2332,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
 {
 	long min = mark;
 	int o;
-	const bool alloc_harder = (alloc_flags & ALLOC_HARDER);
+	const int alloc_harder = (alloc_flags & ALLOC_HARDER);
 
 	/* free_pages may go negative - that's OK */
 	free_pages -= (1 << order) - 1;
@@ -2356,14 +2356,19 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
 		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
 #endif
 
+	/*
+	 * Check watermarks for an order-0 allocation request. If these
+	 * are not met, then a high-order request also cannot go ahead
+	 * even if a suitable page happened to be free.
+	 */
 	if (free_pages <= min + z->lowmem_reserve[classzone_idx])
 		return false;
 
-	/* order-0 watermarks are ok */
+	/* If this is an order-0 request then the watermark is fine */
 	if (!order)
 		return true;
 
-	/* Check at least one high-order page is free */
+	/* For a high-order request, check at least one suitable page is free */
 	for (o = order; o < MAX_ORDER; o++) {
 		struct free_area *area = &z->free_area[o];
 		int mt;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1236367 — Re: [PATCH 10/10] mm, page_alloc: Only enforce watermarks for order-0 allocations

On 09/30/2015 10:46 AM, Mel Gorman wrote:
> On Tue, Sep 29, 2015 at 02:05:07PM -0700, Andrew Morton wrote:
>
> The wizard of oz because because!
>
> This should fix it up better than clicking my shoes three times.
>
> ---8<---
> From: Mel Gorman <mgorman@techsingularity.net>
> Subject: [PATCH] mm, page_alloc: only enforce watermarks for order-0
>   allocations -fix
>
> This patch is updating comments for clarity and converts a bool to an
> int. The code as-is is ok as the compiler is meant to cast it correctly
> but it looks odd to people who know the value would be truncated and
> lost for other types.
>
> This is a fix to the mmotm patch
> mm-page_alloc-only-enforce-watermarks-for-order-0-allocations.patch
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Ack (with nitpick below)

> ---
>   mm/page_alloc.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 25731624d734..fedec98aafca 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2332,7 +2332,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>   {
>   	long min = mark;
>   	int o;
> -	const bool alloc_harder = (alloc_flags & ALLOC_HARDER);
> +	const int alloc_harder = (alloc_flags & ALLOC_HARDER);

How bout the !!(alloc_flags & ALLOC_HARDER) conversion to bool? Unless 
it forces to make the compiler some extra work...

>
>   	/* free_pages may go negative - that's OK */
>   	free_pages -= (1 << order) - 1;
> @@ -2356,14 +2356,19 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>   		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
>   #endif
>
> +	/*
> +	 * Check watermarks for an order-0 allocation request. If these
> +	 * are not met, then a high-order request also cannot go ahead
> +	 * even if a suitable page happened to be free.
> +	 */
>   	if (free_pages <= min + z->lowmem_reserve[classzone_idx])
>   		return false;
>
> -	/* order-0 watermarks are ok */
> +	/* If this is an order-0 request then the watermark is fine */
>   	if (!order)
>   		return true;
>
> -	/* Check at least one high-order page is free */
> +	/* For a high-order request, check at least one suitable page is free */
>   	for (o = order; o < MAX_ORDER; o++) {
>   		struct free_area *area = &z->free_area[o];
>   		int mt;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1236432

On Wed, Sep 30, 2015 at 04:17:44PM +0200, Vlastimil Babka wrote:
> >---
> >  mm/page_alloc.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> >diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >index 25731624d734..fedec98aafca 100644
> >--- a/mm/page_alloc.c
> >+++ b/mm/page_alloc.c
> >@@ -2332,7 +2332,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
> >  {
> >  	long min = mark;
> >  	int o;
> >-	const bool alloc_harder = (alloc_flags & ALLOC_HARDER);
> >+	const int alloc_harder = (alloc_flags & ALLOC_HARDER);
> 
> How bout the !!(alloc_flags & ALLOC_HARDER) conversion to bool? Unless it
> forces to make the compiler some extra work...
> 

Some people frown upon that trick as being obscure when it's not unnecessary
and a modern compiler is meant to get it right. The int is clear and
obvious in this context so I just went with it.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1236740

On Wed, 30 Sep 2015 16:12:34 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:

> On Wed, Sep 30, 2015 at 04:17:44PM +0200, Vlastimil Babka wrote:
> > >---
> > >  mm/page_alloc.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > >index 25731624d734..fedec98aafca 100644
> > >--- a/mm/page_alloc.c
> > >+++ b/mm/page_alloc.c
> > >@@ -2332,7 +2332,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
> > >  {
> > >  	long min = mark;
> > >  	int o;
> > >-	const bool alloc_harder = (alloc_flags & ALLOC_HARDER);
> > >+	const int alloc_harder = (alloc_flags & ALLOC_HARDER);
> > 
> > How bout the !!(alloc_flags & ALLOC_HARDER) conversion to bool? Unless it
> > forces to make the compiler some extra work...
> > 
> 
> Some people frown upon that trick as being obscure when it's not unnecessary
> and a modern compiler is meant to get it right. The int is clear and
> obvious in this context so I just went with it.

Yes, the !!  does generate extra code.  It doesn't seem worthwhile
overhead for a tiny cosmetic thing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web