Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200818184122.29C415DF@viggo.jf.intel.com> <20200818184133.DCF58B5F@viggo.jf.intel.com>
In-Reply-To: <20200818184133.DCF58B5F@viggo.jf.intel.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Thu, 20 Aug 2020 15:26:02 -0700
Message-ID: <CAHbLzkr9e=hsT-Fw14cwmBoK7p=GsbzAmVqo6HFCFRXz6sa0iw@mail.gmail.com>
Subject: Re: [RFC][PATCH 6/9] mm/vmscan: add page demotion counter
To:     Dave Hansen <dave.hansen@linux.intel.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        David Rientjes <rientjes@google.com>,
        Huang Ying <ying.huang@intel.com>,
        Dan Williams <dan.j.williams@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 18, 2020 at 11:53 AM Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
>
> From: Yang Shi <yang.shi@linux.alibaba.com>
>
> Account the number of demoted pages into reclaim_state->nr_demoted.
>
> Add pgdemote_kswapd and pgdemote_direct VM counters showed in
> /proc/vmstat.
>
> [ daveh:
>    - __count_vm_events() a bit, and made them look at the THP
>      size directly rather than getting data from migrate_pages()
> ]

This reminds me that we'd better to make migrate_pages() return the
number of base pages. Now both nr_failed and nr_succeeded is inc'ed
regardless of whether it is THP.

A quick look finds the callers of migrate_pages() seem not care about
the exact returned number, they just care if it is positive or not.

And reclaim code depends on correct nr_succeeded to avoid over reclaim.

I will come up with a patch to post to the mailing list.

>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> ---
>
>  b/include/linux/vm_event_item.h |    2 ++
>  b/mm/vmscan.c                   |    6 ++++++
>  b/mm/vmstat.c                   |    2 ++
>  3 files changed, 10 insertions(+)
>
> diff -puN include/linux/vm_event_item.h~mm-vmscan-add-page-demotion-counter include/linux/vm_event_item.h
> --- a/include/linux/vm_event_item.h~mm-vmscan-add-page-demotion-counter 2020-08-18 11:36:54.062583176 -0700
> +++ b/include/linux/vm_event_item.h     2020-08-18 11:36:54.070583176 -0700
> @@ -32,6 +32,8 @@ enum vm_event_item { PGPGIN, PGPGOUT, PS
>                 PGREFILL,
>                 PGSTEAL_KSWAPD,
>                 PGSTEAL_DIRECT,
> +               PGDEMOTE_KSWAPD,
> +               PGDEMOTE_DIRECT,
>                 PGSCAN_KSWAPD,
>                 PGSCAN_DIRECT,
>                 PGSCAN_DIRECT_THROTTLE,
> diff -puN mm/vmscan.c~mm-vmscan-add-page-demotion-counter mm/vmscan.c
> --- a/mm/vmscan.c~mm-vmscan-add-page-demotion-counter   2020-08-18 11:36:54.064583176 -0700
> +++ b/mm/vmscan.c       2020-08-18 11:36:54.072583176 -0700
> @@ -147,6 +147,7 @@ struct scan_control {
>                 unsigned int immediate;
>                 unsigned int file_taken;
>                 unsigned int taken;
> +               unsigned int demoted;
>         } nr;
>
>         /* for recording the reclaimed slab by now */
> @@ -1146,6 +1147,11 @@ static unsigned int demote_page_list(str
>                 list_splice(ret_list, demote_pages);
>         }
>
> +       if (current_is_kswapd())
> +               __count_vm_events(PGDEMOTE_KSWAPD, nr_succeeded);
> +       else
> +               __count_vm_events(PGDEMOTE_DIRECT, nr_succeeded);
> +
>         return nr_succeeded;
>  }
>
> diff -puN mm/vmstat.c~mm-vmscan-add-page-demotion-counter mm/vmstat.c
> --- a/mm/vmstat.c~mm-vmscan-add-page-demotion-counter   2020-08-18 11:36:54.067583176 -0700
> +++ b/mm/vmstat.c       2020-08-18 11:36:54.072583176 -0700
> @@ -1200,6 +1200,8 @@ const char * const vmstat_text[] = {
>         "pgrefill",
>         "pgsteal_kswapd",
>         "pgsteal_direct",
> +       "pgdemote_kswapd",
> +       "pgdemote_direct",
>         "pgscan_kswapd",
>         "pgscan_direct",
>         "pgscan_direct_throttle",
> _