| Message ID | 20240910120100.9460-1-anisinha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] memory: notify hypervisor of all eventfds during listener (de)registration | expand |
Hi, Ani, On Tue, Sep 10, 2024 at 05:31:00PM +0530, Ani Sinha wrote: > When a new listener for an address space is registered, the hypervisor must be > informed of all existing eventfds for that address space by calling > eventfd_add() for that listener. Similarly, when a listener is de-registered > from an address space, the hypervisor must be informed of all existing eventfds > for that address space with a call to eventfd_del(). > > Same is also true for coalesced io. Send coalesced io add/del listener > notifications if any flatrage for the address space registered with the > listener intersects with any coalesced io range. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Ani Sinha <anisinha@redhat.com> This is only to make the API clean, as I see that only kvm uses either of the features (while kvm never registers during VM running). Am I right? I'd like to double check whether we need to copy stable, or whether there's a real bug. > --- > system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 77 insertions(+) > > changelog: > v2: tags added, indentation fixed, commit log fixed, code cleanup. > > diff --git a/system/memory.c b/system/memory.c > index 5e6eb459d5..8379e086fb 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -941,6 +941,38 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as) > } > } > > +static void > +flat_range_coalesced_io_notify_listener_add_del(FlatRange *fr, > + MemoryRegionSection *mrs, > + MemoryListener *listener, > + AddressSpace *as, bool add) > +{ > + CoalescedMemoryRange *cmr; > + MemoryRegion *mr = fr->mr; > + AddrRange tmp; > + > + QTAILQ_FOREACH(cmr, &mr->coalesced, link) { > + tmp = addrrange_shift(cmr->addr, > + int128_sub(fr->addr.start, > + int128_make64(fr->offset_in_region))); > + > + if (!addrrange_intersects(tmp, fr->addr)) { > + return; > + } > + tmp = addrrange_intersection(tmp, fr->addr); > + > + if (add) { Do we need to check ->coalesced_io_add != NULL here? I wonder whether this could crash if some dynamic listener got inserted (with coalesced list non-empty). > + listener->coalesced_io_add(listener, mrs, > + int128_get64(tmp.start), > + int128_get64(tmp.size)); > + } else { > + listener->coalesced_io_del(listener, mrs, Same. > + int128_get64(tmp.start), > + int128_get64(tmp.size)); > + } > + } > +} > + > static void address_space_update_topology_pass(AddressSpace *as, > const FlatView *old_view, > const FlatView *new_view, > @@ -3015,8 +3047,10 @@ void memory_global_dirty_log_stop(unsigned int flags) > static void listener_add_address_space(MemoryListener *listener, > AddressSpace *as) > { > + unsigned i; > FlatView *view; > FlatRange *fr; > + MemoryRegionIoeventfd *fd; > > if (listener->begin) { > listener->begin(listener); > @@ -3041,10 +3075,31 @@ static void listener_add_address_space(MemoryListener *listener, > if (listener->region_add) { > listener->region_add(listener, §ion); > } > + > + /* send coalesced io add notifications */ > + flat_range_coalesced_io_notify_listener_add_del(fr, §ion, > + listener, as, true); > + > if (fr->dirty_log_mask && listener->log_start) { > listener->log_start(listener, §ion, 0, fr->dirty_log_mask); > } > } > + > + /* > + * register all eventfds for this address space for the newly registered > + * listener. > + */ > + for (i = 0; i < as->ioeventfd_nb; i++) { > + fd = &as->ioeventfds[i]; > + MemoryRegionSection section = (MemoryRegionSection) { > + .fv = address_space_to_flatview(as), Nitpick: I don't think it'll be anything different if we hold BQL anyway, but.. might be good to use "view" directly here, as we're holding a refcount there via address_space_get_flatview(). While it's not crystal clear otherwise (when without knowing BQL held) that address_space_to_flatview() will return the same flatview. > + .offset_within_address_space = int128_get64(fd->addr.start), > + .size = fd->addr.size, > + }; > + listener->eventfd_add(listener, §ion, > + fd->match_data, fd->data, fd->e); Check ->eventfd_add != NULL? > + } > + > if (listener->commit) { > listener->commit(listener); > } > @@ -3054,8 +3109,10 @@ static void listener_add_address_space(MemoryListener *listener, > static void listener_del_address_space(MemoryListener *listener, > AddressSpace *as) > { > + unsigned i; > FlatView *view; > FlatRange *fr; > + MemoryRegionIoeventfd *fd; > > if (listener->begin) { > listener->begin(listener); > @@ -3067,10 +3124,30 @@ static void listener_del_address_space(MemoryListener *listener, > if (fr->dirty_log_mask && listener->log_stop) { > listener->log_stop(listener, §ion, fr->dirty_log_mask, 0); > } > + > + /* send coalesced io del notifications */ > + flat_range_coalesced_io_notify_listener_add_del(fr, §ion, > + listener, as, false); > if (listener->region_del) { > listener->region_del(listener, §ion); > } > } > + > + /* > + * de-register all eventfds for this address space for the current > + * listener. > + */ > + for (i = 0; i < as->ioeventfd_nb; i++) { > + fd = &as->ioeventfds[i]; > + MemoryRegionSection section = (MemoryRegionSection) { > + .fv = address_space_to_flatview(as), Same. > + .offset_within_address_space = int128_get64(fd->addr.start), > + .size = fd->addr.size, > + }; > + listener->eventfd_del(listener, §ion, > + fd->match_data, fd->data, fd->e); Check eventfd_del != NULL? Thanks, > + } > + > if (listener->commit) { > listener->commit(listener); > } > -- > 2.42.0 >
> On 18 Sep 2024, at 12:28 AM, Peter Xu <peterx@redhat.com> wrote: > > Hi, Ani, > > On Tue, Sep 10, 2024 at 05:31:00PM +0530, Ani Sinha wrote: >> When a new listener for an address space is registered, the hypervisor must be >> informed of all existing eventfds for that address space by calling >> eventfd_add() for that listener. Similarly, when a listener is de-registered >> from an address space, the hypervisor must be informed of all existing eventfds >> for that address space with a call to eventfd_del(). >> >> Same is also true for coalesced io. Send coalesced io add/del listener >> notifications if any flatrage for the address space registered with the >> listener intersects with any coalesced io range. >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Ani Sinha <anisinha@redhat.com> > > This is only to make the API clean, as I see that only kvm uses either of > the features (while kvm never registers during VM running). Am I right? Yes currently there is no use case for deregistering a listener and then re-registering. Hence there seems to be a gap in the implementation. I am working on something that would require de-register and re-register so we will need to fix this. > > I'd like to double check whether we need to copy stable, or whether there's > a real bug. I think this is not a stable material as up until now, people will not hit this gap in the implementation. > >> --- >> system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 77 insertions(+) >> >> changelog: >> v2: tags added, indentation fixed, commit log fixed, code cleanup. >> >> diff --git a/system/memory.c b/system/memory.c >> index 5e6eb459d5..8379e086fb 100644 >> --- a/system/memory.c >> +++ b/system/memory.c >> @@ -941,6 +941,38 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as) >> } >> } >> >> +static void >> +flat_range_coalesced_io_notify_listener_add_del(FlatRange *fr, >> + MemoryRegionSection *mrs, >> + MemoryListener *listener, >> + AddressSpace *as, bool add) >> +{ >> + CoalescedMemoryRange *cmr; >> + MemoryRegion *mr = fr->mr; >> + AddrRange tmp; >> + >> + QTAILQ_FOREACH(cmr, &mr->coalesced, link) { >> + tmp = addrrange_shift(cmr->addr, >> + int128_sub(fr->addr.start, >> + int128_make64(fr->offset_in_region))); >> + >> + if (!addrrange_intersects(tmp, fr->addr)) { >> + return; >> + } >> + tmp = addrrange_intersection(tmp, fr->addr); >> + >> + if (add) { > > Do we need to check ->coalesced_io_add != NULL here? I wonder whether this > could crash if some dynamic listener got inserted (with coalesced list > non-empty). > >> + listener->coalesced_io_add(listener, mrs, >> + int128_get64(tmp.start), >> + int128_get64(tmp.size)); >> + } else { >> + listener->coalesced_io_del(listener, mrs, > > Same. > >> + int128_get64(tmp.start), >> + int128_get64(tmp.size)); >> + } >> + } >> +} >> + >> static void address_space_update_topology_pass(AddressSpace *as, >> const FlatView *old_view, >> const FlatView *new_view, >> @@ -3015,8 +3047,10 @@ void memory_global_dirty_log_stop(unsigned int flags) >> static void listener_add_address_space(MemoryListener *listener, >> AddressSpace *as) >> { >> + unsigned i; >> FlatView *view; >> FlatRange *fr; >> + MemoryRegionIoeventfd *fd; >> >> if (listener->begin) { >> listener->begin(listener); >> @@ -3041,10 +3075,31 @@ static void listener_add_address_space(MemoryListener *listener, >> if (listener->region_add) { >> listener->region_add(listener, §ion); >> } >> + >> + /* send coalesced io add notifications */ >> + flat_range_coalesced_io_notify_listener_add_del(fr, §ion, >> + listener, as, true); >> + >> if (fr->dirty_log_mask && listener->log_start) { >> listener->log_start(listener, §ion, 0, fr->dirty_log_mask); >> } >> } >> + >> + /* >> + * register all eventfds for this address space for the newly registered >> + * listener. >> + */ >> + for (i = 0; i < as->ioeventfd_nb; i++) { >> + fd = &as->ioeventfds[i]; >> + MemoryRegionSection section = (MemoryRegionSection) { >> + .fv = address_space_to_flatview(as), > > Nitpick: I don't think it'll be anything different if we hold BQL anyway, > but.. might be good to use "view" directly here, as we're holding a > refcount there via address_space_get_flatview(). > > While it's not crystal clear otherwise (when without knowing BQL held) that > address_space_to_flatview() will return the same flatview. > >> + .offset_within_address_space = int128_get64(fd->addr.start), >> + .size = fd->addr.size, >> + }; >> + listener->eventfd_add(listener, §ion, >> + fd->match_data, fd->data, fd->e); > > Check ->eventfd_add != NULL? > >> + } >> + >> if (listener->commit) { >> listener->commit(listener); >> } >> @@ -3054,8 +3109,10 @@ static void listener_add_address_space(MemoryListener *listener, >> static void listener_del_address_space(MemoryListener *listener, >> AddressSpace *as) >> { >> + unsigned i; >> FlatView *view; >> FlatRange *fr; >> + MemoryRegionIoeventfd *fd; >> >> if (listener->begin) { >> listener->begin(listener); >> @@ -3067,10 +3124,30 @@ static void listener_del_address_space(MemoryListener *listener, >> if (fr->dirty_log_mask && listener->log_stop) { >> listener->log_stop(listener, §ion, fr->dirty_log_mask, 0); >> } >> + >> + /* send coalesced io del notifications */ >> + flat_range_coalesced_io_notify_listener_add_del(fr, §ion, >> + listener, as, false); >> if (listener->region_del) { >> listener->region_del(listener, §ion); >> } >> } >> + >> + /* >> + * de-register all eventfds for this address space for the current >> + * listener. >> + */ >> + for (i = 0; i < as->ioeventfd_nb; i++) { >> + fd = &as->ioeventfds[i]; >> + MemoryRegionSection section = (MemoryRegionSection) { >> + .fv = address_space_to_flatview(as), > > Same. > >> + .offset_within_address_space = int128_get64(fd->addr.start), >> + .size = fd->addr.size, >> + }; >> + listener->eventfd_del(listener, §ion, >> + fd->match_data, fd->data, fd->e); > > Check eventfd_del != NULL? > > Thanks, All of the above suggestions are great. I will fix them in a re-spin. Thanks Peter. > >> + } >> + >> if (listener->commit) { >> listener->commit(listener); >> } >> -- >> 2.42.0 >> > > -- > Peter Xu
diff --git a/system/memory.c b/system/memory.c index 5e6eb459d5..8379e086fb 100644 --- a/system/memory.c +++ b/system/memory.c @@ -941,6 +941,38 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as) } } +static void +flat_range_coalesced_io_notify_listener_add_del(FlatRange *fr, + MemoryRegionSection *mrs, + MemoryListener *listener, + AddressSpace *as, bool add) +{ + CoalescedMemoryRange *cmr; + MemoryRegion *mr = fr->mr; + AddrRange tmp; + + QTAILQ_FOREACH(cmr, &mr->coalesced, link) { + tmp = addrrange_shift(cmr->addr, + int128_sub(fr->addr.start, + int128_make64(fr->offset_in_region))); + + if (!addrrange_intersects(tmp, fr->addr)) { + return; + } + tmp = addrrange_intersection(tmp, fr->addr); + + if (add) { + listener->coalesced_io_add(listener, mrs, + int128_get64(tmp.start), + int128_get64(tmp.size)); + } else { + listener->coalesced_io_del(listener, mrs, + int128_get64(tmp.start), + int128_get64(tmp.size)); + } + } +} + static void address_space_update_topology_pass(AddressSpace *as, const FlatView *old_view, const FlatView *new_view, @@ -3015,8 +3047,10 @@ void memory_global_dirty_log_stop(unsigned int flags) static void listener_add_address_space(MemoryListener *listener, AddressSpace *as) { + unsigned i; FlatView *view; FlatRange *fr; + MemoryRegionIoeventfd *fd; if (listener->begin) { listener->begin(listener); @@ -3041,10 +3075,31 @@ static void listener_add_address_space(MemoryListener *listener, if (listener->region_add) { listener->region_add(listener, §ion); } + + /* send coalesced io add notifications */ + flat_range_coalesced_io_notify_listener_add_del(fr, §ion, + listener, as, true); + if (fr->dirty_log_mask && listener->log_start) { listener->log_start(listener, §ion, 0, fr->dirty_log_mask); } } + + /* + * register all eventfds for this address space for the newly registered + * listener. + */ + for (i = 0; i < as->ioeventfd_nb; i++) { + fd = &as->ioeventfds[i]; + MemoryRegionSection section = (MemoryRegionSection) { + .fv = address_space_to_flatview(as), + .offset_within_address_space = int128_get64(fd->addr.start), + .size = fd->addr.size, + }; + listener->eventfd_add(listener, §ion, + fd->match_data, fd->data, fd->e); + } + if (listener->commit) { listener->commit(listener); } @@ -3054,8 +3109,10 @@ static void listener_add_address_space(MemoryListener *listener, static void listener_del_address_space(MemoryListener *listener, AddressSpace *as) { + unsigned i; FlatView *view; FlatRange *fr; + MemoryRegionIoeventfd *fd; if (listener->begin) { listener->begin(listener); @@ -3067,10 +3124,30 @@ static void listener_del_address_space(MemoryListener *listener, if (fr->dirty_log_mask && listener->log_stop) { listener->log_stop(listener, §ion, fr->dirty_log_mask, 0); } + + /* send coalesced io del notifications */ + flat_range_coalesced_io_notify_listener_add_del(fr, §ion, + listener, as, false); if (listener->region_del) { listener->region_del(listener, §ion); } } + + /* + * de-register all eventfds for this address space for the current + * listener. + */ + for (i = 0; i < as->ioeventfd_nb; i++) { + fd = &as->ioeventfds[i]; + MemoryRegionSection section = (MemoryRegionSection) { + .fv = address_space_to_flatview(as), + .offset_within_address_space = int128_get64(fd->addr.start), + .size = fd->addr.size, + }; + listener->eventfd_del(listener, §ion, + fd->match_data, fd->data, fd->e); + } + if (listener->commit) { listener->commit(listener); }
When a new listener for an address space is registered, the hypervisor must be informed of all existing eventfds for that address space by calling eventfd_add() for that listener. Similarly, when a listener is de-registered from an address space, the hypervisor must be informed of all existing eventfds for that address space with a call to eventfd_del(). Same is also true for coalesced io. Send coalesced io add/del listener notifications if any flatrage for the address space registered with the listener intersects with any coalesced io range. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Ani Sinha <anisinha@redhat.com> --- system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) changelog: v2: tags added, indentation fixed, commit log fixed, code cleanup.