[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 16/16] xen/events: use the FIFO-based ABI if available
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? Actually, is it even possible to have guests using different event mechanisms on the same system? + + if (i >= MAX_EVENT_ARRAY_PAGES) + return -EINVAL; + + while (i >= event_array_pages) { + void *array_page; + struct evtchn_expand_array expand_array; + + /* Might already have a page if we've resumed. */ + array_page = event_array[event_array_pages]; + if (!array_page) { + array_page = (void *)__get_free_page(GFP_KERNEL); + if (array_page == NULL) + goto error; + event_array[event_array_pages] = array_page; + } + + /* Mask all events in this page before adding it. */ + init_array_page(array_page); + + expand_array.array_gfn = virt_to_mfn(array_page); + + ret = HYPERVISOR_event_channel_op(EVTCHNOP_expand_array, &expand_array); + if (ret < 0) + goto error; + + event_array_pages++;Should this increment happen in the 'if(!array_page)' clause?No. event_array_pages is the number of pages Xen is aware of. Note how we zero it when resuming on a new domain with the FIFO-based ABI initially disabled.+ } + return 0; + + error: + if (event_array_pages == 0) + panic("xen: unable to expand event array with initial page (%d)\n", ret); + else + printk(KERN_ERR "xen: unable to expand event array (%d)\n", ret); + 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()? -boris _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |