On 7/8/2016 10:35 AM, Jan Beulich wrote:
On 06.07.16 at 17:51, <cz...@bitdefender.com> wrote:
@@ -492,8 +493,12 @@ int vcpu_initialise(struct vcpu *v)
void vcpu_destroy(struct vcpu *v)
{
- xfree(v->arch.vm_event);
- v->arch.vm_event = NULL;
+ if ( unlikely(v->arch.vm_event) )
+ {
+ xfree(v->arch.vm_event->emul_read_data);
+ xfree(v->arch.vm_event);
+ v->arch.vm_event = NULL;
+ }
Considering the repeat of this pattern ...
@@ -52,8 +58,25 @@ void vm_event_cleanup_domain(struct domain *d)
for_each_vcpu ( d, v )
{
- xfree(v->arch.vm_event);
- v->arch.vm_event = NULL;
+ if ( likely(!v->arch.vm_event) )
+ continue;
+
+ /*
+ * Only xfree the entire arch_vm_event if write_data was fully handled.
+ * Otherwise defer entire xfree until domain/vcpu destroyal.
+ */
+ if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) )
+ {
+ xfree(v->arch.vm_event->emul_read_data);
+ xfree(v->arch.vm_event);
+ v->arch.vm_event = NULL;
+ continue;
+ }
... here, please consider making this another helper (inline?) function.
Yeah, I'm sending a separate patch today which will invalidate some of
these changes (then a v4 above that one).
+ /* write_data not fully handled, only xfree emul_read_data */
Comment style again (and more below).
Ack, assuming you mean 'capital letter, end with dot'.
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v,
unsigned int port)
/* Clean up on domain destruction */
void vm_event_cleanup(struct domain *d)
{
+ struct vcpu *v;
+
#ifdef CONFIG_HAS_MEM_PAGING
if ( d->vm_event->paging.ring_page )
{
@@ -560,6 +562,16 @@ void vm_event_cleanup(struct domain *d)
(void)vm_event_disable(d, &d->vm_event->share);
}
#endif
+
+ for_each_vcpu ( d, v )
+ {
+ if ( unlikely(v->arch.vm_event) )
+ {
+ /* vm_event->emul_read_data freed in vm_event_cleanup_domain */
Perhaps worthwhile adding a respective ASSERT()?
Good point, ack.
+static inline bool_t vm_event_vcpu_initialised(struct vcpu *v)
+{
+ return (v->arch.vm_event && v->arch.vm_event->emul_read_data);
+}
+
+static inline bool_t vm_event_domain_initialised(struct domain *d)
+{
+ return (d->max_vcpus && d->vcpu[0] &&
+ vm_event_vcpu_initialised(d->vcpu[0]));
+}
Both functions' parameters should be const. Pointless parentheses
in both return statements.
Jan
Ack (although the parenthesis were there strictly for aesthetics, but
that's subjective).
Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel