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

David Hildenbrand
2021-03-10T17:58:38Z

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