linux-kernel - Re: [PATCH 10/12] hugetlb: batch PMD split for bulk vmemmap dedup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5ddb085e-c134-4dee-c2f3-50534ce28294@oracle.com>
Date:   Thu, 31 Aug 2023 10:26:11 +0100
From:   Joao Martins <joao.m.martins@...cle.com>
To:     Muchun Song <muchun.song@...ux.dev>
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 31/08/2023 04:54, Muchun Song wrote:
> 
> 
>> 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.

OK, fair enough.