Re: [RFC PATCH 3/3] Xen: implement 3-level event channel routines.

[David Vrabel <david.vrabel@citrix.com>]

On 31/12/12 18:38, Wei Liu wrote:
> 

Changeset description?

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  arch/x86/xen/enlighten.c              |    7 +
>  drivers/xen/events.c                  |  419 +++++++++++++++++++++++++++++++--
>  include/xen/events.h                  |    2 +
>  include/xen/interface/event_channel.h |   24 ++
>  include/xen/interface/xen.h           |    2 +-
>  5 files changed, 437 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index bc893e7..f471881 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -43,6 +43,7 @@
>  #include <xen/hvm.h>
>  #include <xen/hvc-console.h>
>  #include <xen/acpi.h>
> +#include <xen/events.h>
>  
>  #include <asm/paravirt.h>
>  #include <asm/apic.h>
> @@ -195,6 +196,9 @@ void xen_vcpu_restore(void)
>  		    HYPERVISOR_vcpu_op(VCPUOP_up, cpu, NULL))
>  			BUG();
>  	}
> +
> +	if (evtchn_level_param == 3)
> +		xen_event_channel_setup_3level();

Why is this here?

>  }
>  
>  static void __init xen_banner(void)
> @@ -1028,6 +1032,9 @@ void xen_setup_vcpu_info_placement(void)
>  	for_each_possible_cpu(cpu)
>  		xen_vcpu_setup(cpu);
>  
> +	if (evtchn_level_param == 3)
> +		xen_event_channel_setup_3level();
> +

Why is this here instead of xen_init_IRQ()?

>  	/* xen_vcpu_setup managed to place the vcpu_info within the
>  	   percpu area for all cpus, so make use of it */
>  	if (have_vcpu_info_placement) {
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index f60ba76..adb94e9 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
[...]
> +
> +/* 2-level event channel does not use current_word_idx_l2 */
>  static DEFINE_PER_CPU(unsigned int, current_word_idx);
> +static DEFINE_PER_CPU(unsigned int, current_word_idx_l2);
>  static DEFINE_PER_CPU(unsigned int, current_bit_idx);

I suggest renaming these to current_word_idx_l3 and current_word_idx_l2.

The use of these variable really needs documentation, particularly why
they're used. I presume (but not really sure) that they're to ensure the
average event latency is constant independent of which channel it is.

> +
>  /*
>   * Mask out the i least significant bits of w
>   */
> @@ -1303,7 +1486,8 @@ static void __xen_evtchn_do_upcall_l2(void)
>  		if (__this_cpu_inc_return(xed_nesting_count) - 1)
>  			goto out;
>  
> -#ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86. */
> +#ifndef CONFIG_X86
> +		/* No need for a barrier -- XCHG is a barrier on x86. */
>  		/* Clear master flag /before/ clearing selector flag. */
>  		wmb();
>  #endif
> @@ -1392,6 +1576,155 @@ out:
>  	put_cpu();
>  }
>  
> +void __xen_evtchn_do_upcall_l3(void)

This is one of my least favourite functions...  A comment describing the
algorithm used here would be nice.

> @@ -1821,14 +2148,74 @@ static struct evtchn_ops evtchn_ops_l2 __read_mostly = {
>  	.xen_debug_interrupt = __xen_debug_interrupt_l2,
>  };
>  
> +static struct evtchn_ops evtchn_ops_l3 __read_mostly = {

const

> +	.active_evtchns = __active_evtchns_l3,
> +	.clear_evtchn = __clear_evtchn_l3,
> +	.set_evtchn = __set_evtchn_l3,
> +	.test_evtchn = __test_evtchn_l3,
> +	.mask_evtchn = __mask_evtchn_l3,
> +	.unmask_evtchn = __unmask_evtchn_l3,
> +	.is_masked = __is_masked_l3,
> +	.xen_evtchn_do_upcall = __xen_evtchn_do_upcall_l3,
> +	.xen_debug_interrupt = __xen_debug_interrupt_l3,
> +};
> +
> +int xen_event_channel_setup_3level(void)
> +{
> +	evtchn_register_nlevel_t reg;
> +	int i, nr_pages, cpu;
> +	unsigned long mfns[nr_cpu_ids];
> +	unsigned long offsets[nr_cpu_ids];

These arrays are too large for the stack if the domain has many VCPUs.
With 256 VCPUs this uses a page of stack.

> diff --git a/include/xen/interface/event_channel.h b/include/xen/interface/event_channel.h
> index f494292..f764d21 100644
> --- a/include/xen/interface/event_channel.h
> +++ b/include/xen/interface/event_channel.h
[...]

> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
> index c66e1ff..7cb9d8f 100644
> --- a/include/xen/interface/xen.h
> +++ b/include/xen/interface/xen.h
[,.,]

Put these in he patch sync'ing the headers.

David