Re: [Xen-devel] [PATCH 8/8] evtchn: add FIFO-based event channel hypercalls and port ops

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Aug 19, 2013 at 11:32:29AM +0100, David Vrabel wrote:
> Can you trim your replies?  There's no need to quote the whole patch.
> 

Sorry, forgot to do that.

> On 16/08/13 17:33, Wei Liu wrote:
> >> +    evtchn_fifo_destroy(d);
> >> +
> > 
> > Don't need to put this inside event_lock critical region?
> 
> No, but it does need to be reordered to be after freeing all the event
> channel buckets.
> 

Yes, please reorder that.

> >> +static int setup_event_array(struct domain *d)
> >> +{
> >> +    if ( d->evtchn_fifo )
> >> +        return 0;
> >> +
> >> +    d->evtchn_fifo = xzalloc(struct evtchn_fifo_domain);
> > 
> > struct evtchn_fifo_domain is very big because it contains event_array
> > which has 1024 elements, however most domains will not use all 2^17
> > ports. Could you add in dynamically allocation for event_array?
> 
> It has
> 
>    2^17 / (PAGE_SIZE / sizeof(event_word_t))
> =  131072 / (4096 / 4)
> =  128 entries.
> 

Ah yes, my calculation was wrong (just came back to Cambridge and still
had jet lag when I reviewed your patch, sorry).

> So sizeof(struct evtchn_fifo_domain) == 2052 bytes (so 1 page).
> 
> Would realloc'ing this as the number of event channels grows actually
> save a useful amount of memory?
> 

It depends on how the allocator works? But I think 1 page is not a big
deal. I raised that based on my wrong calculation so my comment is
effectively void.

> >> +/*
> >> + * Some ports may already be bound, bind them to the correct VCPU so
> >> + * they have a valid queue.
> >> + *
> >> + * Note: any events that are currently pending will not be resent and
> >> + * will be lost.
> >> + */
> > 
> > Can this be fixed? I presume you can check the state of the port and set
> > it to pending in the new ABI when necessary? As you've got hold of the
> > event lock at this stage it should be easy to synchronize the state?
> 
> Why?  Guests should setup the ABI before binding any events if they
> don't want to risk losing any.
> 

Then can you please add the above line to the comment?

> >> +static void cleanup_event_array(struct domain *d)
> >> +{
> >> +    unsigned i;
> >> +
> >> +    if ( d->evtchn_fifo == NULL )
> >> +        return;
> >> +
> >> +    for ( i = 0; i < EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES; i++ )
> >> +    {
> >> +        unmap_guest_page(d->evtchn_fifo->event_array[i].page,
> >> +                         d->evtchn_fifo->event_array[i].virt);
> > 
> > Suggest reset page and virt to NULL / 0, just like you did for 
> > cleanup_control_block.
> 
> Why? d->evtchn_fifo is about to be freed.
> 
> >> +    }
> >> +    xfree(d->evtchn_fifo);
> > 
> > Same for evtchn_fifo.
> 
> Why? The domain is being destroyed.

I just prefer everything to be in known state if possible. Just my
$0.02.


Wei.

> 
> David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel