Re: [PATCH v8 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq (v8)

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/10/14 09:32, Jan Beulich wrote:
>>>> On 24.10.14 at 22:55, <konrad.wilk@oracle.com> wrote:
>> On Fri, Oct 24, 2014 at 11:09:59AM +0100, Jan Beulich wrote:
>>>>>> On 24.10.14 at 03:58, <konrad.wilk@oracle.com> wrote:
>>>> On Thu, Oct 23, 2014 at 10:36:07AM +0100, Jan Beulich wrote:
>>>>>>>> On 21.10.14 at 19:19, <konrad.wilk@oracle.com> 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:[<ffff82d08011d6db>] 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)    [<ffff82d08011d6db>] ASSERT_NOT_IN_ATOMIC+0x22/0x67
>> (XEN)    [<ffff82d080149335>] pt_irq_create_bind+0xf7/0x5c2
>> (XEN)    [<ffff82d080160131>] arch_do_domctl+0x1131/0x23e0
>> (XEN)    [<ffff82d0801048e8>] do_domctl+0x1934/0x1a9c
>> (XEN)    [<ffff82d08022c71b>] 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