| Message ID | 20210325150735.1098387-2-groug@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci | expand |
On Thu, Mar 25, 2021 at 04:07:28PM +0100, Greg Kurz wrote: > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 5728a681b27d..98ed552e001c 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1848,13 +1848,25 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr); > * @match_data: whether to match against @data, instead of just @addr > * @data: the data to match against the guest write > * @e: event notifier to be triggered when @addr, @size, and @data all match. > + * @transaction: whether to start a transaction for the change "start" is unclear. Does it begin a transaction and return with the transaction unfinished? I think instead the function performs the eventfd addition within a transaction. It would be nice to clarify this. > **/ > -void memory_region_add_eventfd(MemoryRegion *mr, > - hwaddr addr, > - unsigned size, > - bool match_data, > - uint64_t data, > - EventNotifier *e); > +void memory_region_add_eventfd_full(MemoryRegion *mr, > + hwaddr addr, > + unsigned size, > + bool match_data, > + uint64_t data, > + EventNotifier *e, > + bool transaction); > + > +static inline void memory_region_add_eventfd(MemoryRegion *mr, > + hwaddr addr, > + unsigned size, > + bool match_data, > + uint64_t data, > + EventNotifier *e) > +{ > + memory_region_add_eventfd_full(mr, addr, size, match_data, data, e, true); > +} > > /** > * memory_region_del_eventfd: Cancel an eventfd. > @@ -1868,13 +1880,25 @@ void memory_region_add_eventfd(MemoryRegion *mr, > * @match_data: whether to match against @data, instead of just @addr > * @data: the data to match against the guest write > * @e: event notifier to be triggered when @addr, @size, and @data all match. > + * @transaction: whether to start a transaction for the change Same here.
On Mon, 29 Mar 2021 18:03:49 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Thu, Mar 25, 2021 at 04:07:28PM +0100, Greg Kurz wrote: > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index 5728a681b27d..98ed552e001c 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -1848,13 +1848,25 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr); > > * @match_data: whether to match against @data, instead of just @addr > > * @data: the data to match against the guest write > > * @e: event notifier to be triggered when @addr, @size, and @data all match. > > + * @transaction: whether to start a transaction for the change > > "start" is unclear. Does it begin a transaction and return with the > transaction unfinished? I think instead the function performs the > eventfd addition within a transaction. It would be nice to clarify this. > What about: * @transaction: if true, the eventfd is added within a nested transaction, * if false, it is up to the caller to ensure this is called * within a transaction. > > **/ > > -void memory_region_add_eventfd(MemoryRegion *mr, > > - hwaddr addr, > > - unsigned size, > > - bool match_data, > > - uint64_t data, > > - EventNotifier *e); > > +void memory_region_add_eventfd_full(MemoryRegion *mr, > > + hwaddr addr, > > + unsigned size, > > + bool match_data, > > + uint64_t data, > > + EventNotifier *e, > > + bool transaction); > > + > > +static inline void memory_region_add_eventfd(MemoryRegion *mr, > > + hwaddr addr, > > + unsigned size, > > + bool match_data, > > + uint64_t data, > > + EventNotifier *e) > > +{ > > + memory_region_add_eventfd_full(mr, addr, size, match_data, data, e, true); > > +} > > > > /** > > * memory_region_del_eventfd: Cancel an eventfd. > > @@ -1868,13 +1880,25 @@ void memory_region_add_eventfd(MemoryRegion *mr, > > * @match_data: whether to match against @data, instead of just @addr > > * @data: the data to match against the guest write > > * @e: event notifier to be triggered when @addr, @size, and @data all match. > > + * @transaction: whether to start a transaction for the change > > Same here. and: * @transaction: if true, the eventfd is cancelled within a nested transaction, * if false, it is up to the caller to ensure this is called * within a transaction. ?
On Tue, Mar 30, 2021 at 09:47:49AM +0200, Greg Kurz wrote: > On Mon, 29 Mar 2021 18:03:49 +0100 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Thu, Mar 25, 2021 at 04:07:28PM +0100, Greg Kurz wrote: > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > > index 5728a681b27d..98ed552e001c 100644 > > > --- a/include/exec/memory.h > > > +++ b/include/exec/memory.h > > > @@ -1848,13 +1848,25 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr); > > > * @match_data: whether to match against @data, instead of just @addr > > > * @data: the data to match against the guest write > > > * @e: event notifier to be triggered when @addr, @size, and @data all match. > > > + * @transaction: whether to start a transaction for the change > > > > "start" is unclear. Does it begin a transaction and return with the > > transaction unfinished? I think instead the function performs the > > eventfd addition within a transaction. It would be nice to clarify this. > > > > What about: > > * @transaction: if true, the eventfd is added within a nested transaction, > * if false, it is up to the caller to ensure this is called > * within a transaction. Sounds good, thanks! Stefan
diff --git a/include/exec/memory.h b/include/exec/memory.h index 5728a681b27d..98ed552e001c 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1848,13 +1848,25 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr); * @match_data: whether to match against @data, instead of just @addr * @data: the data to match against the guest write * @e: event notifier to be triggered when @addr, @size, and @data all match. + * @transaction: whether to start a transaction for the change **/ -void memory_region_add_eventfd(MemoryRegion *mr, - hwaddr addr, - unsigned size, - bool match_data, - uint64_t data, - EventNotifier *e); +void memory_region_add_eventfd_full(MemoryRegion *mr, + hwaddr addr, + unsigned size, + bool match_data, + uint64_t data, + EventNotifier *e, + bool transaction); + +static inline void memory_region_add_eventfd(MemoryRegion *mr, + hwaddr addr, + unsigned size, + bool match_data, + uint64_t data, + EventNotifier *e) +{ + memory_region_add_eventfd_full(mr, addr, size, match_data, data, e, true); +} /** * memory_region_del_eventfd: Cancel an eventfd. @@ -1868,13 +1880,25 @@ void memory_region_add_eventfd(MemoryRegion *mr, * @match_data: whether to match against @data, instead of just @addr * @data: the data to match against the guest write * @e: event notifier to be triggered when @addr, @size, and @data all match. + * @transaction: whether to start a transaction for the change */ -void memory_region_del_eventfd(MemoryRegion *mr, - hwaddr addr, - unsigned size, - bool match_data, - uint64_t data, - EventNotifier *e); +void memory_region_del_eventfd_full(MemoryRegion *mr, + hwaddr addr, + unsigned size, + bool match_data, + uint64_t data, + EventNotifier *e, + bool transaction); + +static inline void memory_region_del_eventfd(MemoryRegion *mr, + hwaddr addr, + unsigned size, + bool match_data, + uint64_t data, + EventNotifier *e) +{ + memory_region_del_eventfd_full(mr, addr, size, match_data, data, e, true); +} /** * memory_region_add_subregion: Add a subregion to a container. diff --git a/softmmu/memory.c b/softmmu/memory.c index d4493ef9e430..1b1942d521cc 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2341,12 +2341,13 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr) static bool userspace_eventfd_warning; -void memory_region_add_eventfd(MemoryRegion *mr, - hwaddr addr, - unsigned size, - bool match_data, - uint64_t data, - EventNotifier *e) +void memory_region_add_eventfd_full(MemoryRegion *mr, + hwaddr addr, + unsigned size, + bool match_data, + uint64_t data, + EventNotifier *e, + bool transaction) { MemoryRegionIoeventfd mrfd = { .addr.start = int128_make64(addr), @@ -2367,7 +2368,9 @@ void memory_region_add_eventfd(MemoryRegion *mr, if (size) { adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_TE); } - memory_region_transaction_begin(); + if (transaction) { + memory_region_transaction_begin(); + } for (i = 0; i < mr->ioeventfd_nb; ++i) { if (memory_region_ioeventfd_before(&mrfd, &mr->ioeventfds[i])) { break; @@ -2380,15 +2383,18 @@ void memory_region_add_eventfd(MemoryRegion *mr, sizeof(*mr->ioeventfds) * (mr->ioeventfd_nb-1 - i)); mr->ioeventfds[i] = mrfd; ioeventfd_update_pending |= mr->enabled; - memory_region_transaction_commit(); + if (transaction) { + memory_region_transaction_commit(); + } } -void memory_region_del_eventfd(MemoryRegion *mr, - hwaddr addr, - unsigned size, - bool match_data, - uint64_t data, - EventNotifier *e) +void memory_region_del_eventfd_full(MemoryRegion *mr, + hwaddr addr, + unsigned size, + bool match_data, + uint64_t data, + EventNotifier *e, + bool transaction) { MemoryRegionIoeventfd mrfd = { .addr.start = int128_make64(addr), @@ -2402,7 +2408,9 @@ void memory_region_del_eventfd(MemoryRegion *mr, if (size) { adjust_endianness(mr, &mrfd.data, size_memop(size) | MO_TE); } - memory_region_transaction_begin(); + if (transaction) { + memory_region_transaction_begin(); + } for (i = 0; i < mr->ioeventfd_nb; ++i) { if (memory_region_ioeventfd_equal(&mrfd, &mr->ioeventfds[i])) { break; @@ -2415,7 +2423,9 @@ void memory_region_del_eventfd(MemoryRegion *mr, mr->ioeventfds = g_realloc(mr->ioeventfds, sizeof(*mr->ioeventfds)*mr->ioeventfd_nb + 1); ioeventfd_update_pending |= mr->enabled; - memory_region_transaction_commit(); + if (transaction) { + memory_region_transaction_commit(); + } } static void memory_region_update_container_subregions(MemoryRegion *subregion)
Each addition or deletion of an eventfd happens in its own MR transaction. This doesn't scale well with multiqueue devices that do 1:1 queue:vCPU mapping (e.g. virtio-scsi-pci or virtio-blk-pci) : these devices typically create at least one eventfd per queue and memory_region_transaction_commit(), which is called during commit, also loops on eventfds, resulting in a quadratic time complexity. This calls for batching : a device should be able to add or delete its eventfds in a single transaction. Prepare ground for this by introducing extended versions of memory_region_add_eventfd() and memory_region_del_eventfd() that take an extra bool argument to control if a transaction should be started or not. No behavior change at this point. Signed-off-by: Greg Kurz <groug@kaod.org> --- include/exec/memory.h | 48 ++++++++++++++++++++++++++++++++----------- softmmu/memory.c | 42 ++++++++++++++++++++++--------------- 2 files changed, 62 insertions(+), 28 deletions(-)