Re: [Qemu-devel] [RFC PATCH 0/8] MemoryRegion and FlatView refcounting, replace hostmem with memory_region_find

[liu ping fan]

On Thu, May 9, 2013 at 10:50 PM, Paolo Bonzini <address@hidden> wrote:
> Il 09/05/2013 02:53, liu ping fan ha scritto:
>> On Wed, May 8, 2013 at 11:44 PM, Paolo Bonzini <address@hidden> wrote:
>>> Il 08/05/2013 08:20, liu ping fan ha scritto:
>>>> On Mon, May 6, 2013 at 10:25 PM, Paolo Bonzini <address@hidden> wrote:
>>>>> Hi,
>>>>>
>>>>> this is an alternative approach to refactoring of dataplane's HostMem
>>>>> code.  Here, I take Ping Fan's idea of RCU-style updating of the
>>>>> region list and apply it to the AddressSpace's FlatView.  With this
>>>>
>>>> In fact, I am worrying about the priority of MemoryListener, if it is
>>>> true, then we should drop RCU-style idea.
>>>
>>> You mean in hostmem, or in general as in this patch?  Note that this
>>> patch releases the old FlatView at the end of all MemoryListener operations.
>>>
>> Both in hostmem and this patch, they all broke the original design of
>> the  MemoryListener, see notes for priority in code.
>
> I think both hostmem and this patch are fine.  The hypervisor is never
> involved, all accesses go through the "old" FlatView and regions cannot
> disappear thanks to ref/unref.
>
Here, we worry about add, not del.

> In fact, we need _more_ RCU-style updates, not less.  For BQL-less
> dispatch, address space mapping/translation can race against the
> MemoryListeners in exec.c.  To fix this, phys_sections and
> AddressSpaceDispatch need to be reference counted and RCU-ified as well.
>
Agree, I like RCU too.

> Paolo
>
>> I have set out 2 patches to highlight this issue, and have CC you and 
>> Stefanha.
>>
>> Regards,
>> Pingfan
>>
>>> Paolo
>>>
>>>> Also if it is true, there is
>>>> already a bug with hostmem listener. It should use region_del, not
>>>> region_nop to reconstruct the local view. But just let me have a deep
>>>> thinking.
>>>>
>>>> Regards,
>>>> Pingfan
>>>>> change, dataplane can simply use memory_region_find instead of
>>>>> hostmem.
>>>>>
>>>>> This is a somewhat larger change, but I prefer it for two reasons.
>>>>>
>>>>> 1) it splits the task of adding BQL-less memory dispatch in two parts,
>>>>>    tacking memory_region_find first (which is simpler because locking
>>>>>    is left to the caller).
>>>>>
>>>>> 2) HostMem duplicates a lot of the FlatView logic, and adding the
>>>>>    RCU-style update in FlatView benefits everyone.
>>>>>
>>>>> The missing ingredients here are:
>>>>>
>>>>> 1) remember and unreference the MemoryRegions that are used in
>>>>>    a vring entry.  In order to implement this, it is probably simpler
>>>>>    to change vring.c to use virtio.c's VirtQueueElement data structure.
>>>>>    We want something like that anyway in order to support migration.
>>>>>
>>>>> 2) add an owner field to MemoryRegion, and set it for all MemoryRegions
>>>>>    for hot-unpluggable devices.  In this series, ref/unref are stubs.
>>>>>
>>>>> For simplicity I based the patches on my IOMMU rebase.  I placed the
>>>>> tree at git://github.com/bonzini/qemu.git, branch iommu.
>>>>>
>>>>> Paolo
>>>>>
>>>>> Paolo Bonzini (8):
>>>>>   memory: add ref/unref calls
>>>>>   exec: check MRU in qemu_ram_addr_from_host
>>>>>   memory: return MemoryRegion from qemu_ram_addr_from_host
>>>>>   memory: ref/unref memory across address_space_map/unmap
>>>>>   memory: access FlatView from a local variable
>>>>>   memory: use a new FlatView pointer on every topology update
>>>>>   memory: add reference counting to FlatView
>>>>>   dataplane: replace hostmem with memory_region_find
>>>>>
>>>>>  exec.c                                |   63 +++++++++---
>>>>>  hw/core/loader.c                      |    1 +
>>>>>  hw/display/exynos4210_fimd.c          |    6 +
>>>>>  hw/display/framebuffer.c              |   10 +-
>>>>>  hw/i386/kvm/ioapic.c                  |    2 +
>>>>>  hw/i386/kvmvapic.c                    |    1 +
>>>>>  hw/misc/vfio.c                        |    2 +
>>>>>  hw/virtio/dataplane/Makefile.objs     |    2 +-
>>>>>  hw/virtio/dataplane/hostmem.c         |  176 
>>>>> ---------------------------------
>>>>>  hw/virtio/dataplane/vring.c           |   56 +++++++++--
>>>>>  hw/virtio/vhost.c                     |    2 +
>>>>>  hw/virtio/virtio-balloon.c            |    1 +
>>>>>  hw/xen/xen_pt.c                       |    4 +
>>>>>  include/exec/cpu-common.h             |    2 +-
>>>>>  include/exec/memory.h                 |    9 ++
>>>>>  include/hw/virtio/dataplane/hostmem.h |   57 -----------
>>>>>  include/hw/virtio/dataplane/vring.h   |    3 +-
>>>>>  kvm-all.c                             |    2 +
>>>>>  memory.c                              |  142 +++++++++++++++++++++-----
>>>>>  target-arm/kvm.c                      |    2 +
>>>>>  target-i386/kvm.c                     |    4 +-
>>>>>  target-sparc/mmu_helper.c             |    1 +
>>>>>  xen-all.c                             |    2 +
>>>>>  23 files changed, 253 insertions(+), 297 deletions(-)
>>>>>  delete mode 100644 hw/virtio/dataplane/hostmem.c
>>>>>  delete mode 100644 include/hw/virtio/dataplane/hostmem.h
>>>>>
>>>
>