Re: [PATCH 5/5] xen: events: use per-cpu variable for cpu_evtchn_mask

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

On Tue, 2010-10-26 at 15:36 +0100, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 25, 2010 at 05:23:33PM +0100, Ian Campbell wrote:
> > I can't see any reason why it isn't already.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  drivers/xen/events.c |   31 +++++++++++--------------------
> >  1 files changed, 11 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 9b58505..144ff72 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -110,19 +110,9 @@ static int *pirq_to_irq;
> >  static int nr_pirqs;
> >  
> >  static int *evtchn_to_irq;
> > -struct cpu_evtchn_s {
> > -	unsigned long bits[NR_EVENT_CHANNELS/BITS_PER_LONG];
> > -};
> >  
> > -static __initdata struct cpu_evtchn_s init_evtchn_mask = {
> > -	.bits[0 ... (NR_EVENT_CHANNELS/BITS_PER_LONG)-1] = ~0ul,
> > -};
> > -static struct cpu_evtchn_s *cpu_evtchn_mask_p = &init_evtchn_mask;
> > -
> > -static inline unsigned long *cpu_evtchn_mask(int cpu)
> > -{
> > -	return cpu_evtchn_mask_p[cpu].bits;
> > -}
> > +static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> > +		      cpu_evtchn_mask);
> >  
> >  /* Xen will never allocate port zero for any purpose. */
> >  #define VALID_EVTCHN(chn)	((chn) != 0)
> > @@ -301,7 +291,7 @@ static inline unsigned long active_evtchns(unsigned int cpu,
> >  					   unsigned int idx)
> >  {
> >  	return (sh->evtchn_pending[idx] &
> > -		cpu_evtchn_mask(cpu)[idx] &
> > +		per_cpu(cpu_evtchn_mask, cpu)[idx] &
> >  		~sh->evtchn_mask[idx]);
> >  }
> >  
> > @@ -314,8 +304,8 @@ static void bind_evtchn_to_cpu(unsigned int chn, unsigned int cpu)
> >  	cpumask_copy(irq_to_desc(irq)->affinity, cpumask_of(cpu));
> >  #endif
> >  
> > -	__clear_bit(chn, cpu_evtchn_mask(cpu_from_irq(irq)));
> > -	__set_bit(chn, cpu_evtchn_mask(cpu));
> > +	__clear_bit(chn, per_cpu(cpu_evtchn_mask, cpu_from_irq(irq)));
> > +	__set_bit(chn, per_cpu(cpu_evtchn_mask, cpu));
> >  
> >  	info_for_irq(irq)->cpu = cpu;
> >  }
> > @@ -332,7 +322,11 @@ static void init_evtchn_cpu_bindings(void)
> >  	}
> >  #endif
> >  
> > -	memset(cpu_evtchn_mask(0), ~0, sizeof(struct cpu_evtchn_s));
> > +	printk(KERN_CRIT "%s: CPU0 at %p size %zd\n", __func__,
> 
> If this is per_cpu, wouldn't the comment 'CPU0 at' be improper?

Oops, that was just debug anyway.

> Hmm, so we use percpu structure, but all of the event channel and such
> are set for CPU0. So what is the benefit of this, when the interrupts
> are _only_ happening on CPU0?

All event channels start off bound to VCPU0 so we need to initialise
cpu_evtchn_mask(0) to have all ones and leave all the other CPU's
bitmaps with all zeros.

> Oh, right, there is also the rebind cpu function somewhere so that the
> spinlock, timers, etc are allocated on other CPUs, right?

All evtchn's start off bound to VCPU 0 but the kernel can rebind as
necessary.

Ian.