linux-kernel - Re: [PATCH v5 3/4] x86/vmemmap: Handle unpopulated sub-pmd ranges

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 Mar 2021 18:58:26 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Oscar Salvador <osalvador@...e.de>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H . Peter Anvin" <hpa@...or.com>,
        Michal Hocko <mhocko@...nel.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] x86/vmemmap: Handle unpopulated sub-pmd ranges

On 10.03.21 18:49, Oscar Salvador wrote:
> On Tue, Mar 09, 2021 at 06:52:38PM +0100, David Hildenbrand wrote:
>>> -#define PAGE_INUSE 0xFD
>>> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
>>> +#define PAGE_UNUSED 0xFD
>>> +
>>> +/* Returns true if the PMD is completely unused and thus it can be freed */
>>> +static bool __meminit vmemmap_pmd_is_unused(unsigned long addr, unsigned long end)
>>> +{
>>
>> I don't think the new name is any better. It implies that all it does is a
>> check - yet it actually clears the given range. (I prefer the old name, but
>> well, I came up with that, so what do I know :D )
> 
> Sorry, I did not mean to offend here.

Oh, I didn't feel offended - I was rather expressing that my opinion 
might be biased because I came up with these names in the s390x variant ;)

> 
> Something like: vmemmap_is_pmd_unused_after_clearing_it would be a bit better
> I guess.
> Tbh, both this and previous one looked fine to me, but I understand where Dave
> confusion was coming from, that is why I decided to rename it.
> 
> Maybe a middle-ground would have been to expand the comment above.

Thinking again, I guess it might be a good idea to factor out the core 
functions into common code. For the optimization part, it might make 
sense too pass some "state" structure that contains e.g., 
"unused_pmd_start".

Then we don't have diverging implementations of essentially the same thing.

Of course, we can do that on top of this series - unifying both 
implementations.

-- 
Thanks,

David / dhildenb