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;
Date:   Fri, 15 Oct 2021 11:34:08 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH] mm/vmalloc: fix numa spreading for large hash tables
To:     Chen Wandun <chenwandun@huawei.com>,
        Shakeel Butt <shakeelb@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Eric Dumazet <edumazet@google.com>, guohanjun@huawei.com,
        LKML <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>,
        =?iso-8859-1?q?Kefeng=0A?= Wang <wangkefeng.wang@huawei.com>
References: <20210928121040.2547407-1-chenwandun@huawei.com>
        <CALvZod4G3SzP3kWxQYn0fj+VgG-G3yWXz=gz17+3N57ru1iajw@mail.gmail.com>
        <8fc5e1ae-a356-6225-2e50-cf0e5ee26208@huawei.com>
In-Reply-To: <8fc5e1ae-a356-6225-2e50-cf0e5ee26208@huawei.com>
MIME-Version: 1.0
Message-Id: <1634261360.fed2opbgxw.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Chen Wandun's message of October 14, 2021 6:59 pm:
>=20
>=20
> =E5=9C=A8 2021/10/14 5:46, Shakeel Butt =E5=86=99=E9=81=93:
>> On Tue, Sep 28, 2021 at 5:03 AM Chen Wandun <chenwandun@huawei.com> wrot=
e:
>>>
>>> Eric Dumazet reported a strange numa spreading info in [1], and found
>>> commit 121e6f3258fe ("mm/vmalloc: hugepage vmalloc mappings") introduce=
d
>>> this issue [2].
>>>
>>> Dig into the difference before and after this patch, page allocation ha=
s
>>> some difference:
>>>
>>> before:
>>> alloc_large_system_hash
>>>      __vmalloc
>>>          __vmalloc_node(..., NUMA_NO_NODE, ...)
>>>              __vmalloc_node_range
>>>                  __vmalloc_area_node
>>>                      alloc_page /* because NUMA_NO_NODE, so choose allo=
c_page branch */
>>>                          alloc_pages_current
>>>                              alloc_page_interleave /* can be proved by =
print policy mode */
>>>
>>> after:
>>> alloc_large_system_hash
>>>      __vmalloc
>>>          __vmalloc_node(..., NUMA_NO_NODE, ...)
>>>              __vmalloc_node_range
>>>                  __vmalloc_area_node
>>>                      alloc_pages_node /* choose nid by nuam_mem_id() */
>>>                          __alloc_pages_node(nid, ....)
>>>
>>> So after commit 121e6f3258fe ("mm/vmalloc: hugepage vmalloc mappings"),
>>> it will allocate memory in current node instead of interleaving allocat=
e
>>> memory.
>>>
>>> [1]
>>> https://lore.kernel.org/linux-mm/CANn89iL6AAyWhfxdHO+jaT075iOa3XcYn9k6J=
Jc7JR2XYn6k_Q@mail.gmail.com/
>>>
>>> [2]
>>> https://lore.kernel.org/linux-mm/CANn89iLofTR=3DAK-QOZY87RdUZENCZUT4O6a=
0hvhu3_EwRMerOg@mail.gmail.com/
>>>
>>> Fixes: 121e6f3258fe ("mm/vmalloc: hugepage vmalloc mappings")
>>> Reported-by: Eric Dumazet <edumazet@google.com>
>>> Signed-off-by: Chen Wandun <chenwandun@huawei.com>
>>> ---
>>>   mm/vmalloc.c | 33 ++++++++++++++++++++++++++-------
>>>   1 file changed, 26 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>>> index f884706c5280..48e717626e94 100644
>>> --- a/mm/vmalloc.c
>>> +++ b/mm/vmalloc.c
>>> @@ -2823,6 +2823,8 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>>>                  unsigned int order, unsigned int nr_pages, struct page=
 **pages)
>>>   {
>>>          unsigned int nr_allocated =3D 0;
>>> +       struct page *page;
>>> +       int i;
>>>
>>>          /*
>>>           * For order-0 pages we make use of bulk allocator, if
>>> @@ -2833,6 +2835,7 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>>>          if (!order) {
>>=20
>> Can you please replace the above with if (!order && nid !=3D NUMA_NO_NOD=
E)?
>>=20
>>>                  while (nr_allocated < nr_pages) {
>>>                          unsigned int nr, nr_pages_request;
>>> +                       page =3D NULL;
>>>
>>>                          /*
>>>                           * A maximum allowed request is hard-coded and=
 is 100
>>> @@ -2842,9 +2845,23 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
>>>                           */
>>>                          nr_pages_request =3D min(100U, nr_pages - nr_a=
llocated);
>>>
>>=20
>> Undo the following change in this if block.
>=20
> Yes, It seem like more simpler as you suggested, But it still have=20
> performance regression, I plan to change the following to consider
> both mempolcy and alloc_pages_bulk.

Thanks for finding and debugging this. These APIs are a maze of twisty=20
little passages, all alike so I could be as confused as I was when I
wrote that patch, but doesn't a minimal fix look something like this?

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d77830ff604c..75ee9679f521 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2860,7 +2860,10 @@ vm_area_alloc_pages(gfp_t gfp, int nid,
                struct page *page;
                int i;
=20
-               page =3D alloc_pages_node(nid, gfp, order);
+               if (nid =3D=3D NUMA_NO_NODE)
+                       page =3D alloc_pages(gfp, order);
+               else
+                       page =3D alloc_pages_node(nid, gfp, order);
                if (unlikely(!page))
                        break;
=20

Thanks,
Nick