Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220705034841.40931-1-ligang.bdlg@bytedance.com>
In-Reply-To: <20220705034841.40931-1-ligang.bdlg@bytedance.com>
From:   Muchun Song <songmuchun@bytedance.com>
Date:   Tue, 5 Jul 2022 13:20:19 +0800
Message-ID: <CAMZfGtX+1rVOCCVotpdPpPQ6qE=0BARghqabPw2owr-eJbCRCA@mail.gmail.com>
Subject: Re: [PATCH v2] mm, hugetlb: skip irrelevant nodes in show_free_areas()
To:     Gang Li <ligang.bdlg@bytedance.com>
Cc:     Mike Kravetz <mike.kravetz@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 5, 2022 at 11:49 AM Gang Li <ligang.bdlg@bytedance.com> wrote:
>
> show_free_areas() allows to filter out node specific data which is
> irrelevant to the allocation request. But hugetlb_show_meminfo() still
> show hugetlb on all nodes, which is redundant and unnecessary.

s/show/shows/g

>
> Use show_mem_node_skip() to skip irrelevant nodes. And replace
> hugetlb_show_meminfo() with hugetlb_show_meminfo_node(nid).
>
> Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
> ---
> v2: replace hugetlb_show_meminfo() with hugetlb_show_meminfo_node(nid) to avoid
>     exporting show_mem_node_skip.
> ---
>  include/linux/hugetlb.h |  4 ++--
>  mm/hugetlb.c            | 21 ++++++++-------------
>  mm/page_alloc.c         | 10 ++++++++--
>  3 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 29c4d0883d36..21795e62398b 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -155,7 +155,7 @@ void __unmap_hugepage_range_final(struct mmu_gather *tlb,
>                           struct page *ref_page, zap_flags_t zap_flags);
>  void hugetlb_report_meminfo(struct seq_file *);
>  int hugetlb_report_node_meminfo(char *buf, int len, int nid);
> -void hugetlb_show_meminfo(void);
> +void hugetlb_show_meminfo_node(int nid);
>  unsigned long hugetlb_total_pages(void);
>  vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>                         unsigned long address, unsigned int flags);
> @@ -301,7 +301,7 @@ static inline int hugetlb_report_node_meminfo(char *buf, int len, int nid)
>         return 0;
>  }
>
> -static inline void hugetlb_show_meminfo(void)
> +static inline void hugetlb_show_meminfo_node(int nid)
>  {
>  }
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index ca081078e814..c87049c4126e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4490,22 +4490,17 @@ int hugetlb_report_node_meminfo(char *buf, int len, int nid)
>                              nid, h->surplus_huge_pages_node[nid]);
>  }
>
> -void hugetlb_show_meminfo(void)
> +void hugetlb_show_meminfo_node(int nid)
>  {
>         struct hstate *h;
> -       int nid;
>
> -       if (!hugepages_supported())
> -               return;

It is better not to delete this, I know you want to lower the overhead
for users when
!hugepages_supported(). However, it is not a common case and not a hot path.

> -
> -       for_each_node_state(nid, N_MEMORY)
> -               for_each_hstate(h)
> -                       pr_info("Node %d hugepages_total=%u hugepages_free=%u hugepages_surp=%u hugepages_size=%lukB\n",
> -                               nid,
> -                               h->nr_huge_pages_node[nid],
> -                               h->free_huge_pages_node[nid],
> -                               h->surplus_huge_pages_node[nid],
> -                               huge_page_size(h) / SZ_1K);
> +       for_each_hstate(h)
> +               pr_info("Node %d hugepages_total=%u hugepages_free=%u hugepages_surp=%u hugepages_size=%lukB\n",

It is better to use printk here. Two reasons.

 1) To keep consistent with the print in show_free_areas().
 2) If we want to introduce the macro "#define pr_fmt(fmt) "HugeTLB: "
fmt" to hugetlb.c
    in the future, the print here will not be affected.

> +                       nid,
> +                       h->nr_huge_pages_node[nid],
> +                       h->free_huge_pages_node[nid],
> +                       h->surplus_huge_pages_node[nid],
> +                       huge_page_size(h) / SZ_1K);
>  }
>
>  void hugetlb_report_usage(struct seq_file *m, struct mm_struct *mm)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 2eb6ad5a650a..684c2e410923 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6014,7 +6014,7 @@ static void show_migration_types(unsigned char type)
>  void show_free_areas(unsigned int filter, nodemask_t *nodemask)
>  {
>         unsigned long free_pcp = 0;
> -       int cpu;
> +       int cpu, nid;
>         struct zone *zone;
>         pg_data_t *pgdat;
>
> @@ -6202,7 +6202,13 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
>                 printk(KERN_CONT "= %lukB\n", K(total));
>         }
>
> -       hugetlb_show_meminfo();
> +       if (hugepages_supported()) {

As I said above, embed this check into hugetlb_show_meminfo_node().

> +               for_each_node_state(nid, N_MEMORY) {

Please use for_each_online_node here.

Thanks.

> +                       if (show_mem_node_skip(filter, nid, nodemask))
> +                               continue;
> +                       hugetlb_show_meminfo_node(nid);
> +               }
> +       }
>
>         printk("%ld total pagecache pages\n", global_node_page_state(NR_FILE_PAGES));
>
> --
> 2.20.1
>