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

[David Vrabel <david.vrabel@citrix.com>]

Can you trim your replies?  There's no need to quote the whole patch.

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.

>> +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.

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?

>> +/*
>> + * 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.

>> +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.

David