>>> On 03.08.16 at 17:28, <george.dun...@citrix.com> wrote: > On 03/08/16 16:25, Jan Beulich wrote: >>>>> On 03.08.16 at 17:11, <george.dun...@eu.citrix.com> wrote: >>> On Tue, Aug 2, 2016 at 5:12 PM, Jan Beulich <jbeul...@suse.com> wrote: >>>>>>> On 02.08.16 at 17:49, <roger....@citrix.com> wrote: >>>>> On Tue, Aug 02, 2016 at 11:47:22AM +0200, Roger Pau Monne wrote: >>>>>> On Fri, Jul 29, 2016 at 05:47:24PM +0100, Andrew Cooper wrote: >>>>>>> As this is for the construction of dom0, it would be better to take a >>>>>>> preemptible pointer, loop in construct_dom0(), with a >>>>>>> process_pending_softirqs() call in between. >>>>>> >>>>>> Now fixed. >>>>> >>>>> Hm, I have to stand corrected, using hypercall_preempt_check (as >>>>> any of the *_set_allocation function use), is not safe at this point: >>>>> >>>>> (XEN) ----[ Xen-4.8-unstable x86_64 debug=y Tainted: C ]---- >>>>> (XEN) CPU: 0 >>>>> (XEN) RIP: e008:[<ffff82d08022fd47>] >>> hap.c#local_events_need_delivery+0x27/0x40 >>>>> (XEN) RFLAGS: 0000000000010246 CONTEXT: hypervisor >>>>> (XEN) rax: 0000000000000000 rbx: ffff83023f5a5000 rcx: >>>>> ffff82d080312900 >>>>> (XEN) rdx: 0000000000000001 rsi: ffff83023f5a56c8 rdi: >>>>> ffff8300b213d000 >>>>> (XEN) rbp: ffff82d080307cc8 rsp: ffff82d080307cc8 r8: >>>>> 0180000000000000 >>>>> (XEN) r9: 0000000000000000 r10: 0000000000247000 r11: >>>>> ffff82d08029a5b0 >>>>> (XEN) r12: 0000000000000011 r13: 00000000000023ac r14: >>>>> ffff82d080307d4c >>>>> (XEN) r15: ffff83023f5a56c8 cr0: 000000008005003b cr4: >>>>> 00000000001526e0 >>>>> (XEN) cr3: 00000000b20fc000 cr2: 0000000000000000 >>>>> (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: 0000 cs: e008 >>>>> (XEN) Xen code around <ffff82d08022fd47> >>> (hap.c#local_events_need_delivery+0x27/0x40): >>>>> (XEN) 0d ad fa ff 48 8b 47 08 <80> 38 00 74 09 80 78 01 00 0f 94 c0 eb >>>>> 02 31 >>> c0 >>>>> (XEN) Xen stack trace from rsp=ffff82d080307cc8: >>>>> (XEN) ffff82d080307d08 ffff82d08022fc47 0000000000000000 >>>>> ffff83023f5a5000 >>>>> (XEN) ffff83023f5a5648 0000000000000000 ffff82d080307d4c >>>>> 0000000000002400 >>>>> (XEN) ffff82d080307d38 ffff82d08020008c 00000000000ffffd >>>>> ffff8300b1efd000 >>>>> (XEN) ffff83023f5a5000 ffff82d080307d4c ffff82d080307d78 >>>>> ffff82d0802cad30 >>>>> (XEN) 0000000000203000 ffff83023f5a5000 ffff82d0802bf860 >>>>> 0000000000000000 >>>>> (XEN) 0000000000000001 ffff83000008bef0 ffff82d080307de8 >>>>> ffff82d0802c91e0 >>>>> (XEN) ffff82d080307de8 ffff82d080143900 ffff82d080307de8 >>>>> 0000000000000000 >>>>> (XEN) ffff83000008bf00 ffff82d0802eb480 ffff82d080307dc4 >>>>> ffff82d08028b1cd >>>>> (XEN) ffff83000008bf00 0000000000000000 0000000000000001 >>>>> ffff83023f5a5000 >>>>> (XEN) ffff82d080307f08 ffff82d0802bf0c9 0000000000000000 >>>>> 0000000000000000 >>>>> (XEN) 0000000000000000 ffff82d080307f18 ffff83000008bee0 >>>>> 0000000000000001 >>>>> (XEN) 0000000000000001 0000000000000001 0000000000000000 >>>>> 0000000000100000 >>>>> (XEN) 0000000000000001 0000000000247000 ffff83000008bef4 >>>>> 0000000000100000 >>>>> (XEN) ffff830100000000 0000000000247001 0000000000000014 >>>>> 0000000100000000 >>>>> (XEN) ffff8300ffffffec ffff83000008bef0 ffff82d0802e0640 >>>>> ffff83000008bfb0 >>>>> (XEN) 0000000000000000 0000000000000000 0000000000000111 >>>>> 0000000800000000 >>>>> (XEN) 000000010000006e 0000000000000003 00000000000002f8 >>>>> 0000000000000000 >>>>> (XEN) 00000000ad5c0bd0 0000000000000000 0000000000000001 >>>>> 0000000000000008 >>>>> (XEN) 0000000000000000 ffff82d080100073 0000000000000000 >>>>> 0000000000000000 >>>>> (XEN) 0000000000000000 0000000000000000 0000000000000000 >>>>> 0000000000000000 >>>>> (XEN) Xen call trace: >>>>> (XEN) [<ffff82d08022fd47>] hap.c#local_events_need_delivery+0x27/0x40 >>>>> (XEN) [<ffff82d08022fc47>] hap_set_allocation+0x107/0x130 >>>>> (XEN) [<ffff82d08020008c>] paging_set_allocation+0x4c/0x80 >>>>> (XEN) [<ffff82d0802cad30>] domain_build.c#hvm_setup_p2m+0x70/0x1a0 >>>>> (XEN) [<ffff82d0802c91e0>] domain_build.c#construct_dom0_hvm+0x60/0x120 >>>>> (XEN) [<ffff82d0802bf0c9>] __start_xen+0x1ea9/0x23a0 >>>>> (XEN) [<ffff82d080100073>] __high_start+0x53/0x60 >>>>> (XEN) >>>>> (XEN) Pagetable walk from 0000000000000000: >>>> >>>> Sadly you don't make clear what pointer it is that is NULL at that point. >>> >>> It sounds from what he says in the following paragraph like current is NULL. >> >> I don't recall us re-setting current to this late in the boot process. >> Even during early boot we set it to a bogus non-NULL value rather >> than NULL. >> >>>>> I've tried setting current to d->vcpu[0], but that just makes the call to >>>>> hypercall_preempt_check crash in some scheduler assert. In any case, I've >>>>> added the preempt parameter to the paging_set_allocation function, but I >>>>> don't plan to use it in the domain builder for the time being. Does that >>>>> sound right? >>>> >>>> Not really, new huge latency issues like this shouldn't be reintroduced; >>>> we've been fighting hard to get rid of those (and we still occasionally >>>> find some no-one had noticed before). >>> >>> You mean latency in processing softirqs? >>> >>> Maybe what we need to do is to make local_events_need_delivery() safe >>> to call at this point by having it return 0 if current is NULL rather >>> than crashing? >> >> That would have the same effect - no softirq processing, and hence >> possible time issues on huge systems. > > No, local_events_delivery() only checks to see if the current vcpu has > outstanding virtual interrupts. The other half of > hypercall_preempt_check() checks for softirqs, which doesn't appear to > rely on having current available.
Good point, but - current should nevertheless not be NULL (afaict at least), - hypercall_preempt_check() is probably the wrong construct, as we're no in a hypercall. The latter of course could be addressed by, as you did suggest, some refinement to one of the pieces it's being made up from, but I'm not sure that would be better than perhaps making its invocation conditional (with some better alternative in the "else" case) in hap_set_allocation(). Not the least because any adjustment to hypercall_preempt_check() itself would affect all other of its users. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel