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

[David Vrabel <dvrabel@xxxxxxxxxx>]

On 24/09/2013 16:50, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 13, 2013 at 06:00:01PM +0100, David Vrabel wrote:
>> +
>> +  error:
>> +    if (event_array_pages == 0)
>> +            panic("xen: unable to expand event array with initial page 
>> (%d)\n", ret);
> 
> Shouldn't we just printk and return a negative value so we can use
> the old style event channels?

No, once you've called EVTCHNOP_init_control for at least one VCPU the
hypervisor has switched ABIs and there is (deliberatly[*]) no mechanism
to switch back.

[*] it greatly simplifies the hypervisor ABI and implementation if we
don't have to unwind a half initialized switch over.  This failure case
will never happen in practise as the hypervisor will run out of domheap
pages for the guest memory before it runs out of global mapping space.

>> +    else
>> +            printk(KERN_ERR "xen: unable to expand event array (%d)\n", 
>> ret);
>> +    free_unused_array_pages();
>> +    return ret;
>> +}
>> +
>> +static void fifo_bind_to_cpu(struct irq_info *info, int cpu)
>> +{
>> +    /* no-op */
> 
> Perhaps you can also say: "The event array is shared between all of the
> guests VCPUs." (from design-E).

That's not relevant here.  This is a no-op because there is no guest
side state necessary for tracking which VCPU an event channel is bound
to -- this is all done by the per-VCPU queues.

>> +static void fifo_unmask(int port)
>> +{
>> +    unsigned int cpu = get_cpu();
>> +    bool do_hypercall = false;
>> +    bool evtchn_pending = false;
>> +
>> +    BUG_ON(!irqs_disabled());
>> +
>> +    if (unlikely((cpu != cpu_from_evtchn(port))))
>> +            do_hypercall = true;
> 
> Since the event array is shared across all of the vCPUs is this still
> needed?

The shared event array isn't relevant (the 2-level ABI also has shared
pending and mask arrays).

But, since a hypercall is always required if an event is pending then we
can omit the above check -- it's not possible (as in the 2-level case)
to optimize out the hypercall if we're already on the VCPU.

>> +static uint32_t clear_linked(volatile event_word_t *word)
>> +{
>> +    event_word_t n, o, w;
> 
> How about just 'new', 'old', 'temp' ?

The letters match the design doc.

>> +
>> +static void fifo_handle_events(int cpu)
>> +{
>> +    struct evtchn_fifo_control_block *control_block;
>> +    uint32_t ready;
>> +    int q;
>> +
>> +    control_block = per_cpu(cpu_control_block, cpu);
>> +
>> +    ready = xchg(&control_block->ready, 0);
>> +
>> +    while (ready & EVTCHN_FIFO_READY_MASK) {
>> +            q = find_first_bit(BM(&ready), EVTCHN_FIFO_MAX_QUEUES);
>> +            consume_one_event(cpu, control_block, q, &ready);
>> +            ready |= xchg(&control_block->ready, 0);
> 
> Say priority 0 is where VIRQ_TIMER is set and the TIMER is set to
> be incredibly short.
> 
> Couldn't this cause a scenario where the guest clears the ready in
> consume_one_event (clear_bit(0, BM(ready)) and the hypervisor
> at the same time can set the bit. We then process the IRQ (virq_timer).
> 
> Then we get to the xchg here and end up with the ready having the
> priority 0 bit set again. 
> 
> And then loop again and again..

Don't do that then.  This is no different to the behaviour of hardware
interrupts.

>> +            ret = HYPERVISOR_event_channel_op(EVTCHNOP_init_control,
>> +                                              &init_control);
>> +            if (ret < 0)
>> +                    BUG();
> 
> So if this is run after migration to an older hypevisor won't we
> blow up here? Shouldn't we just exit with an return and use the
> old style classis events?

Migrating to older hypervisors has never been supported.  You can only
go forwards.

>> +static int __cpuinit fifo_cpu_notification(struct notifier_block *self,
>> +                                       unsigned long action, void *hcpu)
>> +{
>> +    int cpu = (long)hcpu;
>> +    int ret = 0;
>> +
>> +    switch (action) {
>> +    case CPU_UP_PREPARE:
>> +            if (!per_cpu(cpu_control_block, cpu))
>> +                    ret = fifo_init_control_block(cpu);
>> +            break;
>> +    default:
>> +            break;
>> +    }
>> +    return ret < 0 ? NOTIFY_BAD : NOTIFY_OK;
> 
> I think you should return NOTIFY_OK.
> 
> Say you do this:
> 
> 1) Launch guest on a new hypervisor (which has FIFO ABI)
> 2) Bring down all CPUs but one.
> 3) Migrate to an older hypervisor (no FIFI ABI)
> 4) Bring up all of the CPUs.
> 
> We shouldn't return NOTIFY_BAD b/c the hypervisor we are resuming
> on is too old. We should just continue on without the FIFO mechanism
> in place.

Again, migrating to an older hypervisor has never been supported.  Also,
once we have switched to the FIFO-based ABI for one VCPU we cannot go
back and it's better to fail to bring up the VCPU than have one that
cannot receive events.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel