Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Minchan Kim <minchan@kernel.org>]

On Fri, Jan 13, 2012 at 11:31:31AM +0800, Huang Shijie wrote:
> 
> >On Fri, Jan 13, 2012 at 10:35:42AM +0800, Huang Shijie wrote:
> >>Hi,
> >>>I think simple patch is returning "return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;"
> >>>It's very clear and readable, I think.
> >>>In this patch, what's the problem you think?
> >>>
> >>sorry for the wrong thread, please read the following thread:
> >>http://marc.info/?l=linux-mm&m=132532266130861&w=2
> >Huang, Thanks for notice that thread.
> >I read and if I understand correctly, the point is that Mel want to see tracepoint
> >"trace_mm_compaction_migratepages" and account "count_vm_event(COMPACTBLOCKS);"
> >My patch does accounting COMPACTBLOCKS so it's not a problem.
> Your patch also accounts the COMPACTBLOCKS In the ISOLATE_NONE and
> ISOLATE_ABOART when :
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>     /* Do not cross the free scanner or scan within a memory hole */
>     if (end_pfn > cc->free_pfn || !pfn_valid(low_pfn)) {
>         cc->migrate_pfn = end_pfn;
>         return ISOLATE_NONE;
>     }
> 
>     /*
>      * Ensure that there are not too many pages isolated from the LRU
>      * list by either parallel reclaimers or compaction. If there are,
>      * delay for some time until fewer pages are isolated
>      */
>     while (unlikely(too_many_isolated(zone))) {
>         /* async migration should just abort */
>         if (!cc->sync)
>             return ISOLATE_ABORT;
> 
>         congestion_wait(BLK_RW_ASYNC, HZ/10);
> 
>         if (fatal_signal_pending(current))
>             return ISOLATE_ABORT;
>     }
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> this not make sense.
> >The problem is my patch doesn't emit trace of "trace_mm_compaction_migratepages".
> >But doesn't it matter? When we doesn't isolate any page at all, both argument in
> >trace_mm_compaction_migratepages are always zero. Is it meaningful tracepoint?
> >Do we really want it?
> >
> IMHO, yes.
> 
> For it _DOES_  scan one PAGEBLOCK even we can not get any page from
> this pageblock.
> it should trace the scan even the parameters are both zero.

Okay. If you want it really, How about this?
Why I insist on is I don't want to change ISOLATE_NONE's semantic.
It's very clear and readable.
We should change code itself instead of semantic of ISOLATE_NONE.

--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
 
        trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
 
-       return ISOLATE_SUCCESS;
+       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
 }
 
 /*
@@ -547,6 +547,12 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
                        ret = COMPACT_PARTIAL;
                        goto out;
                case ISOLATE_NONE:
+                       /*
+                        * If we can't isolate pages at all, we want to
+                        * trace, still.
+                        */
+                       count_vm_event(COMPACTBLOCKS);
+                       trace_mm_compaction_migratepages(0, 0);
                        continue;
                case ISOLATE_SUCCESS:
                        ;



> 
> Huang Shijie
> >>Best Regards
> >>Huang Shijie
> >>
> 
> 

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>