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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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