[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <70844AE7-794D-475D-A660-F02EED4BA301@linux.dev>
Date: Thu, 31 Aug 2023 11:54:28 +0800
From: Muchun Song <muchun.song@...ux.dev>
To: Joao Martins <joao.m.martins@...cle.com>
Cc: Mike Kravetz <mike.kravetz@...cle.com>,
Muchun Song <songmuchun@...edance.com>,
Oscar Salvador <osalvador@...e.de>,
David Hildenbrand <david@...hat.com>,
Miaohe Lin <linmiaohe@...wei.com>,
David Rientjes <rientjes@...gle.com>,
Anshuman Khandual <anshuman.khandual@....com>,
Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
Barry Song <song.bao.hua@...ilicon.com>,
Michal Hocko <mhocko@...e.com>,
Matthew Wilcox <willy@...radead.org>,
Xiongchun Duan <duanxiongchun@...edance.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Linux-MM <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/12] hugetlb: batch PMD split for bulk vmemmap dedup
> On Aug 31, 2023, at 00:03, Joao Martins <joao.m.martins@...cle.com> wrote:
>
> On 30/08/2023 12:13, Joao Martins wrote:
>> On 30/08/2023 09:09, Muchun Song wrote:
>>> On 2023/8/26 03:04, Mike Kravetz wrote:
>>>> +
>>>> + /*
>>>> + * We are only splitting, not remapping the hugetlb vmemmap
>>>> + * pages.
>>>> + */
>>>> + if (bulk)
>>>> + continue;
>>>
>>> Actually, we don not need a flag to detect this situation, you could
>>> use "!@...k->remap_pte" to determine whether we should go into the
>>> next level traversal of the page table. ->remap_pte is used to traverse
>>> the pte entry, so it make senses to continue to the next pmd entry if
>>> it is NULL.
>>>
>>
>> Yeap, great suggestion.
>>
>>>> +
>>>> vmemmap_pte_range(pmd, addr, next, walk);
>>>> } while (pmd++, addr = next, addr != end);
>>>> @@ -197,7 +211,8 @@ static int vmemmap_remap_range(unsigned long start,
>>>> unsigned long end,
>>>> return ret;
>>>> } while (pgd++, addr = next, addr != end);
>>>> - flush_tlb_kernel_range(start, end);
>>>> + if (!(walk->flags & VMEMMAP_REMAP_ONLY_SPLIT))
>>>> + flush_tlb_kernel_range(start, end);
>>>
>>> This could be:
>>>
>>> if (walk->remap_pte)
>>> flush_tlb_kernel_range(start, end);
>>>
>> Yeap.
>>
>
> Quite correction: This stays as is, except with a flag rename. That is because
> this is actual flush that we intend to batch in the next patch. And while the
> PMD split could just use !walk->remap_pte, the next patch would just need to
> test NO_TLB_FLUSH flag. Meaning we endup anyways just testing for this
> to-be-consolidated flag
I think this really should be "if (walk->remap_pte && !(flag & VMEMMAP_NO_TLB_FLUSH))"
in your next patch. This TLB flushing only make sense for the case of existing of
@walk->remap_pte. I know "if (!(flag & VMEMMAP_NO_TLB_FLUSH))" check is suitable for your
use case, but what if a user (even if it does not exist now, but it may in the future)
passing a NULL @walk->remap_pte and not specifying VMEMMAP_NO_TLB_FLUSH? Then we will
do a useless TLB flushing. This is why I suggest you change this to "if (walk->remap_pte)"
in this patch and change it to "if (walk->remap_pte && !(flag & VMEMMAP_NO_TLB_FLUSH))"
in the next patch.
Thanks.
Powered by blists - more mailing lists