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

Oscar Salvador
2021-03-09T08:26:14Z

On Mon, Mar 08, 2021 at 07:43:30PM +0100, Oscar Salvador wrote:
> On Thu, Mar 04, 2021 at 09:02:36AM -0800, Dave Hansen wrote:
> > Also, logically, it would make a lot of sense if you can move the actual
> > PMD freeing logic in here. That way, the caller is just saying, "unuse
> > this PMD region", and then this takes care of the rest. As it stands,
> > it's a bit weird that the caller takes care of the freeing.
>
> You mean to move the
>
> free_hugepage_table(pmd_page(*pmd), altmap);
> spin_lock(&init_mm.page_table_lock);
> pmd_clear(pmd);
> spin_unlock(&init_mm.page_table_lock);
>
> block in there?
>
> Well, from where I see it, it is more like:
>
> if (is_the_range_unused())
> : if so, free everything
>
> But I agree with you what it might make some sense to move it there.
> Since I do not feel strong about this, I will move it.

hi Dave,

So, after splitting this patch and re-shape it to address all the
feedback, I am still not sure about this one.
Honestly, I think the freeing logic would be better off kept in
remove_pmd_table.

The reason for me is that vmemmap_unuse_sub_pmd only 1) marks the range
to be removed as unused and 2) checks whether after that, the whole
PMD range is unused.

I think the confusion comes from the name.
"vmemmap_pmd_is_unused" might be a better fit?

What do you think? Do you feel strong about moving the log in there
regardless of the name?

--
Oscar Salvador
SUSE L3