Re: VMA_MERGING_FIXUP and patch

[Andrea Arcangeli <andrea@suse.de>]

On Mon, Mar 22, 2004 at 07:02:01PM +0000, Hugh Dickins wrote:
> On Mon, 22 Mar 2004, Andrea Arcangeli wrote:
> > 
> > what about this?
> > 
> > @@ -344,6 +360,10 @@ void fastcall page_remove_rmap(struct pa
> >    
> >   out_unlock:
> >  	page_map_unlock(page);
> > +
> > +	if (page_test_and_clear_dirty(page) && !page_mapped(page))
> > +		set_page_dirty(page);
> > +
> >  	return;
> >  }
> 
> No, it has to be
> 	if (!page_mapped(page) && page_test_and_clear_dirty(page))
> 		set_page_dirty(page);
> but the positioning is fine.

you're right, I'll fixup. You saved s390 for the second time in a row ;)

> > @@ -523,6 +543,11 @@ int fastcall try_to_unmap(struct page * 
> >  		dec_page_state(nr_mapped);
> >  		ret = SWAP_SUCCESS;
> >  	}
> > +	page_map_unlock(page);
> > +
> > +	if (page_test_and_clear_dirty(page) && ret == SWAP_SUCCESS)
> > +		set_page_dirty(page);
> > +
> >  	return ret;
> >  }
> 
> No, it has to be
> 	if (ret == SWAP_SUCCESS && page_test_and_clear_dirty(page))
> 		set_page_dirty(page);

agreed.

> Personally, I'd prefer we leave try_to_unmap with the lock we
> had on entry, and do this at the shrink_list end - though I
> can see that the way you've done it actually reduces the code.

I agree with you here too, I also preferred not to drop the lock before
return, I did it to reduce the code (in the asm especially).

> (The s390 header file comments that the page_test_and_clear_dirty
> needs to be done while not mapped, because of race with referenced
> bit, and we are opening up to a race now; but unless s390 is very
> different, I see nothing wrong with a rare race on referenced -
> whereas everything wrong with any race that might lose dirty.)

Agreed. I don't see how could ever lose the dirty bit, however the above
comment is scary, and needs clarification from the hw experts. If it's
not safe we can put it back under the page_map_lock. I don't recall
stuff taking the page_map_lock while holding the mapping locks, so it
should be safe (though if we can do it ouside it's preferable.

> Excited by that glimpse of find_pte_nonlinear you just gave us;
> disappointed to find it empty ;-)

8)8)