Re: [PATCH 06/16] xen/events: move 2-level specific code into its own file

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 10/08/2013 08:49 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> In preparation for alternative event channel ABIs, move all the
> functions accessing the shared data structures into their own file.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   drivers/xen/events/Makefile          |    1 +
>   drivers/xen/events/events.c          |  372 +---------------------------------
>   drivers/xen/events/events_2l.c       |  322 +++++++++++++++++++++++++++++
>   drivers/xen/events/events_internal.h |   74 +++++++
>   4 files changed, 408 insertions(+), 361 deletions(-)
>   create mode 100644 drivers/xen/events/events_2l.c
>   create mode 100644 drivers/xen/events/events_internal.h
...

> - */
>   static void __xen_evtchn_do_upcall(void)
>   {
> -	int start_word_idx, start_bit_idx;
> -	int word_idx, bit_idx;
> -	int i, irq;
> -	int cpu = get_cpu();
> -	struct shared_info *s = HYPERVISOR_shared_info;
>   	struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
> +	int cpu = get_cpu();
>   	unsigned count;
>   
>   	do {
> -		xen_ulong_t pending_words;
> -		xen_ulong_t pending_bits;
> -		struct irq_desc *desc;
> -
>   		vcpu_info->evtchn_upcall_pending = 0;
>   
>   		if (__this_cpu_inc_return(xed_nesting_count) - 1)
>   			goto out;
>   
> -		/*
> -		 * Master flag must be cleared /before/ clearing
> -		 * selector flag. xchg_xen_ulong must contain an
> -		 * appropriate barrier.
> -		 */
> -		if ((irq = per_cpu(virq_to_irq, cpu)[VIRQ_TIMER]) != -1) {
> -			int evtchn = evtchn_from_irq(irq);
> -			word_idx = evtchn / BITS_PER_LONG;
> -			pending_bits = evtchn % BITS_PER_LONG;
> -			if (active_evtchns(cpu, s, word_idx) & (1ULL << pending_bits)) {
> -				desc = irq_to_desc(irq);
> -				if (desc)
> -					generic_handle_irq_desc(irq, desc);
> -			}
> -		}

This chunk dealing with timer interrupt has been completely removed. I 
see that you have a later patch that raises priority of VIRQ_TIMER. If 
that patch is replacement for what this chunk used to do, should you be 
removing this code in that patch?

Also, it appears that you are not simply moving code around. There are 
some changes to logic (for example this one) and it should be reflected 
in the commit message.

> -
> -		pending_words = xchg_xen_ulong(&vcpu_info->evtchn_pending_sel, 0);
> -
> -		start_word_idx = __this_cpu_read(current_word_idx);
> -		start_bit_idx = __this_cpu_read(current_bit_idx);
> -

...

> -
> +
> +void unmask_evtchn(int port)
> +{
> +	struct shared_info *s = HYPERVISOR_shared_info;
> +	unsigned int cpu = get_cpu();
> +	int do_hypercall = 0, evtchn_pending = 0;
> +
> +	BUG_ON(!irqs_disabled());
> +
> +	if (unlikely((cpu != cpu_from_evtchn(port))))
> +		do_hypercall = 1;
> +	else {
> +		sync_clear_bit(port, BM(&s->evtchn_mask[0]));
> +		evtchn_pending = sync_test_bit(port, BM(&s->evtchn_pending[0]));
> +
> +		if (unlikely(evtchn_pending && xen_hvm_domain()))
> +			do_hypercall = 1;

Why are you no longer setting back evtchn_mask bit in the 'if' clause? 
Wouldn't this make you possibly miss a pending event from Xen's 
evtchn_unmask()?

You also removed a comment that talks about why we clear the bit. Is 
this comment no longer relevant?

-boris

> +	}
> +
> +	/* Slow path (hypercall) if this is a non-local port or if this is
> +	 * an hvm domain and an event is pending (hvm domains don't have
> +	 * their own implementation of irq_enable). */
> +	if (do_hypercall) {
> +		struct evtchn_unmask unmask = { .port = port };
> +		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
> +	} else {
> +		struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
> +
> +		/*
> +		 * The following is basically the equivalent of
> +		 * 'hw_resend_irq'. Just like a real IO-APIC we 'lose
> +		 * the interrupt edge' if the channel is masked.
> +		 */
> +		if (evtchn_pending &&
> +		    !sync_test_and_set_bit(port / BITS_PER_EVTCHN_WORD,
> +					   BM(&vcpu_info->evtchn_pending_sel)))
> +			vcpu_info->evtchn_upcall_pending = 1;
> +	}
> +
> +	put_cpu();
> +}
> +