Re: [PATCH v5 4/4] add profile information for invalidated page

[Mel Gorman]

On Sat, Feb 19, 2011 at 07:07:01AM +0900, Minchan Kim wrote:
> Hi Mel,
> 
> On Sat, Feb 19, 2011 at 1:58 AM, Mel Gorman <mel@xxxxxxxxx> wrote:
> > On Fri, Feb 18, 2011 at 12:08:22AM +0900, Minchan Kim wrote:
> >> This patch adds profile information about invalidated page reclaim.
> >> It's just for profiling for test so it is never for merging.
> >>
> >> Acked-by: Rik van Riel <riel@xxxxxxxxxx>
> >> Cc: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
> >> Cc: Wu Fengguang <fengguang.wu@xxxxxxxxx>
> >> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> >> Cc: Nick Piggin <npiggin@xxxxxxxxx>
> >> Cc: Mel Gorman <mel@xxxxxxxxx>
> >> Signed-off-by: Minchan Kim <minchan.kim@xxxxxxxxx>
> >> ---
> >>  include/linux/vmstat.h |    4 ++--
> >>  mm/swap.c              |    3 +++
> >>  mm/vmstat.c            |    3 +++
> >>  3 files changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> >> index 833e676..c38ad95 100644
> >> --- a/include/linux/vmstat.h
> >> +++ b/include/linux/vmstat.h
> >> @@ -30,8 +30,8 @@
> >>
> >>  enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
> >>               FOR_ALL_ZONES(PGALLOC),
> >> -             PGFREE, PGACTIVATE, PGDEACTIVATE,
> >> -             PGFAULT, PGMAJFAULT,
> >> +             PGFREE, PGACTIVATE, PGDEACTIVATE, PGINVALIDATE,
> >> +             PGRECLAIM, PGFAULT, PGMAJFAULT,
> >>               FOR_ALL_ZONES(PGREFILL),
> >>               FOR_ALL_ZONES(PGSTEAL),
> >>               FOR_ALL_ZONES(PGSCAN_KSWAPD),
> >> diff --git a/mm/swap.c b/mm/swap.c
> >> index 0a33714..980c17b 100644
> >> --- a/mm/swap.c
> >> +++ b/mm/swap.c
> >> @@ -397,6 +397,7 @@ static void lru_deactivate(struct page *page, struct zone *zone)
> >>                * is _really_ small and  it's non-critical problem.
> >>                */
> >>               SetPageReclaim(page);
> >> +             __count_vm_event(PGRECLAIM);
> >>       } else {
> >>               /*
> >>                * The page's writeback ends up during pagevec
> >
> > Is this name potentially misleading?
> >
> > Pages that are reclaimed are accounted for with _steal. It's not particularly
> > obvious but that's the name it was given. I'd worry that an administrator that
> > was not aware of *_steal would read pgreclaim as "pages that were reclaimed"
> > when this is not necessarily the case.
> >
> > Is there a better name for this? pginvalidate_deferred
> > or pginvalidate_delayed maybe?
> >
> 
> Yep. Your suggestion is fair enough. But as I said in description,
> It's just for testing for my profiling, not merging so I didn't care
> about it. I don't think we need new vmstat of pginvalidate.
> 

My bad. I was treating this piece of information as something we'd keep
around and did not read the introduction clearly enough. If it's just for
evaluation, the name does not matter as long as the reviewers know what it
is. The figures look good and I have no problem with the series. I didn't
ack the memcg parts but only because memcg is not an area I'm familiar enough
for my ack to proper meaning. If there are no other objections, I'd suggest
resubmitting minus this patch.

Thanks.

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