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

Groups > linux.kernel > #1463434 > unrolled thread

Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper

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 v6 08/11] mm, compaction: create compact_gap wrapper Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 08:20 +0200
    Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:40 +0200
      Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 14:20 +0200

#1463434 — Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper

On 08/16/2016 08:15 AM, Joonsoo Kim wrote:
> On Wed, Aug 10, 2016 at 11:12:23AM +0200, Vlastimil Babka wrote:
>> --- a/include/linux/compaction.h
>> +++ b/include/linux/compaction.h
>> @@ -58,6 +58,22 @@ enum compact_result {
>>
>>  struct alloc_context; /* in mm/internal.h */
>>
>> +/*
>> + * Number of free order-0 pages that should be available above given watermark
>> + * to make sure compaction has reasonable chance of not running out of free
>> + * pages that it needs to isolate as migration target during its work.
>> + */
>> +static inline unsigned long compact_gap(unsigned int order)
>> +{
>> +	/*
>> +	 * Although all the isolations for migration are temporary, compaction
>> +	 * may have up to 1 << order pages on its list and then try to split
>> +	 * an (order - 1) free page. At that point, a gap of 1 << order might
>> +	 * not be enough, so it's safer to require twice that amount.
>> +	 */
>> +	return 2UL << order;
>> +}
>
> I agree with this wrapper function but there is a question.
>
> Could you elaborate more on this code comment? Freescanner could keep
> COMPACT_CLUSTER_MAX freepages on the list. It's not associated with
> requested order at least for now. Why compact_gap is 2UL << order in
> this case?

It's true that for high enough order, COMPACT_CLUSTER_MAX might be more 
limiting than 1 << order. But then it also helps to have more free pages 
for probability of compaction success, so I don't think it's worth 
complicating the compact_gap() formula.

> Thanks.
>

[toc] | [next] | [standalone]

#1463451

On Tue, Aug 16, 2016 at 08:15:36AM +0200, Vlastimil Babka wrote:
> On 08/16/2016 08:15 AM, Joonsoo Kim wrote:
> >On Wed, Aug 10, 2016 at 11:12:23AM +0200, Vlastimil Babka wrote:
> >>--- a/include/linux/compaction.h
> >>+++ b/include/linux/compaction.h
> >>@@ -58,6 +58,22 @@ enum compact_result {
> >>
> >> struct alloc_context; /* in mm/internal.h */
> >>
> >>+/*
> >>+ * Number of free order-0 pages that should be available above given watermark
> >>+ * to make sure compaction has reasonable chance of not running out of free
> >>+ * pages that it needs to isolate as migration target during its work.
> >>+ */
> >>+static inline unsigned long compact_gap(unsigned int order)
> >>+{
> >>+	/*
> >>+	 * Although all the isolations for migration are temporary, compaction
> >>+	 * may have up to 1 << order pages on its list and then try to split
> >>+	 * an (order - 1) free page. At that point, a gap of 1 << order might
> >>+	 * not be enough, so it's safer to require twice that amount.
> >>+	 */
> >>+	return 2UL << order;
> >>+}
> >
> >I agree with this wrapper function but there is a question.
> >
> >Could you elaborate more on this code comment? Freescanner could keep
> >COMPACT_CLUSTER_MAX freepages on the list. It's not associated with
> >requested order at least for now. Why compact_gap is 2UL << order in
> >this case?
> 
> It's true that for high enough order, COMPACT_CLUSTER_MAX might be
> more limiting than 1 << order. But then it also helps to have more

AFAIK, regardless of order, migration scanner isolates
COMPACT_CLUSTER_MAX pages. And, freepage scanner isolates
more than nr_migratepages freepages.

> free pages for probability of compaction success, so I don't think
> it's worth complicating the compact_gap() formula.

I agree that it's not worth complicating the compact_gap() formula but
it would be better to fix the comment?

Thanks.

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

#1465211

On 08/16/2016 08:41 AM, Joonsoo Kim wrote:
>> free pages for probability of compaction success, so I don't think
>> it's worth complicating the compact_gap() formula.
> 
> I agree that it's not worth complicating the compact_gap() formula but
> it would be better to fix the comment?

OK, Andrew can you add this -fix?
Thanks.

----8<----
From cad7fb3a61c24feb53e8754248d67715b8ce0c3e Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 18 Aug 2016 14:11:28 +0200
Subject: [PATCH] mm, compaction: create compact_gap wrapper-fix

Clarify the comment of compact_gap() wrt COMPACT_CLUSTER_MAX, per Joonsoo.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/compaction.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index e7f0d34a90fe..585d55cb0dc0 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -67,9 +67,16 @@ static inline unsigned long compact_gap(unsigned int order)
 {
 	/*
 	 * Although all the isolations for migration are temporary, compaction
-	 * may have up to 1 << order pages on its list and then try to split
-	 * an (order - 1) free page. At that point, a gap of 1 << order might
-	 * not be enough, so it's safer to require twice that amount.
+	 * free scanner may have up to 1 << order pages on its list and then
+	 * try to split an (order - 1) free page. At that point, a gap of
+	 * 1 << order might not be enough, so it's safer to require twice that
+	 * amount. Note that the number of pages on the list is also
+	 * effectively limited by COMPACT_CLUSTER_MAX, as that's the maximum
+	 * that the migrate scanner can have isolated on migrate list, and free
+	 * scanner is only invoked when the number of isolated free pages is
+	 * lower than that. But it's not worth to complicate the formula here
+	 * as a bigger gap for higher orders than strictly necessary can also
+	 * improve chances of compaction success.
 	 */
 	return 2UL << order;
 }
-- 
2.9.2

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web