From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v8 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq (v8) Date: Mon, 27 Oct 2014 14:00:10 -0400 Message-ID: <20141027180010.GB12989@laptop.dumpdata.com> References: <20141024015858.GB28850@laptop.dumpdata.com> <544A41970200007800041D63@mail.emea.novell.com> <20141024205544.GA19814@laptop.dumpdata.com> <544E1F590200007800042517@mail.emea.novell.com> <544E2105.1000907@citrix.com> <544E33B902000078000425F0@mail.emea.novell.com> <544E27D6.1040305@citrix.com> <544E397F0200007800042651@mail.emea.novell.com> <20141027170115.GC11893@laptop.dumpdata.com> <544E8283.1030907@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Xioai-0003Pp-1Q for xen-devel@lists.xenproject.org; Mon, 27 Oct 2014 18:00:28 +0000 Content-Disposition: inline In-Reply-To: <544E8283.1030907@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: keir@xen.org, ian.campbell@citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, Jan Beulich , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Mon, Oct 27, 2014 at 05:36:03PM +0000, Andrew Cooper wrote: > On 27/10/14 17:01, Konrad Rzeszutek Wilk wrote: > > On Mon, Oct 27, 2014 at 11:24:31AM +0000, Jan Beulich wrote: > >>>>> On 27.10.14 at 12:09, wrote: > >>> Can it ever be the case that we are waiting for a remote pcpu to run its > >>> softirq handler? If so, the time spent looping here could be up to 1 > >>> scheduling timeslice in the worst case, and 30ms is a very long time to > >>> wait. > >> Good point - I think this can be the case. But there seems to be a > >> simple counter measure: The first time we get to this point, send an > >> event check IPI to the CPU in question (or in the worst case > >> broadcast one if the CPU can't be determined in a race free way). > > I can either do this using the wrapper: > > > > if ( pt_pirq_softirq_active(pirq_dpci) ) > > { > > spin_unlock(&d->event_lock); > > if ( pirq_dpci->cpu >= 0 ) > > { > > cpu_raise_softirq(pirq_dpci->cpu, HVM_DPCI_SOFTIRQ); > > pirq_dpci->cpu = -1; > > } > > cpu_relax(); > > goto restart; > > > > Ought to do it (cpu_raise_softirq will exit out if > > the 'pirq_dpci->cpu == smp_processor_id()'). It also has some batching checks > > so that we won't do the IPI if we are in the middle of IPI-ing already > > an CPU. > > > > Or just write it out (and bypass some of the checks 'cpu_raise_softirq' > > has): > > > > if ( pt_pirq_softirq_active(pirq_dpci) ) > > { > > spin_unlock(&d->event_lock); > > if ( pirq_dpci->cpu >= 0 && pirq_dpci->cpu != smp_processor_id() ) > > { > > smp_send_event_check_cpu(pirq_dpci->cpu); > > pirq_dpci->cpu = -1; > > } > > cpu_relax(); > > goto restart; > > > > > > Note: > > > > The 'cpu' is stashed whenever 'raise_softirq_for' has been called. > > > > You need to send at most 1 IPI, or you will be pointlessly spamming the > target pcpu. Therefore, a blind goto restart seems ill-advised. Right. That is what it does (it sets pirq_dpci->cpu to a negative value so that we don't try to spam the target). > > The second version doesn't necessarily set HVM_DPCI_SOFTIRQ pending, It does not have to as the target has already done so. That is because the ->cpu value is set in raise_softirq_for which also sets the HVM_DPIC_SOFTIRQ pending. > while the first version suffers a risk of the softirq being caught in a > batch. Correct. > > Furthermore, with mwait support, the IPI is elided completely, which is > completely wrong in this situation. Wait, where did that come from? If we use mwaits IPIs are ignored? Oh, you mean with the 'batching' support. >O > Therefore, I think you need to manually set the HVM_DPCI_SOFTIRQ bit, > then forcibly send the IPI. OK, so the second (smp_send_event_check_cpu). And the bit is already set - but I will add a comment explaining that. > > ~Andrew >