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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Sep 24, 2013 at 05:48:58PM +0100, David Vrabel wrote:
> 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.

OK. And that is nicely mentioned in the design doc which I failed to spot
until now.
> 
> >> +	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.

Ah, gotcha.
> 
> >> +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.

Since you have to modify the design document anyway would it make
sense to alter it as well to have less cryptic variables?

> 
> >> +
> >> +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.

I was thinking there might be some mitigation techinque - and there is.

The Linux kernel would disable the IRQ, which hopefully means that this
defective port ends up being masked.


> 
> >> +		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.

OK.
> 
> >> +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.

OK.
> 
> David