From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:52270)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UcWyZ-00045l-RS
	for qemu-devel@nongnu.org; Wed, 15 May 2013 04:22:22 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UcWyV-0004pN-77
	for qemu-devel@nongnu.org; Wed, 15 May 2013 04:22:19 -0400
Received: from mx1.redhat.com ([209.132.183.28]:44301)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UcWyU-0004pG-Sw
	for qemu-devel@nongnu.org; Wed, 15 May 2013 04:22:15 -0400
Message-ID: <519345AB.3030208@redhat.com>
Date: Wed, 15 May 2013 10:22:03 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1368415264-10800-1-git-send-email-qemulist@gmail.com>
	<1368415264-10800-3-git-send-email-qemulist@gmail.com>
	<5190B2FD.7090008@redhat.com>
	<CAJnKYQn6yqfEc_ieR1+bAAUSn-9uQAq0k6BYMu+7pCLEf8EqHw@mail.gmail.com>
	<51920512.3090708@redhat.com>
	<CAJnKYQnrnsa0vHoox9Er2V3VRVQJauR6WoTEDiJyzd4LiKMQ7A@mail.gmail.com>
In-Reply-To: <CAJnKYQnrnsa0vHoox9Er2V3VRVQJauR6WoTEDiJyzd4LiKMQ7A@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC PATCH 2/2] mem: prepare address_space
	listener rcu style
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: liu ping fan <qemulist@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>, Anthony Liguori <anthony@codemonkey.ws>, "Michael S. Tsirkin" <mst@redhat.com>, Jan Kiszka <jan.kiszka@siemens.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>

Il 15/05/2013 03:29, liu ping fan ha scritto:
>>>> Pointers are quite expensive here.  With RCU we can fetch a consistent
>>>> root/table pair like this:
>>>>
>>>>     rcu_read_lock();
>>>>     do {
>>>>         pgtbl = d->cur_pgtbl;
>>>>         smp_rmb();
>>>>         root = d->cur_root;
>>>>
>>>>         /* RCU ensures that d->cur_pgtbl remains alive, thus it cannot
>>>>          * be recycled while this loop is running.  If
>>>>          * d->cur_pgtbl == pgtbl, the root is the right one for this
>>>>          * pgtable.
>>>>          */
>>>>         smp_rmb();
>>>>     } while (d->cur_pgtbl == pgtbl);
>>
>> Ouch, != of course.
>>
>>>>     ...
>>>>     rcu_read_unlock();
>>>>
>>> It seems to break the semantic of rcu_dereference() and rcu_assign().
>>
>> It doesn't.  In fact it is even stronger, I'm using a "full" rmb instead
>> of read_barrier_depends.
>>
> rcu_dereference()/rcu_assign() ensure the switch from prev to next
> version, based on atomic-ops.

rcu_dereference()/rcu_assign() are not magic, they are simply
read+read_barrier_depends and wmb+write.

> I think your method _does_ work based on
> read+check+barrier skill, but it is not the standard RCU method, and
> export some concept (barrier) outside RCU.

It is a standard method to load 2 words and ensure it is consistent.  If
you want to use rcu_dereference(&d->cur_pgtbl) and
rcu_dereference(&d->cur_root), that's fine.  But you still need the read
barrier.

>>> If pointers are expensive, how about this:
>>> if (unlikely(d->prev_map!=d->cur_map)) {
>>>     d->root = d->cur_map->root;
>>>     d->pgtbl = d->cur_map->root;
>>>     d->prev_map = d->cur_map;
>>> }
>>> So usually, we use cache value.
>>
> rcu_read_lock();
> map = rcu_derefenrence(d->cur_map)
> if (unlikely(d->prev_map!=map) {
>     d->root = map->root;
>     d->pgtbl = map->pgtbl;
> }
> ......
> rcu_read_unlock();
> 
> Then it can avoid ABA problem.

I don't see the assignment of prev_map, which is where the ABA problem
arises.

Paolo