Re: [PATCH] mm: page_alloc: remove branch operation infree_pages_prepare()

[Hugh Dickins]

On Fri, 8 Mar 2013, Joonsoo Kim wrote:
> On Thu, Mar 07, 2013 at 10:54:15AM -0800, Hugh Dickins wrote:
> > On Thu, 7 Mar 2013, Joonsoo Kim wrote:
> > 
> > > When we found that the flag has a bit of PAGE_FLAGS_CHECK_AT_PREP,
> > > we reset the flag. If we always reset the flag, we can reduce one
> > > branch operation. So remove it.
> > > 
> > > Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> > 
> > I don't object to this patch.  But certainly I would have written it
> > that way in order not to dirty a cacheline unnecessarily.  It may be
> > obvious to you that the cacheline in question is almost always already
> > dirty, and the branch almost always more expensive.  But I'll leave that
> > to you, and to those who know more about these subtle costs than I do.
> 
> Yes. I already think about that. I thought that even if a cacheline is
> not dirty at this time, we always touch the 'struct page' in
> set_freepage_migratetype() a little later, so dirtying is not the problem.

I expect that a very high proportion of user pages have
PG_uptodate to be cleared here; and there's also the recently added
page_nid_reset_last(), which will dirty the flags or a nearby field
when CONFIG_NUMA_BALANCING.  Those argue in favour of your patch.

> 
> But, now, I re-think this and decide to drop this patch.
> The reason is that 'struct page' of 'compound pages' may not be dirty
> at this time and will not be dirty at later time.

Actual compound pages would have PG_head or PG_tail or PG_compound
to be cleared there, I believe (check if I'm right on that).  The
questionable case is the ordinary order>0 case without __GFP_COMP
(and page_nid_reset_last() is applied to each subpage of those).

> So this patch is bad idea.

I'm not so sure.  I doubt your patch will make a giant improvement
in kernel performance!  But it might make a little - maybe you just
need to give some numbers from perf to justify it (but I'm easily
dazzled by numbers - don't expect me to judge the result).

Hugh

> 
> Is there any comments?
> 
> Thanks.
> 
> > Hugh
> > 
> > > 
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 8fcced7..778f2a9 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -614,8 +614,7 @@ static inline int free_pages_check(struct page *page)
> > >  		return 1;
> > >  	}
> > >  	page_nid_reset_last(page);
> > > -	if (page->flags & PAGE_FLAGS_CHECK_AT_PREP)
> > > -		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> > > +	page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> > >  	return 0;
> > >  }
> > >  
> > 
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/