[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 3/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup



On 7/8/2016 10:35 AM, Jan Beulich wrote:
On 06.07.16 at 17:51, <czuzu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.