Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility

On Tue, Aug 21, 2012 at 02:28:20PM -0300, Rafael Aquini wrote:

On Tue, Aug 21, 2012 at 09:24:32AM -0700, Paul E. McKenney wrote:

quoted

On Tue, Aug 21, 2012 at 05:20:11PM +0200, Peter Zijlstra wrote:

quoted

On Tue, 2012-08-21 at 09:47 -0300, Rafael Aquini wrote:

quoted

+       mapping = rcu_access_pointer(page->mapping);
+       if (mapping)
+               mapping = mapping->assoc_mapping; 

The comment near rcu_access_pointer() explicitly says:

 * Return the value of the specified RCU-protected pointer, but omit the
 * smp_read_barrier_depends() and keep the ACCESS_ONCE().  This is useful
 * when the value of this pointer is accessed, but the pointer is not
 * dereferenced,

Yet you dereference the pointer... smells like fail to me.

Indeed!

This will break DEC Alpha.  In addition, if ->mapping can transition
from non-NULL to NULL, and if you used rcu_access_pointer() rather
than rcu_dereference() to avoid lockdep-RCU from yelling at you about
not either being in an RCU read-side critical section or holding an
update-side lock, you can see failures as follows:

1.	CPU 0 runs the above code, picks up mapping, and finds it non-NULL.

2.	CPU 0 is preempted or otherwise delayed.  (Keep in mind that
	even disabling interrupts in a guest OS does not prevent the
	host hypervisor from preempting!)

3.	Some other CPU NULLs page->mapping.  Because CPU 0 isn't doing
	anything to prevent it, this other CPU frees the memory.

4.	CPU 0 resumes, and then accesses what is now the freelist.
	Arbitrarily bad things start happening.

If you are in a read-side critical section, use rcu_dereference() instead
of rcu_access_pointer().  If you are holding an update-side lock, use
rcu_dereference_protected() and say what lock you are holding.  If you
are doing something else, please say what it is.

							Thanx, Paul

Paul & Peter,

Thanks for looking into this stuff and providing me such valuable feedback, and
RCU usage crashcourse.

I believe rcu_dereference_protected() is what I want/need here, since this code
is always called for pages which we hold locked (PG_locked bit).

It would only help if we locked the page while updating the mapping,
as far as I can see we don't.

So, it brings me
to ask you if the following usage looks sane enough to fix the well pointed issue,
or if it's another misuse of RCU API:

+       mapping = rcu_dereference_protecetd(page->mapping, PageLocked(page));
+       if (mapping)
+               mapping = mapping->assoc_mapping; 

On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:

quoted

I believe rcu_dereference_protected() is what I want/need here, since this code
is always called for pages which we hold locked (PG_locked bit).

It would only help if we locked the page while updating the mapping,
as far as I can see we don't.

But we can do it. In fact, by doing it (locking the page) we can easily avoid
the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.