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

[David Hildenbrand <david@redhat.com>]

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