Re: [PATCH 2/4] mm: more intensive memory corruption debug

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Fri, Nov 11, 2011 at 02:29:53PM +0000, Mel Gorman wrote:
> >  	if (PageBuddy(buddy) && page_order(buddy) == order) {
> >  		VM_BUG_ON(page_count(buddy) != 0);
> >  		return 1;
> > @@ -518,9 +562,15 @@ static inline void __free_one_page(struct page *page,
> >  			break;
> >  
> >  		/* Our buddy is free, merge with it and move up one order. */
> > -		list_del(&buddy->lru);
> > -		zone->free_area[order].nr_free--;
> > -		rmv_page_order(buddy);
> > +		if (page_is_corrupt_dbg(buddy)) {
> > +			clear_page_corrupt_dbg(buddy);
> > +			set_page_private(page, 0);
> > +			__mod_zone_page_state(zone, NR_FREE_PAGES, 1 << order);
> 
> Why are the buddies not merged?
I believe they are merged, but I'll double check.

> >  static inline void expand(struct zone *zone, struct page *page,
> > -	int low, int high, struct free_area *area,
> > +	unsigned int low, unsigned int high, struct free_area *area,
> >  	int migratetype)
> >  {
> >  	unsigned long size = 1 << high;
> > @@ -746,9 +796,16 @@ static inline void expand(struct zone *zone, struct page *page,
> >  		high--;
> >  		size >>= 1;
> >  		VM_BUG_ON(bad_range(zone, &page[size]));
> > -		list_add(&page[size].lru, &area->free_list[migratetype]);
> > -		area->nr_free++;
> > -		set_page_order(&page[size], high);
> > +		if (high < corrupt_dbg()) {
> > +			INIT_LIST_HEAD(&page[size].lru);
> > +			set_page_corrupt_dbg(&page[size]);
> > +			set_page_private(&page[size], high);
> > +			__mod_zone_page_state(zone, NR_FREE_PAGES, -(1 << high));
> > +		} else {
> 
> Because high is a signed integer, I don't think this would necessarily
> optimised away at compile time when DEBUG_PAGEALLOC is not set adding a
> new branch to a heavily executed fast path.
> 
> For the fast paths, you should not add new branches if you can. Move the
> debugging code to inline functions that only exist when DEBUG_PAGEALLOC
> is set so there is no additional overhead in the !CONFIG_DEBUG_PAGEALLOC
> case.

I changed "high" type from int to unsigned int in the patch, and checked
that this branch is removed by compiler in !CONFIG_DEBUG_PAGEALLOC case.
But perhaps having this inside preprocessor checks is cleaner, so I'll do
that.

Thanks for the comments, I'll rework and repost.
Stanislaw