Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751192AbdFEOtu (ORCPT ); Mon, 5 Jun 2017 10:49:50 -0400 Received: from smtp.eu.citrix.com ([185.25.65.24]:13337 "EHLO SMTP.EU.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751112AbdFEOtt (ORCPT ); Mon, 5 Jun 2017 10:49:49 -0400 X-IronPort-AV: E=Sophos;i="5.39,300,1493683200"; d="scan'208";a="47228364" Subject: Re: [Xen-devel] [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> <5a5d9355-34fc-57aa-825c-81123f6bb74e@citrix.com> From: Anoob Soman Message-ID: <0c2a3a4b-e442-8c8c-6a71-6f9972ff29fc@citrix.com> Date: Mon, 5 Jun 2017 15:49: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: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-ClientProxiedBy: FTLPEX02CAS01.citrite.net (10.13.99.120) 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: 2046 Lines: 44 On 05/06/17 15:10, Boris Ostrovsky wrote: >> 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. > Doesn't initial affinity (which is what we expect here since irqbalance > has not run yet) typically cover all guest VCPUs? Yes, initial affinity covers all online VCPUs. But there is a small chance that initial affinity might change, before evtch_bind_interdom_next_vcpu is called. For example, I could run a script to change irq affinity, just when irq sysfs entry appears. This is the reason that I thought it would be sensible (based on your suggestion) to include irq_affinity to calculate the next VCPU. If you think, changing irq_affinity between request_irq() and evtch_bind_interdom_next_vcpu is virtually impossible, then we can drop affinity and just use cpu_online_mask. >> >> I think we would still require spin_lock(). spin_lock is for irq_desc. > If you are trying to protect affinity then it may well change after you > drop the lock. > > In fact, don't you have a race here? If we offline a VCPU we will (by > way of cpu_disable_common()->fixup_irqs()) update affinity to reflect > that a CPU is gone and there is a chance that xen_rebind_evtchn_to_cpu() > will happen after that. > > So, contrary to what I said earlier ;-) not only do you need the lock, > but you should hold it across xen_rebind_evtchn_to_cpu() call. Does this > make sense? Yes, you are correct. .irq_set_affinity pretty much does the same thing. The code will now looks like this. raw_spin_lock_irqsave(lock, flags); percpu read select_cpu percpu write xen_rebind_evtchn_to_cpu(evtchn, selected_cpu) raw_spin_unlock_irqsave(lock, flags);