Re: [External] RE: [PATCH v4 00/21] Free some vmemmap pages of hugetlb page

[Muchun Song <songmuchun@bytedance.com>]

On Wed, Nov 18, 2020 at 3:22 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Nov 18, 2020 at 12:29:07AM +0800, Muchun Song wrote:
> > > ideally, we should be able to free PageTail if we change struct page in some way.
> > > Then we will save much more for 2MB hugetlb. but it seems it is not easy.
> >
> > Now for the 2MB HugrTLB page, we only free 6 vmemmap pages.
> > But your words woke me up. Maybe we really can free 7 vmemmap
> > pages. In this case, we can see 8 of the 512 struct page structures
> > has beed set PG_head flag. If we can adjust compound_head()
> > slightly and make compound_head() return the real head struct
> > page when the parameter is the tail struct page but with PG_head
> > flag set. I will start an investigation and a test.
>
> What are you thinking?
>
> static inline struct page *compound_head(struct page *page)
> {
>         unsigned long head = READ_ONCE(page->compound_head);
>
>         if (unlikely(head & 1))
>                 return (struct page *) (head - 1);
> +       if (unlikely(page->flags & PG_head))
> +               return (struct page *)(page[1]->compound_head - 1)

Yeah, I think so too. Maybe adding an align check is better.

+         if ((test_bit(PG_head, &page->flags) &&
+              IS_ALIGNED((unsigned long)page, PAGE_SIZE))

>         return page;
> }
>
> ... because if it's that, there are code paths which also just test
> PageHead, and so we'd actually need to change PageHead to be something
> like:

Yeah, I also think that rework compound_head() and PageHead() is enough.

Thanks.

>
> static inline bool PageHead(struct page *page)
> {
>         return (page->flags & PG_head) &&
>                 (page[1]->compound_head == (unsigned long)page + 1);
> }
>
> I'm not sure if that's worth doing -- there may be other things I
> haven't thought of.



-- 
Yours,
Muchun