rmap: page_mapping barrier

rmap: page_mapping barrier

[Hugh Dickins]

My page_mapping(page) says PageAnon(page)? NULL: page->mapping;
I've just realized, looking again at sync_page but it goes way
beyond it, that we need smp barriers of some kind somewhere,
don't we?  That is, we cannot just write the address of one of
our non-address_space structures into page->mapping, without
being very careful that others will see the PageAnon and treat
it as NULL.  There are places all over using page_mapping(page)
while another cpu might be right in page_add_rmap.  I go very
mushy when it comes to barriers, you understand them better
than most, any idea what we need to do in page_mapping(page),
and when setting and clearing PageAnon?

Thanks,
Hugh

Re: rmap: page_mapping barrier

[Andrea Arcangeli]

On Thu, Apr 08, 2004 at 02:22:29PM +0100, Hugh Dickins wrote:
> My page_mapping(page) says PageAnon(page)? NULL: page->mapping;
> I've just realized, looking again at sync_page but it goes way
> beyond it, that we need smp barriers of some kind somewhere,
> don't we?  That is, we cannot just write the address of one of
> our non-address_space structures into page->mapping, without
> being very careful that others will see the PageAnon and treat
> it as NULL.  There are places all over using page_mapping(page)
> while another cpu might be right in page_add_rmap.  I go very
> mushy when it comes to barriers, you understand them better
> than most, any idea what we need to do in page_mapping(page),
> and when setting and clearing PageAnon?

could you elaborate those many places that execute page_mapping while
the other cpu does page_add_rmap or page_remove_rmap?

If the VM does a page_mapping in the pagecache layer in my tree, that
means we already executed page_add_rmap long before calling
__add_to_page_cache, the __add_to_page_cache/__remove_from_page_cache
spinlocks are enough to serialize PageAnon. That covers sync_page and
the rest of the pagecache layer.

There's just one place that I'm wondering about and it's page_mapping in
memory.c but it only changes a mark_page_accessed so I don't see any
trouble whatever happens there (mark_page_accessed can be run on any
random page anytime and it cannot do any harm). I believe we can live
with that race just fine, it's controlled by mark_page_accessed
internally by checking PageLru inside the zone->lru_lock.

Please elaborate which is the place that can run at the same time of
page_add_rmap/page_remove_rmap (I mean of the _first_ or _last_
page_add_rmap/page_remove_rmap, the ones that are allowed toggle the
PageAnon bitflag, all the nested ones are harmless).

Re: rmap: page_mapping barrier

[Hugh Dickins]

On Thu, 8 Apr 2004, Andrea Arcangeli wrote:
> On Thu, Apr 08, 2004 at 02:22:29PM +0100, Hugh Dickins wrote:
> > My page_mapping(page) says PageAnon(page)? NULL: page->mapping;
> > I've just realized, looking again at sync_page but it goes way
> > beyond it, that we need smp barriers of some kind somewhere,
> > don't we?  That is, we cannot just write the address of one of
> > our non-address_space structures into page->mapping, without
> > being very careful that others will see the PageAnon and treat
> > it as NULL.  There are places all over using page_mapping(page)
> > while another cpu might be right in page_add_rmap.  I go very
> > mushy when it comes to barriers, you understand them better
> > than most, any idea what we need to do in page_mapping(page),
> > and when setting and clearing PageAnon?
> 
> could you elaborate those many places that execute page_mapping while
> the other cpu does page_add_rmap or page_remove_rmap?

Good challenge.  I thought they were all over, but closer inspection
shows almost all seem safe.  I can't quite bring myself to say
"all are safe".  Most of them (e.g. in arch's flush_dcache_page)
look like they're called when we know there's a real file mapping
(might be truncated beneath us, but won't turn anon), and we could
BUG_ON(PageAnon(page)) - but I don't feel quite sure enough to do
that (nor to use straight page->mapping instead).

> If the VM does a page_mapping in the pagecache layer in my tree, that
> means we already executed page_add_rmap long before calling
> __add_to_page_cache, the __add_to_page_cache/__remove_from_page_cache
> spinlocks are enough to serialize PageAnon. That covers sync_page and
> the rest of the pagecache layer.

I'd expected set_page_dirty to be a problem (it often has been),
but failed to find a problematic instance: there's a nice habit
of doing the set_page_dirty before lowering mapcount.

The one instance I am still a little worried about is sync_page:
I didn't follow your argument above.  But I think I'm remembering
a time long past when there was code which did lock_page on a page
without holding a reference to that page (in which case it could
turn into something else by the time lock acquired); but we don't
do that now - find_lock_page is good, and you've put a good
BUG_ON(PageAnon(page)) into it.

> There's just one place that I'm wondering about and it's page_mapping in
> memory.c but it only changes a mark_page_accessed so I don't see any
> trouble whatever happens there (mark_page_accessed can be run on any
> random page anytime and it cannot do any harm). I believe we can live
> with that race just fine, it's controlled by mark_page_accessed
> internally by checking PageLru inside the zone->lru_lock.

As you say, it wouldn't be a problem anyway; but that one is perfectly
okay because it's before the page_remove_rmap, so it's either stably
PageAnon there, or stably !PageAnon.

There's an (I think) unstable one in refill_inactive_zone, below
the ancient FIXME, but again that's entirely safe because we don't
dereference mapping, and it doesn't matter if we sometimes make a
wrong decision.

I think, ignore my PageAnon barrier concern; but allow me
to say "I told you so" if we ever do find such a race.

Hugh

Re: rmap: page_mapping barrier

[Andrea Arcangeli]

On Thu, Apr 08, 2004 at 06:49:32PM +0100, Hugh Dickins wrote:
> "all are safe".  Most of them (e.g. in arch's flush_dcache_page)

I admit I didn't focus too much on flush_dcache_page since it's a noop
on x86 x86-64 alpha. I don't expect unfixable troubles there, but we
need to re-check non-x86 later.

> The one instance I am still a little worried about is sync_page:
> I didn't follow your argument above.  But I think I'm remembering

sync_page for anon pages is only called after the page is in the
swapcache. Not sure what your code does, my code sure will never call
sync_page unless the mapping is in place and the mapping for me depends
only on pageswapcache:

static inline struct address_space * page_mapping(struct page * page)
{
	extern struct address_space swapper_space;
	struct address_space * mapping = NULL;

	if (unlikely(PageSwapCache(page)))
		mapping = &swapper_space;
	else if (!PageAnon(page))
		mapping = page->mapping;
	return mapping;
}

PageAnon is absolutely worthless for any sync_page in my tree, no way
you will ever call sync_page on anything that isn't swapcache and is
anonymous at the same time, so I don't have to care about it.

if I've overlooking something let me know of course, but my review
showed no issues in terms of PageAnon locking (I mean modulo
flush_dcache_page that I didn't focus much on and as you found some arch
as much bigger troubles with flush_dcache_page regardless PageAnon).

> I think, ignore my PageAnon barrier concern; but allow me
> to say "I told you so" if we ever do find such a race.

I'll do further checking on this when I'm back from vacations on 14,
though if you didn't find anything by that time it most certainly means
there's no race ;). I don't want to discourage you in searching for more
races in this area ;), I just don't see problems. BTW, if my tree will
have a problem here, anonmm should have it too, maybe the other way
around not (really Hugh, my approch of being transparent with
page_mapping if far superior, you may find it ugly to check for
swapper_inode in the pagecache entry/exit points but then it gets
everything right and more obviously, just see the backing-dev-unplugging
code, and the swap_unplug_fn stuff, I only had to change a page->mapping
into a page_mapping in block_sync_page [the per-mapping unplug stuff]
and everything else just worked without rejects).

Re: rmap: page_mapping barrier

[Hugh Dickins]

On Thu, 8 Apr 2004, Andrea Arcangeli wrote:
> 
> I'll do further checking on this when I'm back from vacations on 14,

Have a good one.

> though if you didn't find anything by that time it most certainly means
> there's no race ;).

I'm afraid we can't deduce the nonexistence of a race from my failure
to search very hard.

> I don't want to discourage you in searching for more
> races in this area ;), I just don't see problems. BTW, if my tree will
> have a problem here, anonmm should have it too, maybe the other way

Oh yes, we're quite equal on this: I wasn't trying to blacken
your approach, just raising an issue and asking for help.

> around not (really Hugh, my approch of being transparent with
> page_mapping if far superior, you may find it ugly to check for
> swapper_inode in the pagecache entry/exit points but then it gets
> everything right and more obviously, just see the backing-dev-unplugging
> code, and the swap_unplug_fn stuff, I only had to change a page->mapping
> into a page_mapping in block_sync_page [the per-mapping unplug stuff]
> and everything else just worked without rejects).

On that we differ, and always shall, I expect.  Yes, carrying around
the PageSwapCache? &swapper_space stuff does make sync_page look
cleaner, and shrink_list quite a lot easier - I gave up on trying to
eliminate it completely, would uglify shrink_list too much.  But it
is a fiction, and for everywhere else (I think that's correct) it's
just baggage - made sense when you could stuff it in page->mapping
and then forget about it, but a lot less sense if we have to check
PageSwapCache all over (even if that is hidden in an inline function).

Hugh