Re: [PATCH 16/16] xen/events: use the FIFO-based ABI if available

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

On 15/10/13 21:39, Boris Ostrovsky wrote:
> On 10/15/2013 02:58 PM, David Vrabel wrote:
>> On 14/10/13 20:30, Boris Ostrovsky wrote:
>>> On 10/08/2013 08:49 AM, David Vrabel wrote:
>>>> @@ -1636,7 +1637,13 @@ void xen_callback_vector(void) {}
>>>>      void __init xen_init_IRQ(void)
>>>>    {
>>>> -    xen_evtchn_2l_init();
>>>> +    int ret;
>>>> +
>>>> +    ret = xen_evtchn_fifo_init();
>>>> +    if (ret < 0) {
>>>> +        printk(KERN_INFO "xen: falling back to n-level event
>>>> channels");
>>>> +        xen_evtchn_2l_init();
>>>> +    }
>>> Should we provide users with ability to choose which mechanism to use?
>>> Is there any advantage in staying with 2-level? Stability, I guess,
>>> would be one.
>> If someone can demonstrate a use case where 2-level is better then we
>> could consider an option.  I don't think we want options for new
>> software features just because they might be buggy.
> 
> I think we should always try to have a way to force old behavior when a
> new feature is introduced. If a user discovers a bug we don't want them
> to wait for a fix when a simpler solution is possible. I can see that
> having this option in the kernel may be a bit too much but is there at
> least an option to force 2-level in the hypervisor?

The majority of the risk in this series is the refactoring patches and
not the new ABI code so an option to disable it wouldn't really help.

Also, if we are not confidant that a feature is production ready that we
need knobs to turn it off then we shouldn't merge it.

> Actually, is it even possible to have guests using different event
> mechanisms on the same system?
> 

>>>> +    free_unused_array_pages();
>>> Do you need to clean up in the hypervisor as well?
>> There's noting to clean up in the hypervisor here.
>> free_unused_array_pages() is freeing array pages that Xen is not aware
>> of.
> 
> You made calls to ENTCHOP_expand_array that at some point calls
> map_domain_page_global(). Don't you need to do unmap_domain_page_global()?

This is done when Xen destroys the domain.

David