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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

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