Patchwork [RFC,v1,08/22] memory: provide defaults for MemoryListener operations

login
register
mail settings
Submitter Avi Kivity
Date Oct. 3, 2012, 4:03 p.m.
Message ID <1349280245-16341-9-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/188864/
State New
Headers show

Comments

Avi Kivity - Oct. 3, 2012, 4:03 p.m.
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(+)
Anthony Liguori - Oct. 4, 2012, 2:05 p.m.
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
Avi Kivity - Oct. 4, 2012, 2:29 p.m.
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.
Anthony Liguori - Oct. 9, 2012, 3:14 p.m.
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
Avi Kivity - Oct. 9, 2012, 3:28 p.m.
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.
Anthony Liguori - Oct. 9, 2012, 6:34 p.m.
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

Patch

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
  *