Re: [PATCH v2] mm/folio: Avoid special handling for order value 0 in folio_set_order

[Mike Kravetz <mike.kravetz@oracle.com>]

On 06/08/23 15:33, Tarun Sahu wrote:
> Hi Mike,
> 
> Please find my comments inline.
> 
> Mike Kravetz <mike.kravetz@oracle.com> writes:
> 
> > On 06/06/23 10:32, Tarun Sahu wrote:
> >>                                        
> >> Hi Mike,              
> >>     
> >> Thanks for your inputs.                          
> >> I wanted to know if you find it okay, Can I send it again adding your Reviewed-by?
> >
> > Hi Tarun,
> >
> > Just a few more comments/questions.
> >
> > On 05/15/23 22:38, Tarun Sahu wrote:
> >> folio_set_order(folio, 0) is used in kernel at two places
> >> __destroy_compound_gigantic_folio and __prep_compound_gigantic_folio.
> >> Currently, It is called to clear out the folio->_folio_nr_pages and
> >> folio->_folio_order.
> >> 
> >> For __destroy_compound_gigantic_folio:
> >> In past, folio_set_order(folio, 0) was needed because page->mapping used
> >> to overlap with _folio_nr_pages and _folio_order. So if these fields were
> >> left uncleared during freeing gigantic hugepages, they were causing
> >> "BUG: bad page state" due to non-zero page->mapping. Now, After
> >> Commit a01f43901cfb ("hugetlb: be sure to free demoted CMA pages to
> >> CMA") page->mapping has explicitly been cleared out for tail pages. Also,
> >> _folio_order and _folio_nr_pages no longer overlaps with page->mapping.
> >
> > I believe the same logic/reasoning as above also applies to
> > __prep_compound_gigantic_folio.
> > Why?
> > In __prep_compound_gigantic_folio we only call folio_set_order(folio, 0)
> > in the case of error.  If __prep_compound_gigantic_folio fails, the caller
> > will then call free_gigantic_folio() on the "gigantic page".  However, it is
> > not really a gigantic  at this point in time, and we are simply calling
> > cma_release() or free_contig_range().
> > The end result is that I do not believe the existing call to
> > folio_set_order(folio, 0) in __prep_compound_gigantic_folio is actually
> > required.  ???
> No, there is a difference. IIUC, __destroy_compound_gigantic_folio
> explicitly reset page->mapping for each page of compound page which
> makes sure, even if in future some fields of struct page/folio overlaps
> with page->mapping, it won't cause `BUG: bad page state` error. But If we
> just remove folio_set_order(folio, 0) from __prep_compound_gigantic_folio
> without moving folio_set_order(folio, order), this will cause extra
> maintenance overhead to track if page->_folio_order overlaps with
> page->mapping everytime struct page fields are changed. As in case of
> overlapping page->mapping will be non-zero. IMHO, To avoid it,
> moving the folio_set_order(folio, order) after all error checks are
> done on tail pages. So, _folio_order is either set on success and not
> set in case of error. (which is the original proposal). But for
> folio_set_head, I agree the way you suggested below.
> 
> WDYT?

Right.  It is more 'future proof' to only set folio order on success as
done in your original patch.

> >
> > If my reasoning above is correct, then we could just have one patch to
> > remove the folio_set_order(folio, 0) calls and remove special casing for
> > order 0 in folio_set_order.
> >
> > However, I still believe your restructuring of __prep_compound_gigantic_folio,
> > is of value.  I do not believe there is an issue as questioned by Matthew.  My
> > reasoning has been stated previously.  We could make changes like the following
> > to retain the same order of operations in __prep_compound_gigantic_folio and
> > totally avoid Matthew's question.  Totally untested.
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index ea24718db4af..a54fee663cb1 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1950,10 +1950,8 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> >  	int nr_pages = 1 << order;
> >  	struct page *p;
> >  
> > -	__folio_clear_reserved(folio);
> > -	__folio_set_head(folio);
> >  	/* we rely on prep_new_hugetlb_folio to set the destructor */
> > -	folio_set_order(folio, order);
> > +
> >  	for (i = 0; i < nr_pages; i++) {
> >  		p = folio_page(folio, i);
> >  
> > @@ -1969,7 +1967,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> >  		 * on the head page when they need know if put_page() is needed
> >  		 * after get_user_pages().
> >  		 */
> > -		if (i != 0)	/* head page cleared above */
> > +		if (i != 0)	/* head page cleared below */
> >  			__ClearPageReserved(p);
> >  		/*
> >  		 * Subtle and very unlikely
> > @@ -1996,8 +1994,14 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> >  		} else {
> >  			VM_BUG_ON_PAGE(page_count(p), p);
> >  		}
> > -		if (i != 0)
> > +
> > +		if (i == 0) {
> > +			__folio_clear_reserved(folio);
> > +			__folio_set_head(folio);
> > +			folio_set_order(folio, order);
> With folio_set_head, I agree to this, But does not feel good with
> folio_set_order as per my above reasoning. WDYT?

Agree with your reasoning.  We should just move __folio_set_head and
folio_set_order after the loop as you originally suggested.

> 
> > +		} else {
> >  			set_compound_head(p, &folio->page);
> > +		}
> >  	}
> >  	atomic_set(&folio->_entire_mapcount, -1);
> >  	atomic_set(&folio->_nr_pages_mapped, 0);
> > @@ -2017,7 +2021,6 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> >  		p = folio_page(folio, j);
> >  		__ClearPageReserved(p);
> >  	}
> > -	folio_set_order(folio, 0);
> >  	__folio_clear_head(folio);
> >  	return false;
> >  }
> >
> >
> >> 
> >> struct page {
> >> ...
> >>    struct address_space * mapping;  /* 24     8 */
> >> ...
> >> }
> >> 
> >> struct folio {
> >> ...
> >>     union {
> >>         struct {
> >>         	long unsigned int _flags_1;      /* 64    8 */
> >>         	long unsigned int _head_1;       /* 72    8 */
> >>         	unsigned char _folio_dtor;       /* 80    1 */
> >>         	unsigned char _folio_order;      /* 81    1 */
> >> 
> >>         	/* XXX 2 bytes hole, try to pack */
> >> 
> >>         	atomic_t   _entire_mapcount;     /* 84    4 */
> >>         	atomic_t   _nr_pages_mapped;     /* 88    4 */
> >>         	atomic_t   _pincount;            /* 92    4 */
> >>         	unsigned int _folio_nr_pages;    /* 96    4 */
> >>         };                                       /* 64   40 */
> >>         struct page __page_1 __attribute__((__aligned__(8))); /* 64   64 */
> >>     }
> >> ...
> >> }
> >
> > I do not think the copy of page/folio definitions adds much value to the
> > commit message.
> Yeah, Will remove it.
> >

I think we are finally on the same page.  I am good with this v2 patch.
Only change is to update commit message to remove the definitions.  
-- 
Mike Kravetz