From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v8 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq (v8) Date: Mon, 27 Oct 2014 10:40:05 +0000 Message-ID: <544E2105.1000907@citrix.com> References: <1413911967-26886-1-git-send-email-konrad.wilk@oracle.com> <1413911967-26886-3-git-send-email-konrad.wilk@oracle.com> <5448E82702000078000415B5@mail.emea.novell.com> <20141024015858.GB28850@laptop.dumpdata.com> <544A41970200007800041D63@mail.emea.novell.com> <20141024205544.GA19814@laptop.dumpdata.com> <544E1F590200007800042517@mail.emea.novell.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 1Xihid-0004TD-4D for xen-devel@lists.xenproject.org; Mon, 27 Oct 2014 10:40:11 +0000 In-Reply-To: <544E1F590200007800042517@mail.emea.novell.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: Jan Beulich , Konrad Rzeszutek Wilk Cc: xen-devel@lists.xenproject.org, keir@xen.org, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org On 27/10/14 09:32, Jan Beulich wrote: >>>> On 24.10.14 at 22:55, wrote: >> On Fri, Oct 24, 2014 at 11:09:59AM +0100, Jan Beulich wrote: >>>>>> On 24.10.14 at 03:58, wrote: >>>> On Thu, Oct 23, 2014 at 10:36:07AM +0100, Jan Beulich wrote: >>>>>>>> On 21.10.14 at 19:19, wrote: >>>> Was not sure whether you prefer 'true' >>>> or 'false' instead of numbers - decided on numbers since most of the >> code-base >>>> uses numbers. >>> So far we don't use "true" and "false" in hypervisor code at all (or if >>> you spotted any such use, it slipped in by mistake). We ought to >>> consider switching to bool/true/false though. >> The dec_lzma2 had it. > With its private.h having > > #define false 0 > #define true 1 > >>> Apart from the indentation being wrong (case labels having the same >>> indentation as the switch()'s opening brace), this doesn't seem to be a >>> proper equivalent of the former code: There you cleared ->dom when >>> STATE_RUN regardless of STATE_SCHED. Leaving out the comments >>> I'd suggest >>> >>> switch ( cmpxchg(&pirq_dpci->state, STATE_SCHED, 0) ) >>> { >>> case STATE_SCHED: >>> put_domain(d); >>> case STATE_RUN: case STATE_SCHED|STATE_RUN: >> .. which made me realize that to testing of values as opposed to >> bit positions requires ditching the 'enum' and introducing an >> STATE_SCHED_BIT, STATE_SCHED, STATE_RUN_BIT, and STATE_RUN_BIT >> to complement each other when checking for values or setting bits. > No, keep the enum and just use 1 << STATE_... here. > >> I added some extra code so that I could reliabily trigger the error paths >> and got: >> >> (XEN) pt_pirq_softirq_active: is 0 (debug: 1) >> >> This is the first ever usage of pt_pirq_create_bind and for fun the >> code returns 'false' >> >> (XEN) Assertion '!preempt_count()' failed at preempt.c:37 >> (XEN) ----[ Xen-4.5.0-rc x86_64 debug=y Not tainted ]---- >> (XEN) CPU: 9 >> (XEN) RIP: e008:[] ASSERT_NOT_IN_ATOMIC+0x22/0x67 >> (XEN) RFLAGS: 0000000000010202 CONTEXT: hypervisor >> (XEN) rax: ffff82d080320d20 rbx: ffff8302fb126470 rcx: 0000000000000001 >> (XEN) rdx: 00000032b8d99300 rsi: 000000000000000a rdi: ffff8303149670b8 >> (XEN) rbp: ffff8303390a7bd8 rsp: ffff8303390a7bd8 r8: 0000000000000000 >> (XEN) r9: 0000000000000000 r10: 00000000fffffffd r11: ffff82d08023e5e0 >> (XEN) r12: ffff83025c126700 r13: ffff830314967000 r14: 0000000000000030 >> (XEN) r15: ffff83025c126728 cr0: 0000000080050033 cr4: 00000000000026f0 >> (XEN) cr3: 000000031b4b7000 cr2: 0000000000000000 >> (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: e010 cs: e008 >> (XEN) Xen stack trace from rsp=ffff8303390a7bd8: >> (XEN) ffff8303390a7c98 ffff82d080149335 ffff8303390a7ca8 ffff82d08016d0e6 >> (XEN) 0000000000000001 0000000000000206 00000003c0027c38 ffff8303390a7d88 >> (XEN) 00000072390a7c68 ffff830312e5f4d0 0000000000000000 0000001000000003 >> (XEN) 0000000000000246 ffff8303390a7c58 ffff82d08012ce66 ffff8303390a7e48 >> (XEN) 0000007f390a7c88 ffff8303149670b8 fffffffffffffffd fffffffffffffffd >> (XEN) 00007f81af369004 ffff830314967000 ffff8303390a7e38 0000000000000008 >> (XEN) ffff8303390a7d78 ffff82d080160131 ffff8303390a7cd8 ffff830302526f10 >> (XEN) ffff8303390a7ce8 0000000000000073 ffff830330907324 ffff830330907300 >> (XEN) ffff8303390a7d08 ffff82d08016dc79 ffff8303390a7cf8 ffff82d08013b469 >> (XEN) ffff8303390a7d28 ffff82d08016e628 ffff8303390a7d28 ffff830314967000 >> (XEN) 000000000000007f 0000000000000206 ffff8303390a7dc8 ffff82d0801711dc >> (XEN) ffff8303390a7d88 ffff82d08011e203 0000000000000202 fffffffffffffffd >> (XEN) 00007f81af369004 ffff830314967000 ffff8800331eb488 0000000000000008 >> (XEN) ffff8303390a7ef8 ffff82d0801048e8 ffff830302526f10 ffff83025c126700 >> (XEN) ffff8303390a7dc8 ffff830314967000 00000000ffffffff 0000000000000000 >> (XEN) ffff8303390a7e98 ffff8303390a7e70 ffff8303390a7e48 ffff82d080184019 >> (XEN) ffff8303390a7f18 ffffffff8158b0de ffff8303390a7e98 ffff8303149670b8 >> (XEN) ffff83030000007f ffff82d080191105 000000730000f800 ffff8303390a7e74 >> (XEN) ffff8300bf52e000 000000000000000d 00007f81af369004 ffff8300bf52e000 >> (XEN) 0000000a00000026 0000000000f70002 000000020000007f 00007f8100000002 >> (XEN) Xen call trace: >> (XEN) [] ASSERT_NOT_IN_ATOMIC+0x22/0x67 >> (XEN) [] pt_irq_create_bind+0xf7/0x5c2 >> (XEN) [] arch_do_domctl+0x1131/0x23e0 >> (XEN) [] do_domctl+0x1934/0x1a9c >> (XEN) [] syscall_enter+0xeb/0x145 >> >> which is entirely due to holding the 'domctl_lock_acquire',' >> 'rcu_read_lock', and 'pcidevs_lock' lock. >> >> It seems that the approach of waiting for kicking 'process_pending_softirq' >> is not good as other softirq might want to grab any of those locks >> at some point and be unhappy. >> >> Ugh. > Ugly. I'd suggest keeping the ASSERT_NOT_IN_ATOMIC() in place > but commented out, briefly explaining that this can't be used with > since the domctl lock is being held. That lock is not really going to be > acquired by softirq handlers, but we also shouldn't adjust the > generic macro to account for this one special case. > > Jan The domctl lock is a hot lock, and looping with it held like this will further starve other toolstack operations. What is the likelihood that this loop will actually be used? I am guessing fairly rare, although it looks more likely to happen for an interrupt which is firing frequently? ~Andrew