From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:43479)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1Sz2wL-00076i-MM
	for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:52:34 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1Sz2wJ-0005M7-Bw
	for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:52:33 -0400
Received: from mx1.redhat.com ([209.132.183.28]:1993)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1Sz2wJ-0005Lx-37
	for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:52:31 -0400
Message-ID: <502236D8.3040902@redhat.com>
Date: Wed, 08 Aug 2012 11:52:24 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1344407156-25562-1-git-send-email-qemulist@gmail.com>
	<1344407156-25562-14-git-send-email-qemulist@gmail.com>
In-Reply-To: <1344407156-25562-14-git-send-email-qemulist@gmail.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 13/15] hotplug: introduce
 qdev_unplug_complete() to remove device from views
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: kvm@vger.kernel.org, Jan Kiszka <jan.kiszka@siemens.com>, Marcelo Tosatti <mtosatti@redhat.com>, qemu-devel@nongnu.org, Blue Swirl <blauwirbel@gmail.com>, Avi Kivity <avi@redhat.com>, Anthony Liguori <anthony@codemonkey.ws>, Stefan Hajnoczi <stefanha@gmail.com>, =?ISO-8859-15?Q?Andreas_F=E4rber?= <afaerber@suse.de>

Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
> +void qdev_unplug_complete(DeviceState *dev, Error **errp)
> +{
> +    /* isolate from mem view */
> +    qdev_unmap(dev);
> +    qemu_lock_devtree();
> +    /* isolate from device tree */
> +    qdev_unset_parent(dev);
> +    qemu_unlock_devtree();
> +    object_unref(OBJECT(dev));

Rather than deferring the free, you should defer the unref.  Otherwise
the following can happen when you have "real" RCU access to the memory
map on the read-side:

    VCPU thread                    I/O thread
=====================================================================
    get MMIO request
    rcu_read_lock()
    walk memory map
                                   qdev_unmap()
                                   lock_devtree()
                                   ...
                                   unlock_devtree
                                   unref dev -> refcnt=0, free enqueued
    ref()
    rcu_read_unlock()
                                   free()
    <dangling pointer!>

If you defer the unref, you have instead

    VCPU thread                    I/O thread
=====================================================================
    get MMIO request
    rcu_read_lock()
    walk memory map
                                   qdev_unmap()
                                   lock_devtree()
                                   ...
                                   unlock_devtree
                                   unref is enqueued
    ref() -> refcnt = 2
    rcu_read_unlock()
                                   unref() -> refcnt=1
    unref() -> refcnt = 1

So this also makes patch 14 unnecessary.

Paolo

> +}