1. Patchew
  2. QEMU
  3. View series

[PATCH] memory: avoid unnecessary iteration when updating ioeventfds

Longpeng(Mike) via posted 1 patch 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230228142514.2582-1-longpeng2@huawei.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
include/exec/memory.h |  2 ++
softmmu/memory.c      | 28 +++++++++++++++++++++++++++-
2 files changed, 29 insertions(+), 1 deletion(-)
[PATCH] memory: avoid unnecessary iteration when updating ioeventfds
Posted by Longpeng(Mike) via 1 year, 2 months ago 
From: Longpeng <longpeng2@huawei.com>

When updating ioeventfds, we need to iterate all address spaces and
iterate all flat ranges of each address space. There is so much
redundant process that a FlatView would be iterated for so many times
during one commit (memory_region_transaction_commit).

We can mark a FlatView as UPDATED and then skip it in the next iteration
and clear the UPDATED flag at the end of the commit. The overhead can
be significantly reduced.

For example, a VM with 16 vdpa net devices and each one has 65 vectors,
can reduce the time spent on memory_region_transaction_commit by 95%.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 include/exec/memory.h |  2 ++
 softmmu/memory.c      | 28 +++++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2e602a2fad..974eabf765 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1093,6 +1093,8 @@ struct FlatView {
     unsigned nr_allocated;
     struct AddressSpaceDispatch *dispatch;
     MemoryRegion *root;
+#define FLATVIEW_FLAG_IOEVENTFD_UPDATED (1 << 0)
+    unsigned flags;
 };
 
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 9d64efca26..71ff996712 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -815,6 +815,15 @@ FlatView *address_space_get_flatview(AddressSpace *as)
     return view;
 }
 
+static void address_space_reset_view_flags(AddressSpace *as, unsigned mask)
+{
+    FlatView *view = address_space_get_flatview(as);
+
+    if (view->flags & mask) {
+        view->flags &= ~mask;
+    }
+}
+
 static void address_space_update_ioeventfds(AddressSpace *as)
 {
     FlatView *view;
@@ -825,6 +834,12 @@ static void address_space_update_ioeventfds(AddressSpace *as)
     AddrRange tmp;
     unsigned i;
 
+    view = address_space_get_flatview(as);
+    if (view->flags & FLATVIEW_FLAG_IOEVENTFD_UPDATED) {
+        return;
+    }
+    view->flags |= FLATVIEW_FLAG_IOEVENTFD_UPDATED;
+
     /*
      * It is likely that the number of ioeventfds hasn't changed much, so use
      * the previous size as the starting value, with some headroom to avoid
@@ -833,7 +848,6 @@ static void address_space_update_ioeventfds(AddressSpace *as)
     ioeventfd_max = QEMU_ALIGN_UP(as->ioeventfd_nb, 4);
     ioeventfds = g_new(MemoryRegionIoeventfd, ioeventfd_max);
 
-    view = address_space_get_flatview(as);
     FOR_EACH_FLAT_RANGE(fr, view) {
         for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
             tmp = addrrange_shift(fr->mr->ioeventfds[i].addr,
@@ -1086,6 +1100,15 @@ void memory_region_transaction_begin(void)
     ++memory_region_transaction_depth;
 }
 
+static inline void address_space_update_ioeventfds_finish(void)
+{
+    AddressSpace *as;
+
+    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
+        address_space_reset_view_flags(as, FLATVIEW_FLAG_IOEVENTFD_UPDATED);
+    }
+}
+
 void memory_region_transaction_commit(void)
 {
     AddressSpace *as;
@@ -1106,12 +1129,14 @@ void memory_region_transaction_commit(void)
             }
             memory_region_update_pending = false;
             ioeventfd_update_pending = false;
+            address_space_update_ioeventfds_finish();
             MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
         } else if (ioeventfd_update_pending) {
             QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
                 address_space_update_ioeventfds(as);
             }
             ioeventfd_update_pending = false;
+            address_space_update_ioeventfds_finish();
         }
    }
 }
@@ -3076,6 +3101,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
     as->name = g_strdup(name ? name : "anonymous");
     address_space_update_topology(as);
     address_space_update_ioeventfds(as);
+    address_space_reset_view_flags(as, FLATVIEW_FLAG_IOEVENTFD_UPDATED);
 }
 
 static void do_address_space_destroy(AddressSpace *as)
-- 
2.23.0
Re: [PATCH] memory: avoid unnecessary iteration when updating ioeventfds
Posted by Jason Wang 1 year, 2 months ago 
On Tue, Feb 28, 2023 at 10:25 PM Longpeng(Mike) <longpeng2@huawei.com> wrote:
>
> From: Longpeng <longpeng2@huawei.com>
>
> When updating ioeventfds, we need to iterate all address spaces and
> iterate all flat ranges of each address space. There is so much
> redundant process that a FlatView would be iterated for so many times
> during one commit (memory_region_transaction_commit).
>
> We can mark a FlatView as UPDATED and then skip it in the next iteration
> and clear the UPDATED flag at the end of the commit. The overhead can
> be significantly reduced.
>
> For example, a VM with 16 vdpa net devices and each one has 65 vectors,
> can reduce the time spent on memory_region_transaction_commit by 95%.
>
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>  include/exec/memory.h |  2 ++
>  softmmu/memory.c      | 28 +++++++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2e602a2fad..974eabf765 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1093,6 +1093,8 @@ struct FlatView {
>      unsigned nr_allocated;
>      struct AddressSpaceDispatch *dispatch;
>      MemoryRegion *root;
> +#define FLATVIEW_FLAG_IOEVENTFD_UPDATED (1 << 0)
> +    unsigned flags;
>  };
>
>  static inline FlatView *address_space_to_flatview(AddressSpace *as)
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 9d64efca26..71ff996712 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -815,6 +815,15 @@ FlatView *address_space_get_flatview(AddressSpace *as)
>      return view;
>  }
>
> +static void address_space_reset_view_flags(AddressSpace *as, unsigned mask)
> +{
> +    FlatView *view = address_space_get_flatview(as);
> +
> +    if (view->flags & mask) {
> +        view->flags &= ~mask;
> +    }
> +}
> +
>  static void address_space_update_ioeventfds(AddressSpace *as)
>  {
>      FlatView *view;
> @@ -825,6 +834,12 @@ static void address_space_update_ioeventfds(AddressSpace *as)
>      AddrRange tmp;
>      unsigned i;
>
> +    view = address_space_get_flatview(as);
> +    if (view->flags & FLATVIEW_FLAG_IOEVENTFD_UPDATED) {
> +        return;
> +    }
> +    view->flags |= FLATVIEW_FLAG_IOEVENTFD_UPDATED;
> +

Won't we lose the listener calls if multiple address spaces have the
same flatview?

Thanks

>      /*
>       * It is likely that the number of ioeventfds hasn't changed much, so use
>       * the previous size as the starting value, with some headroom to avoid
> @@ -833,7 +848,6 @@ static void address_space_update_ioeventfds(AddressSpace *as)
>      ioeventfd_max = QEMU_ALIGN_UP(as->ioeventfd_nb, 4);
>      ioeventfds = g_new(MemoryRegionIoeventfd, ioeventfd_max);
>
> -    view = address_space_get_flatview(as);
>      FOR_EACH_FLAT_RANGE(fr, view) {
>          for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
>              tmp = addrrange_shift(fr->mr->ioeventfds[i].addr,
> @@ -1086,6 +1100,15 @@ void memory_region_transaction_begin(void)
>      ++memory_region_transaction_depth;
>  }
>
> +static inline void address_space_update_ioeventfds_finish(void)
> +{
> +    AddressSpace *as;
> +
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        address_space_reset_view_flags(as, FLATVIEW_FLAG_IOEVENTFD_UPDATED);
> +    }
> +}
> +
>  void memory_region_transaction_commit(void)
>  {
>      AddressSpace *as;
> @@ -1106,12 +1129,14 @@ void memory_region_transaction_commit(void)
>              }
>              memory_region_update_pending = false;
>              ioeventfd_update_pending = false;
> +            address_space_update_ioeventfds_finish();
>              MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
>          } else if (ioeventfd_update_pending) {
>              QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>                  address_space_update_ioeventfds(as);
>              }
>              ioeventfd_update_pending = false;
> +            address_space_update_ioeventfds_finish();
>          }
>     }
>  }
> @@ -3076,6 +3101,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>      as->name = g_strdup(name ? name : "anonymous");
>      address_space_update_topology(as);
>      address_space_update_ioeventfds(as);
> +    address_space_reset_view_flags(as, FLATVIEW_FLAG_IOEVENTFD_UPDATED);
>  }
>
>  static void do_address_space_destroy(AddressSpace *as)
> --
> 2.23.0
>
Re: [PATCH] memory: avoid unnecessary iteration when updating ioeventfds
Posted by Peter Xu 1 year, 2 months ago 
On Wed, Mar 01, 2023 at 04:36:20PM +0800, Jason Wang wrote:
> On Tue, Feb 28, 2023 at 10:25 PM Longpeng(Mike) <longpeng2@huawei.com> wrote:
> >
> > From: Longpeng <longpeng2@huawei.com>
> >
> > When updating ioeventfds, we need to iterate all address spaces and
> > iterate all flat ranges of each address space. There is so much
> > redundant process that a FlatView would be iterated for so many times
> > during one commit (memory_region_transaction_commit).
> >
> > We can mark a FlatView as UPDATED and then skip it in the next iteration
> > and clear the UPDATED flag at the end of the commit. The overhead can
> > be significantly reduced.
> >
> > For example, a VM with 16 vdpa net devices and each one has 65 vectors,
> > can reduce the time spent on memory_region_transaction_commit by 95%.
> >
> > Signed-off-by: Longpeng <longpeng2@huawei.com>
> > ---
> >  include/exec/memory.h |  2 ++
> >  softmmu/memory.c      | 28 +++++++++++++++++++++++++++-
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 2e602a2fad..974eabf765 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1093,6 +1093,8 @@ struct FlatView {
> >      unsigned nr_allocated;
> >      struct AddressSpaceDispatch *dispatch;
> >      MemoryRegion *root;
> > +#define FLATVIEW_FLAG_IOEVENTFD_UPDATED (1 << 0)
> > +    unsigned flags;
> >  };
> >
> >  static inline FlatView *address_space_to_flatview(AddressSpace *as)
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 9d64efca26..71ff996712 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -815,6 +815,15 @@ FlatView *address_space_get_flatview(AddressSpace *as)
> >      return view;
> >  }
> >
> > +static void address_space_reset_view_flags(AddressSpace *as, unsigned mask)
> > +{
> > +    FlatView *view = address_space_get_flatview(as);
> > +
> > +    if (view->flags & mask) {
> > +        view->flags &= ~mask;
> > +    }
> > +}
> > +
> >  static void address_space_update_ioeventfds(AddressSpace *as)
> >  {
> >      FlatView *view;
> > @@ -825,6 +834,12 @@ static void address_space_update_ioeventfds(AddressSpace *as)
> >      AddrRange tmp;
> >      unsigned i;
> >
> > +    view = address_space_get_flatview(as);
> > +    if (view->flags & FLATVIEW_FLAG_IOEVENTFD_UPDATED) {
> > +        return;
> > +    }
> > +    view->flags |= FLATVIEW_FLAG_IOEVENTFD_UPDATED;
> > +
> 
> Won't we lose the listener calls if multiple address spaces have the
> same flatview?

I have the same concern with Jason.  I don't think it matters in reality,
since only address_space_io uses it so far. but it doesn't really look
reasonable and clean.

One other idea of optimizing ioeventfd update is we can add a per-AS
counter (ioeventfd_notifiers), increase if any eventfd_add|del is
registered in memory_listener_register(), and decrease when unregister.
Then address_space_update_ioeventfds() can be skipped completely if
ioeventfd_notifiers==0.

Side note: Jason, do you think we should drop vhost_eventfd_add|del?
They're all no-ops right now.

Thanks,

-- 
Peter Xu
Re: [PATCH] memory: avoid unnecessary iteration when updating ioeventfds
Posted by Jason Wang 1 year, 2 months ago 
On Mon, Mar 6, 2023 at 5:27 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Mar 01, 2023 at 04:36:20PM +0800, Jason Wang wrote:
> > On Tue, Feb 28, 2023 at 10:25 PM Longpeng(Mike) <longpeng2@huawei.com> wrote:
> > >
> > > From: Longpeng <longpeng2@huawei.com>
> > >
> > > When updating ioeventfds, we need to iterate all address spaces and
> > > iterate all flat ranges of each address space. There is so much
> > > redundant process that a FlatView would be iterated for so many times
> > > during one commit (memory_region_transaction_commit).
> > >
> > > We can mark a FlatView as UPDATED and then skip it in the next iteration
> > > and clear the UPDATED flag at the end of the commit. The overhead can
> > > be significantly reduced.
> > >
> > > For example, a VM with 16 vdpa net devices and each one has 65 vectors,
> > > can reduce the time spent on memory_region_transaction_commit by 95%.
> > >
> > > Signed-off-by: Longpeng <longpeng2@huawei.com>
> > > ---
> > >  include/exec/memory.h |  2 ++
> > >  softmmu/memory.c      | 28 +++++++++++++++++++++++++++-
> > >  2 files changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > index 2e602a2fad..974eabf765 100644
> > > --- a/include/exec/memory.h
> > > +++ b/include/exec/memory.h
> > > @@ -1093,6 +1093,8 @@ struct FlatView {
> > >      unsigned nr_allocated;
> > >      struct AddressSpaceDispatch *dispatch;
> > >      MemoryRegion *root;
> > > +#define FLATVIEW_FLAG_IOEVENTFD_UPDATED (1 << 0)
> > > +    unsigned flags;
> > >  };
> > >
> > >  static inline FlatView *address_space_to_flatview(AddressSpace *as)
> > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > index 9d64efca26..71ff996712 100644
> > > --- a/softmmu/memory.c
> > > +++ b/softmmu/memory.c
> > > @@ -815,6 +815,15 @@ FlatView *address_space_get_flatview(AddressSpace *as)
> > >      return view;
> > >  }
> > >
> > > +static void address_space_reset_view_flags(AddressSpace *as, unsigned mask)
> > > +{
> > > +    FlatView *view = address_space_get_flatview(as);
> > > +
> > > +    if (view->flags & mask) {
> > > +        view->flags &= ~mask;
> > > +    }
> > > +}
> > > +
> > >  static void address_space_update_ioeventfds(AddressSpace *as)
> > >  {
> > >      FlatView *view;
> > > @@ -825,6 +834,12 @@ static void address_space_update_ioeventfds(AddressSpace *as)
> > >      AddrRange tmp;
> > >      unsigned i;
> > >
> > > +    view = address_space_get_flatview(as);
> > > +    if (view->flags & FLATVIEW_FLAG_IOEVENTFD_UPDATED) {
> > > +        return;
> > > +    }
> > > +    view->flags |= FLATVIEW_FLAG_IOEVENTFD_UPDATED;
> > > +
> >
> > Won't we lose the listener calls if multiple address spaces have the
> > same flatview?
>
> I have the same concern with Jason.  I don't think it matters in reality,
> since only address_space_io uses it so far. but it doesn't really look
> reasonable and clean.

Yes, I think in the memory core it should not assume how the eventfds are used.

>
> One other idea of optimizing ioeventfd update is we can add a per-AS
> counter (ioeventfd_notifiers), increase if any eventfd_add|del is
> registered in memory_listener_register(), and decrease when unregister.
> Then address_space_update_ioeventfds() can be skipped completely if
> ioeventfd_notifiers==0.
>
> Side note: Jason, do you think we should drop vhost_eventfd_add|del?
> They're all no-ops right now.

I think so.

Thanks

>
> Thanks,
>
> --
> Peter Xu
>
Re: [PATCH] memory: avoid unnecessary iteration when updating ioeventfds
Posted by David Hildenbrand 1 year, 2 months ago 
On 28.02.23 15:25, Longpeng(Mike) via wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> When updating ioeventfds, we need to iterate all address spaces and
> iterate all flat ranges of each address space. There is so much
> redundant process that a FlatView would be iterated for so many times
> during one commit (memory_region_transaction_commit).
> 
> We can mark a FlatView as UPDATED and then skip it in the next iteration
> and clear the UPDATED flag at the end of the commit. The overhead can
> be significantly reduced.
> 
> For example, a VM with 16 vdpa net devices and each one has 65 vectors,
> can reduce the time spent on memory_region_transaction_commit by 95%.
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>   include/exec/memory.h |  2 ++
>   softmmu/memory.c      | 28 +++++++++++++++++++++++++++-
>   2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2e602a2fad..974eabf765 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1093,6 +1093,8 @@ struct FlatView {
>       unsigned nr_allocated;
>       struct AddressSpaceDispatch *dispatch;
>       MemoryRegion *root;
> +#define FLATVIEW_FLAG_IOEVENTFD_UPDATED (1 << 0)
> +    unsigned flags;
>   };
>   
>   static inline FlatView *address_space_to_flatview(AddressSpace *as)
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 9d64efca26..71ff996712 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -815,6 +815,15 @@ FlatView *address_space_get_flatview(AddressSpace *as)
>       return view;
>   }
>   
> +static void address_space_reset_view_flags(AddressSpace *as, unsigned mask)
> +{
> +    FlatView *view = address_space_get_flatview(as);
> +
> +    if (view->flags & mask) {
> +        view->flags &= ~mask;
> +    }

Just do it unconditionally.

Unfortunately, I cannot comment on the bigger picture, but it does look 
good to me.

-- 
Thanks,

David / dhildenb

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.