Re: vm fixes for 2.4.19rc1

[Andrea Arcangeli]

On Thu, Jun 27, 2002 at 11:28:46PM +0100, Hugh Dickins wrote:
> On Thu, 27 Jun 2002, Andrea Arcangeli wrote:
> > I will make a full vm update for 2.5 [in small pieces based on post-Andrew
> > split of the monolithic patch] in the next days anyways):
> 
> Great!
> 
> > diff -urNp 2.4.19rc1/mm/page_alloc.c 2.4.19rc1aa1/mm/page_alloc.c
> > --- 2.4.19rc1/mm/page_alloc.c	Tue Jun 25 23:56:23 2002
> > +++ 2.4.19rc1aa1/mm/page_alloc.c	Wed Jun 26 00:55:35 2002
> > @@ -82,8 +82,11 @@ static void __free_pages_ok (struct page
> >  	/* Yes, think what happens when other parts of the kernel take 
> >  	 * a reference to a page in order to pin it for io. -ben
> >  	 */
> > -	if (PageLRU(page))
> > +	if (PageLRU(page)) {
> > +		if (unlikely(in_interrupt()))
> > +			BUG();
> >  		lru_cache_del(page);
> > +	}
> > 
> > this adds a debugging check just in case.
> 
> Well, of course I like this patch, having proposed it months ago.
> But the evidence for needing it seems to have died down, and I
> don't think -rc is the right time for adding a BUG() where you
> can almost always get away with such bad behaviour - I intended
> it for a diagnostic aid in the early stages of a release -
> but I'm happy to go whichever way Marcelo decides on it.

it's up to Marcelo as you say, personally I prefer it to go in for
robusteness.

> > @@ -91,6 +94,8 @@ static void __free_pages_ok (struct page
> >  		BUG();
> >  	if (!VALID_PAGE(page))
> >  		BUG();
> > +	if (PageSwapCache(page))
> > +		BUG();
> > 
> > This resurrects a valid debugging check dropped in rc1.
> 
> Sorry, Andrea, perhaps I should have copied you explicitly on my
> patch which dropped that check.  It is a valid check, yes, but it's
> a waste of space and time: remember that PageSwapCache(page) just
> checks whether page->mapping == &swapper_space (used to be a bitflag,
> yes, but not since last Autumn), and the BUG() you can just see above
> was if page->mapping was set at all.  One day, yes, PageSwapCache may
> become bitflag test again: then would be the time to add the test back.
> 
> > @@ -280,10 +285,12 @@ static struct page * balance_classzone(z
> >  						BUG();
> >  					if (!VALID_PAGE(page))
> >  						BUG();
> > +					if (PageSwapCache(page))
> > +						BUG();
> > 
> > same as above. a page with page->mapping set cannot be freed, if it
> > happens it's a bug that we want to trap.
> 
> As you say, same as above: again the BUG() you can just see at the top
> was on a test for whether page->mapping is set, so PageSwapCache test
> again redundant.  If compiler optimized it away, I'd leave it: but no.

ok.

> > diff -urNp 2.4.19rc1/mm/swap_state.c 2.4.19rc1aa1/mm/swap_state.c
> > --- 2.4.19rc1/mm/swap_state.c	Tue Jan 22 12:55:27 2002
> > +++ 2.4.19rc1aa1/mm/swap_state.c	Wed Jun 26 00:48:13 2002
> > @@ -95,11 +95,8 @@ int add_to_swap_cache(struct page *page,
> >   */
> >  void __delete_from_swap_cache(struct page *page)
> >  {
> > -	if (!PageLocked(page))
> > -		BUG();
> >  	if (!PageSwapCache(page))
> >  		BUG();
> > -	ClearPageDirty(page);
> >  	__remove_inode_page(page);
> >  	INC_CACHE_INFO(del_total);
> >  }
> > 
> > Here I play risky for a rc1, but it made perfect sense to me,
> 
> Yes and yes.  My comment when I weakened the __remove_inode_page BUG
> was "Remove the prior ClearPageDirty?  maybe but not without deeper
> thought: let stay for now."  I've not given it deep enough thought,
> and I'm not convinced you have yet: need to consider the peculiarities
> of mm/shmem.c, and the way clearing the flag lets the page be dirtied

mm/shmem.c looked trivial too, just grep for __delete_from_swap_cache and
delete_from_swap_cache. they all correctly set the dirty bit on the page
afterwards. Infact we shouldn't only remove the clearpagedirty, we
should replace it with a setpagedirty above, just the opposite :), then
we can drop various setpagedirty after delete_from_swap_cache.

> on to a list later.  My _guess_ is that it's fine to remove that
> ClearPageDirty in 2.4, but 2.5 a rather different case; but it
> seems to me a risky cleanup, not -rc material.  (But, sorry,
> maybe I'm just advertizing the shallowness of _my_ thought.)

well, the other PG_swap_cache cleanups that happened in rc1 weren't
strictly necessary too, but I'm fine to postpone the above one. I will
just take care of those bits for next -aa according to the corrections
you made. thanks,

> > @@ -114,9 +111,6 @@ void delete_from_swap_cache(struct page 
> >  {
> >  	swp_entry_t entry;
> >  
> > -	if (!PageLocked(page))
> > -		BUG();
> > -
> >  	block_flushpage(page, 0);
> >  
> >  	entry.val = page->index;
> > 
> > dropped also two pagelocked checks because there execute unconditionally
> > paths that checks the pagelocked bit at the lower layer.
> 
> I don't mind that removal if you leave the !PageLocked BUG test in
> __delete_from_swap_cache, but you've proposed to remove that one too?

Andrea