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

[David Hildenbrand]

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