Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751550AbdFEKOu (ORCPT ); Mon, 5 Jun 2017 06:14:50 -0400 Received: from smtp.ctxuk.citrix.com ([185.25.65.24]:50651 "EHLO SMTP.EU.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751290AbdFEKOt (ORCPT ); Mon, 5 Jun 2017 06:14:49 -0400 X-IronPort-AV: E=Sophos;i="5.39,300,1493683200"; d="scan'208";a="47211729" Subject: Re: [PATCH] xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online VCPU To: Boris Ostrovsky , , CC: References: <1496414988-12878-1-git-send-email-anoob.soman@citrix.com> <363cb97a-7dc1-ae4f-da93-30e7658cef00@oracle.com> From: Anoob Soman Message-ID: <5a5d9355-34fc-57aa-825c-81123f6bb74e@citrix.com> Date: Mon, 5 Jun 2017 11:14:42 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <363cb97a-7dc1-ae4f-da93-30e7658cef00@oracle.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-ClientProxiedBy: FTLPEX02CAS03.citrite.net (10.13.99.94) To AMSPEX02CL02.citrite.net (10.69.22.126) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2596 Lines: 79 On 02/06/17 17:24, Boris Ostrovsky wrote: >> >> static int set_affinity_irq(struct irq_data *data, const struct cpumask *dest, >> bool force) >> diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c >> index 10f1ef5..1192f24 100644 >> --- a/drivers/xen/evtchn.c >> +++ b/drivers/xen/evtchn.c >> @@ -58,6 +58,8 @@ >> #include >> #include >> >> +static DEFINE_PER_CPU(int, bind_last_selected_cpu); > This should be moved into evtchn_bind_interdom_next_vcpu() since that's > the only place referencing it. Sure, I will do it. > > Why is it a percpu variable BTW? Wouldn't making it global result in > better interrupt distribution? The reason for percpu instead of global, was to avoid locking. We can have a global variable (last_cpu) without locking, but value of last_cpu wont be consistent, without locks. Moreover, since irq_affinity is also used in the calculation of cpu to bind, having a percpu or global wouldn't really matter, as the result (selected_cpu) is more likely to be random (because different irqs can have different affinity). What do you guys suggest. > >> + >> struct per_user_data { >> struct mutex bind_mutex; /* serialize bind/unbind operations */ >> struct rb_root evtchns; >> @@ -421,6 +423,36 @@ static void evtchn_unbind_from_user(struct per_user_data *u, >> del_evtchn(u, evtchn); >> } >> >> +static void evtchn_bind_interdom_next_vcpu(int evtchn) >> +{ >> + unsigned int selected_cpu, irq; >> + struct irq_desc *desc = NULL; >> + unsigned long flags; >> + >> + irq = irq_from_evtchn(evtchn); >> + desc = irq_to_desc(irq); >> + >> + if (!desc) >> + return; >> + >> + raw_spin_lock_irqsave(&desc->lock, flags); >> + selected_cpu = this_cpu_read(bind_last_selected_cpu); >> + selected_cpu = cpumask_next_and(selected_cpu, >> + desc->irq_common_data.affinity, cpu_online_mask); >> + >> + if (unlikely(selected_cpu >= nr_cpu_ids)) >> + selected_cpu = cpumask_first_and(desc->irq_common_data.affinity, >> + cpu_online_mask); >> + >> + raw_spin_unlock_irqrestore(&desc->lock, flags); > I think if you follow Juergen's suggestion of wrapping everything into > irq_enable/disable you can drop the lock altogether (assuming you keep > bind_last_selected_cpu percpu). > > -boris > I think we would still require spin_lock(). spin_lock is for irq_desc. >> + this_cpu_write(bind_last_selected_cpu, selected_cpu); >> + >> + local_irq_disable(); >> + /* unmask expects irqs to be disabled */ >> + xen_rebind_evtchn_to_cpu(evtchn, selected_cpu); >> + local_irq_enable(); >> +} >> + >>