From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 16/16] xen/events: use the FIFO-based ABI if available Date: Tue, 15 Oct 2013 16:39:03 -0400 Message-ID: <525DA7E7.8020009@oracle.com> References: <1381236555-27493-1-git-send-email-david.vrabel@citrix.com> <1381236555-27493-17-git-send-email-david.vrabel@citrix.com> <525C4663.70907@oracle.com> <525D906C.5080401@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <525D906C.5080401@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel Cc: Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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