[PATCH -mm] remove __GFP_NO_KSWAPD

[PATCH -mm] remove __GFP_NO_KSWAPD

[Rik van Riel]

When transparent huge pages were introduced, memory compaction and
swap storms were an issue, and the kernel had to be careful to not
make THP allocations cause pageout or compaction.

Now that we have working compaction deferral, kswapd is smart enough
to invoke compaction and the quadratic behaviour around isolate_free_pages
has been fixed, it should be safe to remove __GFP_NO_KSWAPD.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
This has been running fine on my system for a while, but my system
only has 12GB and moderate memory pressure. I propose we keep this
in -mm and -next for a while, and merge it for 3.7 if nobody complains.

 include/linux/gfp.h |    5 +----
 mm/page_alloc.c     |    7 +++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 581e74b..4f3d4d2 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -34,7 +34,6 @@ struct vm_area_struct;
 #else
 #define ___GFP_NOTRACK		0
 #endif
-#define ___GFP_NO_KSWAPD	0x400000u
 #define ___GFP_OTHER_NODE	0x800000u
 #define ___GFP_WRITE		0x1000000u
 
@@ -84,7 +83,6 @@ struct vm_area_struct;
 #define __GFP_RECLAIMABLE ((__force gfp_t)___GFP_RECLAIMABLE) /* Page is reclaimable */
 #define __GFP_NOTRACK	((__force gfp_t)___GFP_NOTRACK)  /* Don't track with kmemcheck */
 
-#define __GFP_NO_KSWAPD	((__force gfp_t)___GFP_NO_KSWAPD)
 #define __GFP_OTHER_NODE ((__force gfp_t)___GFP_OTHER_NODE) /* On behalf of other node */
 #define __GFP_WRITE	((__force gfp_t)___GFP_WRITE)	/* Allocator intends to dirty page */
 
@@ -114,8 +112,7 @@ struct vm_area_struct;
 				 __GFP_MOVABLE)
 #define GFP_IOFS	(__GFP_IO | __GFP_FS)
 #define GFP_TRANSHUGE	(GFP_HIGHUSER_MOVABLE | __GFP_COMP | \
-			 __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN | \
-			 __GFP_NO_KSWAPD)
+			 __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN)
 
 #ifdef CONFIG_NUMA
 #define GFP_THISNODE	(__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a13ded1..87b7721 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2217,9 +2217,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		goto nopage;
 
 restart:
-	if (!(gfp_mask & __GFP_NO_KSWAPD))
-		wake_all_kswapd(order, zonelist, high_zoneidx,
-						zone_idx(preferred_zone));
+	wake_all_kswapd(order, zonelist, high_zoneidx,
+					zone_idx(preferred_zone));
 
 	/*
 	 * OK, we're below the kswapd watermark and have kicked background
@@ -2286,7 +2285,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	 * has requested the system not be heavily disrupted, fail the
 	 * allocation now instead of entering direct reclaim
 	 */
-	if (deferred_compaction && (gfp_mask & __GFP_NO_KSWAPD))
+	if (deferred_compaction)
 		goto nopage;
 
 	/* Try direct reclaim and then allocating */

--
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] remove __GFP_NO_KSWAPD

[Minchan Kim]

Hi Rik,

On Tue, Jul 24, 2012 at 11:12:22AM -0400, Rik van Riel wrote:
> When transparent huge pages were introduced, memory compaction and
> swap storms were an issue, and the kernel had to be careful to not
> make THP allocations cause pageout or compaction.
> 
> Now that we have working compaction deferral, kswapd is smart enough
> to invoke compaction and the quadratic behaviour around isolate_free_pages
> has been fixed, it should be safe to remove __GFP_NO_KSWAPD.

Could you point out specific patches you mentiond which makes kswapd/compaction
smart? It will make description very clear.

> 
> Signed-off-by: Rik van Riel <riel@redhat.com>

I support it because I had a concern about that flags which is likely to be
used by other subsystems without careful thinking when the flag was introduced.
It's proved by mtd_kmalloc_up_to which was merged with sneaking without catching
from mm guys's eyes. When I read comment of that function, it seems to be proper
usage but I don't like it because it requries users of mm to know mm internal
like kswapd. So it should be avoided if possible.

Plus, it means you need to fix it with show_gfp_flags. :)


> ---
> This has been running fine on my system for a while, but my system
> only has 12GB and moderate memory pressure. I propose we keep this
> in -mm and -next for a while, and merge it for 3.7 if nobody complains.

Yes. it should be very careful.
I guess Mel and Andrea would have opinions and benchmark.

-- 
Kind regards,
Minchan Kim

--
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] remove __GFP_NO_KSWAPD

[Rik van Riel]

On 07/24/2012 07:34 PM, Minchan Kim wrote:
> Hi Rik,
>
> On Tue, Jul 24, 2012 at 11:12:22AM -0400, Rik van Riel wrote:
>> When transparent huge pages were introduced, memory compaction and
>> swap storms were an issue, and the kernel had to be careful to not
>> make THP allocations cause pageout or compaction.
>>
>> Now that we have working compaction deferral, kswapd is smart enough
>> to invoke compaction and the quadratic behaviour around isolate_free_pages
>> has been fixed, it should be safe to remove __GFP_NO_KSWAPD.
>
> Could you point out specific patches you mentiond which makes kswapd/compaction
> smart? It will make description very clear.

That could be a list of 50+ patches, merged over the
last two or so years.

In other words, such a large amount of data that it
is unlikely to clarify the discussion...

>> Signed-off-by: Rik van Riel <riel@redhat.com>
>
> I support it because I had a concern about that flags which is likely to be
> used by other subsystems without careful thinking when the flag was introduced.
> It's proved by mtd_kmalloc_up_to which was merged with sneaking without catching
> from mm guys's eyes. When I read comment of that function, it seems to be proper
> usage but I don't like it because it requries users of mm to know mm internal
> like kswapd. So it should be avoided if possible.
>
> Plus, it means you need to fix it with show_gfp_flags. :)

Ohh, a place I forgot to grep!

I'll send in an incremental patch right now.

>> ---
>> This has been running fine on my system for a while, but my system
>> only has 12GB and moderate memory pressure. I propose we keep this
>> in -mm and -next for a while, and merge it for 3.7 if nobody complains.
>
> Yes. it should be very careful.
> I guess Mel and Andrea would have opinions and benchmark.

It's not as much benchmarks that I am worried about,
but somebody running something unexpected on their
system.


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

[PATCH -mm] remove __GFP_NO_KSWAPD fixes

[Rik van Riel]

Turns out I missed two spots where __GFP_NO_KSWAPD is used.

The removal from the trace code is obvious, since the flag
got removed there is no need to print it.

For mtdcore.c, now that memory compaction has been fixed,
we should no longer see large swap storms from an attempt
to allocate a large buffer, removing the need to specify
__GFP_NO_KSWAPD.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 drivers/mtd/mtdcore.c           |    3 +--
 include/trace/events/gfpflags.h |    1 -
 2 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 9a9ce71..af1e932 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -761,8 +761,7 @@ EXPORT_SYMBOL_GPL(mtd_writev);
  */
 void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size)
 {
-	gfp_t flags = __GFP_NOWARN | __GFP_WAIT |
-		       __GFP_NORETRY | __GFP_NO_KSWAPD;
+	gfp_t flags = __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY;
 	size_t min_alloc = max_t(size_t, mtd->writesize, PAGE_SIZE);
 	void *kbuf;
 
diff --git a/include/trace/events/gfpflags.h b/include/trace/events/gfpflags.h
index 9fe3a36..8ffc050 100644
--- a/include/trace/events/gfpflags.h
+++ b/include/trace/events/gfpflags.h
@@ -35,7 +35,6 @@
 	{(unsigned long)__GFP_RECLAIMABLE,	"GFP_RECLAIMABLE"},	\
 	{(unsigned long)__GFP_MOVABLE,		"GFP_MOVABLE"},		\
 	{(unsigned long)__GFP_NOTRACK,		"GFP_NOTRACK"},		\
-	{(unsigned long)__GFP_NO_KSWAPD,	"GFP_NO_KSWAPD"},	\
 	{(unsigned long)__GFP_OTHER_NODE,	"GFP_OTHER_NODE"}	\
 	) : "GFP_NOWAIT"
 

--
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] remove __GFP_NO_KSWAPD fixes

[Minchan Kim]

On Wed, Jul 25, 2012 at 02:51:19PM -0400, Rik van Riel wrote:
> Turns out I missed two spots where __GFP_NO_KSWAPD is used.
> 
> The removal from the trace code is obvious, since the flag
> got removed there is no need to print it.
> 
> For mtdcore.c, now that memory compaction has been fixed,
> we should no longer see large swap storms from an attempt
> to allocate a large buffer, removing the need to specify
> __GFP_NO_KSWAPD.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
Reviewed-by: Minchan Kim <minchan@kernel.org>

You should have tidied up comment of the function.
I hope Andrew can do it if he see this review.

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index fcfce24..6ff1308 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1065,8 +1065,7 @@ EXPORT_SYMBOL_GPL(mtd_writev);
  * until the request succeeds or until the allocation size falls below
  * the system page size. This attempts to make sure it does not adversely
  * impact system performance, so when allocating more than one page, we
- * ask the memory allocator to avoid re-trying, swapping, writing back
- * or performing I/O.
+ * ask the memory allocator to avoid re-trying.
  *
  * Note, this function also makes sure that the allocated buffer is aligned to
  * the MTD device's min. I/O unit, i.e. the "mtd->writesize" value.

Thanks.

-- 
Kind regards,
Minchan Kim

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