From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 13/15] hotplug: introduce
 qdev_unplug_complete() to remove device from views
Date: Wed, 08 Aug 2012 11:52:24 +0200
Message-ID: <502236D8.3040902@redhat.com>
References: <1344407156-25562-1-git-send-email-qemulist@gmail.com>
	<1344407156-25562-14-git-send-email-qemulist@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
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>
To: Liu Ping Fan <qemulist@gmail.com>
Return-path: <qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org>
In-Reply-To: <1344407156-25562-14-git-send-email-qemulist@gmail.com>
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>
Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
List-Id: kvm.vger.kernel.org

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

> +}