[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 13/13] xen/events: use the FIFO-based ABI if available
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |