[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |