| Message ID | 1349280245-16341-9-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
Avi Kivity <avi@redhat.com> writes: > Many listeners don't need to respond to all MemoryListener callbacks; > provide suitable defaults instead. > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > memory.c | 15 +++++++++++++++ > memory.h | 21 +++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/memory.c b/memory.c > index b58b97c..efefcb8 100644 > --- a/memory.c > +++ b/memory.c > @@ -1514,6 +1514,21 @@ void memory_listener_unregister(MemoryListener *listener) > QTAILQ_REMOVE(&memory_listeners, listener, link); > } > > +void memory_listener_default_global(MemoryListener *listener) > +{ > +} > + > +void memory_listener_default_section(MemoryListener *listener, > + MemoryRegionSection *section) > +{ > +} > + > +void memory_listener_default_eventfd(MemoryListener *listener, > + MemoryRegionSection *section, > + bool match_data, uint64_t data, EventNotifier *e) > +{ > +} > + > void address_space_init(AddressSpace *as, MemoryRegion *root) > { > memory_region_transaction_begin(); > diff --git a/memory.h b/memory.h > index 46bc5e1..0ef95cb 100644 > --- a/memory.h > +++ b/memory.h > @@ -223,6 +223,27 @@ struct MemoryListener { > QTAILQ_ENTRY(MemoryListener) link; > }; > > +#define MEMORY_LISTENER_DEFAULT_OPS \ > + .begin = memory_listener_default_global, \ > + .commit = memory_listener_default_global, \ > + .region_add = memory_listener_default_section, \ > + .region_del = memory_listener_default_section, \ > + .region_nop = memory_listener_default_section, \ > + .log_start = memory_listener_default_section, \ > + .log_stop = memory_listener_default_section, \ > + .log_sync = memory_listener_default_section, \ > + .log_global_start = memory_listener_default_global, \ > + .log_global_stop = memory_listener_default_global, \ > + .eventfd_add = memory_listener_default_eventfd, \ > + .eventfd_del = memory_listener_default_eventfd \ > + > +void memory_listener_default_global(MemoryListener *listener); > +void memory_listener_default_section(MemoryListener *listener, > + MemoryRegionSection *section); > +void memory_listener_default_eventfd(MemoryListener *listener, > + MemoryRegionSection *section, > + bool match_data, uint64_t data, EventNotifier *e); > + > /** > * memory_region_init: Initialize a memory region > * I think it'd be nicer to check for NULL when invoking the functions in the memory core. Then you avoid the exported stub functions entirely. Regards, Anthony Liguori > -- > 1.7.12
On 10/04/2012 04:05 PM, Anthony Liguori wrote: > Avi Kivity <avi@redhat.com> writes: > >> Many listeners don't need to respond to all MemoryListener callbacks; >> provide suitable defaults instead. >> >> +#define MEMORY_LISTENER_DEFAULT_OPS \ >> + .begin = memory_listener_default_global, \ >> + .commit = memory_listener_default_global, \ >> + .region_add = memory_listener_default_section, \ >> + .region_del = memory_listener_default_section, \ >> + .region_nop = memory_listener_default_section, \ >> + .log_start = memory_listener_default_section, \ >> + .log_stop = memory_listener_default_section, \ >> + .log_sync = memory_listener_default_section, \ >> + .log_global_start = memory_listener_default_global, \ >> + .log_global_stop = memory_listener_default_global, \ >> + .eventfd_add = memory_listener_default_eventfd, \ >> + .eventfd_del = memory_listener_default_eventfd \ >> + >> +void memory_listener_default_global(MemoryListener *listener); >> +void memory_listener_default_section(MemoryListener *listener, >> + MemoryRegionSection *section); >> +void memory_listener_default_eventfd(MemoryListener *listener, >> + MemoryRegionSection *section, >> + bool match_data, uint64_t data, EventNotifier *e); >> + >> /** >> * memory_region_init: Initialize a memory region >> * > > I think it'd be nicer to check for NULL when invoking the functions in > the memory core. > > Then you avoid the exported stub functions entirely. Yes, that's the common style, but I happen not to like the extra check, both from a performance point of view (doesn't apply here of course) and from a readability point of view.
Avi Kivity <avi@redhat.com> writes: > On 10/04/2012 04:05 PM, Anthony Liguori wrote: >> Avi Kivity <avi@redhat.com> writes: >> >>> Many listeners don't need to respond to all MemoryListener callbacks; >>> provide suitable defaults instead. >>> > >>> +#define MEMORY_LISTENER_DEFAULT_OPS \ >>> + .begin = memory_listener_default_global, \ >>> + .commit = memory_listener_default_global, \ >>> + .region_add = memory_listener_default_section, \ >>> + .region_del = memory_listener_default_section, \ >>> + .region_nop = memory_listener_default_section, \ >>> + .log_start = memory_listener_default_section, \ >>> + .log_stop = memory_listener_default_section, \ >>> + .log_sync = memory_listener_default_section, \ >>> + .log_global_start = memory_listener_default_global, \ >>> + .log_global_stop = memory_listener_default_global, \ >>> + .eventfd_add = memory_listener_default_eventfd, \ >>> + .eventfd_del = memory_listener_default_eventfd \ >>> + >>> +void memory_listener_default_global(MemoryListener *listener); >>> +void memory_listener_default_section(MemoryListener *listener, >>> + MemoryRegionSection *section); >>> +void memory_listener_default_eventfd(MemoryListener *listener, >>> + MemoryRegionSection *section, >>> + bool match_data, uint64_t data, EventNotifier *e); >>> + >>> /** >>> * memory_region_init: Initialize a memory region >>> * >> >> I think it'd be nicer to check for NULL when invoking the functions in >> the memory core. >> >> Then you avoid the exported stub functions entirely. > > Yes, that's the common style, but I happen not to like the extra check, > both from a performance point of view (doesn't apply here of course) and > from a readability point of view. The trouble with your approach is that it introduced a subtle behavior based on ordering. IOW: MemoryListenerOps foo = { MEMORY_LISTENER_DEFAULT_OPS, .log_sync = ..., }; vs. MemoryListenerOps foo = { .log_sync = ..., MEMORY_LISTENER_DEFAULT_OPS, }; Both compile fine but have potentially difficult to debug differences. Relying on zero-initialization eliminates the possibility of this problem. Regards, Anthony Liguori > > -- > error compiling committee.c: too many arguments to function
On 10/09/2012 05:14 PM, Anthony Liguori wrote: >>> >>> I think it'd be nicer to check for NULL when invoking the functions in >>> the memory core. >>> >>> Then you avoid the exported stub functions entirely. >> >> Yes, that's the common style, but I happen not to like the extra check, >> both from a performance point of view (doesn't apply here of course) and >> from a readability point of view. > > The trouble with your approach is that it introduced a subtle behavior > based on ordering. IOW: > > MemoryListenerOps foo = { > MEMORY_LISTENER_DEFAULT_OPS, > .log_sync = ..., > }; > > vs. > > MemoryListenerOps foo = { > .log_sync = ..., > MEMORY_LISTENER_DEFAULT_OPS, > }; > > Both compile fine but have potentially difficult to debug differences. > Relying on zero-initialization eliminates the possibility of this problem. > I don't think this is likely (esp. as the bad behaviour would be the code not working at all) but i will update this for the next version.
Avi Kivity <avi@redhat.com> writes: > On 10/09/2012 05:14 PM, Anthony Liguori wrote: >>>> >>>> I think it'd be nicer to check for NULL when invoking the functions in >>>> the memory core. >>>> >>>> Then you avoid the exported stub functions entirely. >>> >>> Yes, that's the common style, but I happen not to like the extra check, >>> both from a performance point of view (doesn't apply here of course) and >>> from a readability point of view. >> >> The trouble with your approach is that it introduced a subtle behavior >> based on ordering. IOW: >> >> MemoryListenerOps foo = { >> MEMORY_LISTENER_DEFAULT_OPS, >> .log_sync = ..., >> }; >> >> vs. >> >> MemoryListenerOps foo = { >> .log_sync = ..., >> MEMORY_LISTENER_DEFAULT_OPS, >> }; >> >> Both compile fine but have potentially difficult to debug differences. >> Relying on zero-initialization eliminates the possibility of this problem. >> > > I don't think this is likely (esp. as the bad behaviour would be the > code not working at all) but i will update this for the next version. Thanks. That's the only minor grip I had with the series. It's really a great cleanup! Regards, Anthony Liguori > > -- > error compiling committee.c: too many arguments to function
diff --git a/memory.c b/memory.c index b58b97c..efefcb8 100644 --- a/memory.c +++ b/memory.c @@ -1514,6 +1514,21 @@ void memory_listener_unregister(MemoryListener *listener) QTAILQ_REMOVE(&memory_listeners, listener, link); } +void memory_listener_default_global(MemoryListener *listener) +{ +} + +void memory_listener_default_section(MemoryListener *listener, + MemoryRegionSection *section) +{ +} + +void memory_listener_default_eventfd(MemoryListener *listener, + MemoryRegionSection *section, + bool match_data, uint64_t data, EventNotifier *e) +{ +} + void address_space_init(AddressSpace *as, MemoryRegion *root) { memory_region_transaction_begin(); diff --git a/memory.h b/memory.h index 46bc5e1..0ef95cb 100644 --- a/memory.h +++ b/memory.h @@ -223,6 +223,27 @@ struct MemoryListener { QTAILQ_ENTRY(MemoryListener) link; }; +#define MEMORY_LISTENER_DEFAULT_OPS \ + .begin = memory_listener_default_global, \ + .commit = memory_listener_default_global, \ + .region_add = memory_listener_default_section, \ + .region_del = memory_listener_default_section, \ + .region_nop = memory_listener_default_section, \ + .log_start = memory_listener_default_section, \ + .log_stop = memory_listener_default_section, \ + .log_sync = memory_listener_default_section, \ + .log_global_start = memory_listener_default_global, \ + .log_global_stop = memory_listener_default_global, \ + .eventfd_add = memory_listener_default_eventfd, \ + .eventfd_del = memory_listener_default_eventfd \ + +void memory_listener_default_global(MemoryListener *listener); +void memory_listener_default_section(MemoryListener *listener, + MemoryRegionSection *section); +void memory_listener_default_eventfd(MemoryListener *listener, + MemoryRegionSection *section, + bool match_data, uint64_t data, EventNotifier *e); + /** * memory_region_init: Initialize a memory region *
Many listeners don't need to respond to all MemoryListener callbacks; provide suitable defaults instead. Signed-off-by: Avi Kivity <avi@redhat.com> --- memory.c | 15 +++++++++++++++ memory.h | 21 +++++++++++++++++++++ 2 files changed, 36 insertions(+)